Strengthen undefined behavior prevention in checkSignedBitfieldOverflow#2418
Conversation
Signed-off-by: Fusl <[email protected]>
Fusl
force-pushed
the
fusl-bitops-sub-overflow-fix
branch
from
d2e1473 to
fb68a39
Compare
zuiderkwast
left a comment
zuiderkwast
left a comment
There was a problem hiding this comment.
Would it make sense to add bitfield 0 set i64 0 1 to some test case just to verify the fix?
I tried adding a test case for this but couldn't trigger it outside of a binary built with afl-clang, maybe I was doing something wrong and didn't supply correct compilation flags when building a custom non-afl binary? |
| int64_t maxincr = (int64_t)((uint64_t)max - (uint64_t)value); | ||
| int64_t minincr = (int64_t)((uint64_t)min - (uint64_t)value); |
There was a problem hiding this comment.
The cast to signed can be implicit, for brevity?
| int64_t maxincr = (int64_t)((uint64_t)max - (uint64_t)value); | |
| int64_t minincr = (int64_t)((uint64_t)min - (uint64_t)value); | |
| int64_t maxincr = (uint64_t)max - (uint64_t)value; | |
| int64_t minincr = (uint64_t)min - (uint64_t)value; |
There was a problem hiding this comment.
I made this explicit since clang-tidy otherwise complains about it:
There was a problem hiding this comment.
Oh, is implicit cast different to explicit? I didn't know...
Let's keep it explicit then.
Out of curiosity, do you know where this stuff is mentioned in the C standards?
There was a problem hiding this comment.
There was a problem hiding this comment.
That explicit cast specifically tells clang-tidy "i know what im doing, please stop complaining", there's no other side effect and the explicit and implicit format both get compiled down to the same byte code.
Ah, I don't know. We can skip the test. |
…ow (valkey-io#2418) This one was found by [afl++](https://github.com/AFLplusplus/AFLplusplus). Executing `bitfield 0 set i64 0 1` triggers UBSan at the `int64_t minincr = min - value;` calculation. To fix the undefined behavior in the `minincr` calculation and strengthen the protection in the `maxincr` calculation, we cast both, the minuend and the subtrahend, to an unsigned int, do the calculation, and then cast the result back into a signed int. Signed-off-by: Fusl <[email protected]>
2 participants
This one was found by afl++. Executing
bitfield 0 set i64 0 1triggers UBSan at theint64_t minincr = min - value;calculation. To fix the undefined behavior in theminincrcalculation and strengthen the protection in themaxincrcalculation, we cast both, the minuend and the subtrahend, to an unsigned int, do the calculation, and then cast the result back into a signed int.