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? |
Wouldn't call 200 lines out of millions of lines "many places". And Linux is written in GNU C, not standard C, so it is free to use gcc specific extensions.
It's as "bad" as using non-stardard builtins, which are also compiler dependent. I have a patch locally that adds I think if you want to add these checking in mpv, it should use standard C23 overflow checking functions directly in code, and the header can contain a shim that turns them into non-checked operations when the compiler is not C23. This mirrors what mpv is already doing with atomics which is C11 standard.
It introduces non-stardard builtins which has problems described above. So I think doing it for all integer operators to completely eliminate overflow would justify the benefit. |
I also prefer to not use non-standard builtins. We can instead switch the codebase to c23 and use standard functions. |
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.