Overengineer a fix for the EDL UB slop#18121
Conversation
Add a new MP_CKD_MUL macro which does a multiplication with overflow checking, backed by either the C23 ckd_mul standard function, or the GCC/Clang __builtin_mul_overflow otherwise.
Add a new function-like macro MP_SATURATE_MUL. It multiplies a * b and stores it in pointer "result", but sets "result" to its type's maximum value if it overflows. Evaluates to true if it saturated, otherwise false.
Use the new MP_SATURATE_MUL to prevent hls_bitrate from overflowing when being multiplied by 8. Fixes: mpv-player#18120
CounterPillow
force-pushed
the
saturating-mul
branch
from
492cec5 to
7e7934c
Compare
|
Unchecked overflows are already so prevalent in mpv anyway, I don't see a benefit of this over making In practice, no one uses these checked operators (other than stuffs like MISRA compliance) when |
Making everything that does arithmetic do 64 bit arithmetic does not fix the problem generally. It just kicks the can down the road hoping nobody ever finds a reason to make one of the two operands something other than 32 bits. There are also genuine hot code paths where the needless 64-bit arithmetic is unpleasant on platforms that don't have fast 64-bit arithmetic, e.g. armv7.
Linux uses them in many places:
Making code correctness rely on the passing of additional compile-time parameters is a bad suggestion. |
Can't we change code incrementally? Does it have to be done for all integer operators in mpv code in this PR? |
3 participants
Prefer the C23 function for checked multiply where it's available by wrapping it in a macro with the same semantics, and fall back to the GCC/Clang one otherwise. If neither is there, the build breaks because whatever compiler that is is probably shit anyway.
The checked multiply is then used to implement a saturating multiply.
Finally, fix the reported issue #18120 with this.