5 ms·

I'm curious, wouldn't this also be caught by static code analysis tools, at least today? An assigment inside an if condition is both, most likely a mistake, and

I'm curious, wouldn't this also be caught by static code analysis tools, at least today? An assigment inside an if condition is both, most likely a mistake, and fairly easy to detect automatically.

rocqua 6y ago
I would guess this is part of the reason why most modern compilers will indeed emit a warning about assignment within if, for, and while - branch checks. At the same time, the standard implementation of strcpy is: while((*dst++ = *src++)); which has a legitimate reason for doing assignment inside the while condition. Then again, one could argue that the above code is 'too clever'. And I would probably agree.
- asddubs 6y ago
  could do this instead, right? do { *dst = *src; *dst++; *src++; } while(*dst);
  - deleted 6y ago
    [deleted]
  - josefx 6y ago
    I think you are not copying the terminating nul character.
    - FartyMcFarter 6y ago
      Unless the first character was null, in which case it would be ignored by the condition... Also, you don't need to dereference a pointer in order to increase it. The grandparent post's code is just nonsensical.
      - asddubs 6y ago
        haha, I was genuinely asking if it made sense, I don't usually do C. Maybe it helped illustrate that the code is too clever for me, at least. e: I submit my revised code: do { *dst = *src; src++; dst++; } while(*(dst - 1));
        asveikau 6y ago
        Looking at src[-1] would work but feels like poor form. My advice is stop trying to make do ... while work here, it isn't looking good. I feel like more idiomatic iterating through a string tends to loop on src not being a terminator. while (*src) { *dst = *src; ++src; ++dst; } *dst = '\0'; I feel like this is idiomatic C but needlessly verbose. Most people would combine the increment with the assignment. And most people would recognize putting it in the while condition as a common strcpy.
- ancgop 6y ago
  However they do not emit a warning if the assignment is parenthesized, like in the exploit. I think static analysis tools are the same, they would be way too chatty if they emitted warning for a parenthesized assignment. Static analysis already has way too many false positives as it stands. For a well maintained code base the rate can easily be 100% false positives, which gets annoying after some time.
lqet 6y ago
I think this is why there are parantheses around current->uid = 0. gcc has the option -Wparentheses, which gives a warning if you write something like this: if (a = b) doSomething; But there is no warning if you write it like this: if ((a = b)) doSomething; The convention is that with these unneeded parantheses, you are signalling that you actually want the assignment here. I would assume other static code analysis tools use this convention as well.

I'm curious, wouldn't this also be caught by static code analysis tools, at least today? An assigment inside an if condition is both, most likely a mistake, and fairly easy to detect automatically.

rocqua 6y ago

I would guess this is part of the reason why most modern compilers will indeed emit a warning about assignment within if, for, and while - branch checks. At the same time, the standard implementation of strcpy is: while((*dst++ = *src++)); which has a legitimate reason for doing assignment inside the while condition. Then again, one could argue that the above code is 'too clever'. And I would probably agree.

asddubs 6y ago
could do this instead, right? do { *dst = *src; *dst++; *src++; } while(*dst);
- deleted 6y ago
  [deleted]
- josefx 6y ago
  I think you are not copying the terminating nul character.
  - FartyMcFarter 6y ago
    Unless the first character was null, in which case it would be ignored by the condition... Also, you don't need to dereference a pointer in order to increase it. The grandparent post's code is just nonsensical.
    - asddubs 6y ago
      haha, I was genuinely asking if it made sense, I don't usually do C. Maybe it helped illustrate that the code is too clever for me, at least. e: I submit my revised code: do { *dst = *src; src++; dst++; } while(*(dst - 1));
      - asveikau 6y ago
        Looking at src[-1] would work but feels like poor form. My advice is stop trying to make do ... while work here, it isn't looking good. I feel like more idiomatic iterating through a string tends to loop on src not being a terminator. while (*src) { *dst = *src; ++src; ++dst; } *dst = '\0'; I feel like this is idiomatic C but needlessly verbose. Most people would combine the increment with the assignment. And most people would recognize putting it in the while condition as a common strcpy.
ancgop 6y ago
However they do not emit a warning if the assignment is parenthesized, like in the exploit. I think static analysis tools are the same, they would be way too chatty if they emitted warning for a parenthesized assignment. Static analysis already has way too many false positives as it stands. For a well maintained code base the rate can easily be 100% false positives, which gets annoying after some time.

lqet 6y ago

I think this is why there are parantheses around current->uid = 0. gcc has the option -Wparentheses, which gives a warning if you write something like this: if (a = b) doSomething; But there is no warning if you write it like this: if ((a = b)) doSomething; The convention is that with these unneeded parantheses, you are signalling that you actually want the assignment here. I would assume other static code analysis tools use this convention as well.