4 ms·
The correct fix is to change the type of the argument to allocatebufs to size_t. Casts are a code smell, and often the best way to fix these warnings is to work
by wahern 7y ago
The correct fix is to change the type of the argument to allocatebufs to size_t. Casts are a code smell, and often the best way to fix these warnings is to work your way backward through the code fixing up the types to something more appropriate. Ideally you'd reach the point where someone could induce a negative value, but in any event simply changing the argument type would have sufficed to safeguard the limit check.
That said, the latest release of glibc would have failed on a negative expression promoted to unsigned:
> Memory allocation functions malloc, calloc, realloc, reallocarray, valloc, pvalloc, memalign, and posix_memalign fail now with total object size larger than PTRDIFF_MAX. This is to avoid potential undefined behavior with pointer subtraction within the allocated object, where results might overflow the ptrdiff_t type.
https://sourceware.org/ml/libc-alpha/2019-08/msg00029.html https://sourceware.org/ml/libc-alpha/2019-08/msg00029.html
The new glibc behavior doesn't help with the multiplication overflow, though, as it could have wrapped to a positive value (notwithstanding the fact that such wrapping is undefined). Which is why if I saw code like this I would've worked my way back to all the call sites to understand why the argument to allocatebufs wasn't size_t to begin with. Maybe they were C++ programmers--for decades "best practice" in C++ was to use int for the size of objects.
Arithmetic overflow leading to potential buffer overflow is still a problem even in languages like Rust. Nobody should ever add an integer cast somewhere without having done their due diligence. Which is why stricter arithmetic type checking can sometimes be a double-edged sword--too many people will add an explicit cast and move on.
- jchw 7y agoFair, I only meant to fix the warning, not improve the code quality. I think the problem here is probably something more along the lines of a bad abstraction, as I get the feeling the idea is "I have an int, I need to safely allocate this much memory within limits." If you just change the interface to size_t, that would push the problem down further, but at some point I'd ask why bother with this helper function.