2

I have came into some legacy code while we are migrated from AIX to Linux Redhat and found Some strange condition for strcmp that's seems was compiled fine for AIX but gave segmentation fault for Linux

void conv_virgin(char original_number[20], char normalised_number[20],char b_imsi[20]) {
        if(strlen(original_number)<2||strcmp(original_number,""||strlen (b_imsi)<6)==0)
        {
               strcpy(normalised_number,original_number);
               return;
        }
}

The line seems compiled fine for AIX as we have the shared object running since more than 2 year 

strcmp(original_number,""||strlen (b_imsi)<6)==0

What am I missing ? The intended code was to check for two fields which is corrected now to 

if(strlen(original_number)<2||strlen (b_imsi)<6)
Mustafa Mahmod
  • 77
  • 7
  • 7
    To be honest, I don't even understand what the original stetement `strcmp(original_number,""||strlen (b_imsi)<6)==0` is supposed to mean. In the context of `||`, `""` evaluates to a true value (i. e. 1), the `strlen()` call is skipped and 1 is passed as a string to `strcmp()`. This is undefined behaviour, and so the program is right to crash. What the original author could have meant to say is unknown to me. – glglgl Nov 21 '19 at 08:55
  • 3
    Besides causing a segmentation fault you should get some compiler warnings. `strcmp` expects 2 strings. You provide a string and the result of a logic OR which is of type integer. 0 or 1 are no good addresses for strings. – Gerhardh Nov 21 '19 at 08:56
  • The issue it doesn't give compilation warning in aix ,the intention might be to check for empty orignal_number which has missed the right parenthesis and ==0 `strcmp(original_number,"")==0 ||strlen` – Mustafa Mahmod Nov 21 '19 at 08:59
  • 1
    I assume it was code like `strcmp(original_number,"")==0||strlen (b_imsi)<6` which got corrupted by marking some text in the editor and dragging into another position with the mouse accidentally. – Gerhardh Nov 21 '19 at 08:59
  • 1
    `strcmp(original_number,""||strlen (b_imsi)<6)==0` is probably suppose to be `strcmp(original_number,"")==0||strlen (b_imsi)<6` – ikegami Nov 21 '19 at 09:00
  • 4
    Address zero us readable on AIX (there starts the kernel), but not readable on Linux. (As it has been said, your code has never been okay.) – Lorinczy Zsigmond Nov 21 '19 at 09:01
  • I doubled check it gives some warning ,but this code is running fine for 1 year ,why not crashed and crached only in unix that's a mystery for me `"mvno_change.c", line 149.64: 1506-280 (W) Function argument assignment between types "const char*" and "int" is not allowed.` – Mustafa Mahmod Nov 21 '19 at 09:07
  • @Mat you are correct to linux redhat ,sorry for that – Mustafa Mahmod Nov 21 '19 at 09:10
  • @Lorinczy Zsigmond so you can dereference NULL pointer on AIX without getting a segfault? – Jabberwocky Nov 21 '19 at 09:40
  • @LorinczyZsigmond got it: https://stackoverflow.com/questions/8990311/handling-null-pointers-on-aix-with-gcc-c – Jabberwocky Nov 21 '19 at 09:42
  • @Jabberwocky Yes, read from address zero is possible (though write is not) – Lorinczy Zsigmond Nov 21 '19 at 09:42
  • 1
    @MustafaMahmod to summarize: the behaviour of `strcmp` is not different on AIX than on Linux, but on AIX you can dereference a NULL pointer (or other very low addresses) wihout getting a segfault, but on Linux you can't. See https://stackoverflow.com/questions/8990311/handling-null-pointers-on-aix-with-gcc-c. Mystery solved. COnclusion: always compile with warnings enabled and treat them as errors – Jabberwocky Nov 21 '19 at 09:45

1 Answers1

1

It looks like a typo in the if statement

if(strlen(original_number)<2||strcmp(original_number,""||strlen (b_imsi)<6)==0)

The condition strlen (b_imsi)<6 is never evaluated because the address of the string literal "" is never equal to NULL. So only the first operand of the logical expression is evaluated

""||strlen (b_imsi)<6

It seems the if statement should be written like

if(strlen(original_number)<2||strcmp(original_number,"" ) ||strlen (b_imsi)<6 )

or like (less likely)

if(strlen(original_number)<2|| strcmp(original_number,"" ) == 0 ||strlen (b_imsi)<6)

In any case the function is very bad. At least it should be declared like

void conv_virgin( const char original_number[20], char normalised_number[20], const char b_imsi[20]);

Or like

void conv_virgin( const char original_number[static 20], char normalised_number[static 20], const char b_imsi[static 20]);
Vlad from Moscow
  • 301,070
  • 26
  • 186
  • 335
  • 1
    `""||whatever` doesn't result in the character pointer needed for strcmp though, so it wouldn't compile cleanly on a sane, compliant compiler. – Lundin Nov 21 '19 at 15:28
  • `||strlen (b_imsi)<6` was clearly intended to be inserted after the `==0` but was put in the wrong place by whoever added it. Indeed this should not even compile. But the whole condition is nonsensical. – R.. GitHub STOP HELPING ICE Nov 21 '19 at 16:38