5 ms·

It seems like this `if (len > PAGE_SIZE - 2 - size);` is less natural than the correct code of `size + len + 2 > PAGE_SIZE`. Can anyone more familiar with C com

It seems like this `if (len > PAGE_SIZE - 2 - size);` is less natural than the correct code of `size + len + 2 > PAGE_SIZE`. Can anyone more familiar with C comment on why someone would write it the first way instead of the second way?

pixelbeat__ 5y ago
Overflow https://www.pixelbeat.org/programming/gcc/integer_overflow.html https://www.pixelbeat.org/programming/gcc/integer_overflow.h...
AceJohnny2 5y ago
As a long-time embedded C coder: I agree that (with the added benefit on hindsight) the original code "smells". I'm well-tuned to consider overflows, and adding numbers together instinctively makes me consider whether there's a risk... Similarly, subtracting numbers should make one consider underflows. Perhaps the original author was used to working with signed values and forgot to consider the risk of underflow with these unsigned variables. (I used to have C's type promotion rules on quickdraw, but I very soon learned that if I needed to refer to them, I was probably doing something wrong)
comex 5y ago
The problem is that neither option is correct if `size` and `len` can both be arbitrary unsigned integers. They can both overflow (counting underflow as a type of overflow). To be correct regardless of their values, you would have to write something like: if (size + len < size || size + len > PAGE_SIZE - 2) But broadly speaking, adding explicit overflow checks everywhere makes code more verbose, and it isn't even a panacea, since it can be non-obvious whether any given overflow check is even correct (especially when C's arcane type conversion rules come into play). Plus, in theory every extra check makes the code larger and slower. Therefore, it's common to see C code try to perform computations in a way that can't overflow in the first place. But that often requires putting them in a less natural form, as well as making assumptions about which things are bounded in which ranges. For example, even in the above example that does have an overflow check, I had to unnaturally put the 2 on the right hand side of the equation rather than the left, and assume that `PAGE_SIZE - 2` doesn't underflow. The fully natural computation would require two overflow checks: if (size + len < size || size + len + 2 < size + len || size + len + 2 > PAGE_SIZE) On the other hand, the buggy single comparison that was actually used: if (len > PAGE_SIZE - 2 - size) would be correct, and reasonably idiomatic, if `size` was known to be bounded below `PAGE_SIZE - 2`. Perhaps the author thought it was. This is really an unfortunate limitation of C, though. It would be nice to be able to succinctly express "if size + len + 2 > PAGE_SIZE or the computation overflowed, then return error". From the hardware's perspective, that sort of check is really cheap; from the compiler's perspective, it's ripe for optimization. So if C had an ergonomic overflow-check feature, you could standardize your codebase on using it for any calculation that's remotely related to buffer sizes, at very little performance cost. With GCC extensions you can at least do this: size_t tmp; if (__builtin_add_overflow(size, len, &tmp) || __builtin_add_overflow(tmp, 2, &tmp) || tmp > PAGE_SIZE) Using these builtins standardizes the form of overflow checks and avoids "is the overflow check correct" worries. But the ergonomics are arguably even worse, since you need to declare a temporary variable, and use long function names instead of normal mathematical operators.

pixelbeat__ 5y ago

Overflow https://www.pixelbeat.org/programming/gcc/integer_overflow.html https://www.pixelbeat.org/programming/gcc/integer_overflow.h...

AceJohnny2 5y ago

As a long-time embedded C coder: I agree that (with the added benefit on hindsight) the original code "smells". I'm well-tuned to consider overflows, and adding numbers together instinctively makes me consider whether there's a risk... Similarly, subtracting numbers should make one consider underflows. Perhaps the original author was used to working with signed values and forgot to consider the risk of underflow with these unsigned variables. (I used to have C's type promotion rules on quickdraw, but I very soon learned that if I needed to refer to them, I was probably doing something wrong)

comex 5y ago

The problem is that neither option is correct if `size` and `len` can both be arbitrary unsigned integers. They can both overflow (counting underflow as a type of overflow). To be correct regardless of their values, you would have to write something like: if (size + len < size || size + len > PAGE_SIZE - 2) But broadly speaking, adding explicit overflow checks everywhere makes code more verbose, and it isn't even a panacea, since it can be non-obvious whether any given overflow check is even correct (especially when C's arcane type conversion rules come into play). Plus, in theory every extra check makes the code larger and slower. Therefore, it's common to see C code try to perform computations in a way that can't overflow in the first place. But that often requires putting them in a less natural form, as well as making assumptions about which things are bounded in which ranges. For example, even in the above example that does have an overflow check, I had to unnaturally put the 2 on the right hand side of the equation rather than the left, and assume that `PAGE_SIZE - 2` doesn't underflow. The fully natural computation would require two overflow checks: if (size + len < size || size + len + 2 < size + len || size + len + 2 > PAGE_SIZE) On the other hand, the buggy single comparison that was actually used: if (len > PAGE_SIZE - 2 - size) would be correct, and reasonably idiomatic, if `size` was known to be bounded below `PAGE_SIZE - 2`. Perhaps the author thought it was. This is really an unfortunate limitation of C, though. It would be nice to be able to succinctly express "if size + len + 2 > PAGE_SIZE or the computation overflowed, then return error". From the hardware's perspective, that sort of check is really cheap; from the compiler's perspective, it's ripe for optimization. So if C had an ergonomic overflow-check feature, you could standardize your codebase on using it for any calculation that's remotely related to buffer sizes, at very little performance cost. With GCC extensions you can at least do this: size_t tmp; if (__builtin_add_overflow(size, len, &tmp) || __builtin_add_overflow(tmp, 2, &tmp) || tmp > PAGE_SIZE) Using these builtins standardizes the form of overflow checks and avoids "is the overflow check correct" worries. But the ergonomics are arguably even worse, since you need to declare a temporary variable, and use long function names instead of normal mathematical operators.