| Message ID | 2390fdc8-13fa-4456-ab67-44f0744db412@moroto.mountain (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | SUNRPC: clean up integer overflow check | expand |
On 6/30/23 12:46, Dan Carpenter wrote: > Use size_mul() to prevent the integer overflow. It silences the warning > and it's cleaner as well. I'm OK with this. But I still don't understand why the original warning was a compiler (actually two compilers we've tried) bug. Dmitry
We were kind of having a discussion about static checkers earlier...
The point of static checkers is to find code that doesn't behave as the
authors intended. It can be tricky to determine what is intentional or
not.
If you have a condition which can never be true then that's fairly
suspicious. Why would we write a condition like that? But you have to
look at the context.
if (!p) {
Maybe that's part of a macro and even though "p" can't be NULL here, it
can be NULL in other places. So we can't warning about bogus NULL
checks if they are inside a macro. There is a valid reason to check.
With NULL checks, people add a lot of unnecessary NULL checks just out
of caution. So even though the NULL checks are unnecessary, they aren't
harmful. You only want to warn about them if there is something else to
make them suspicious. For example, the if the pointer is an error
pointer but you're checking for NULL then probably that's not
intentional. Or people think that list_for_each() loops exit with the
iterator set to NULL but that's not how it works so those checks should
trigger a warning.
Maybe we have a condition "if (unsigned_val < 0", and that's very
suspicious, but maybe the context is:
if (unsigned_val < 0 || unsigned_val > 10)
In that context we can see what the intention was, and that condition
will clamp the value regardless of the type of unsigned_val so it works
as intended. It's not a bug. The static checker should not warn about
that.
Another impossible condition could look like:
if (x & SOMETHING_ZERO) {
Quite often the SOMETHING_ZERO is because it depends on the .config. So
in Smatch what I do is I make a list of macros which can be zero or not
depending on the .config. In a lot of cases, what was intended was:
if (x & (1 << SOMETHING_ZERO)) {
That's a useful warning, but you have to create that list of allowed
defines otherwise it generates too many false positives.
So here we have:
if (u32_val > SOMETHING) {
The condition is impossible when the code is compiled on a 64-bit
system, but a human reading the code can see that it is an integer
overflow check and Smatch can see that too. I haven't looked at Clang
or GCC internals to see how hard it would be to fix them. Maybe it's
really hard to fix?
With static checkers we have to accept the checker is going to get it
wrong some times. The checker is going to warn about code that works
as intended and it's going to miss over 97% of bugs. But hopefully
it generates enough warnings to be worth the time it takes and the
number of false positives is small enough so you can keep up.
regards,
dan carpenter
On Fri, 2023-06-30 at 12:46 +0300, Dan Carpenter wrote: > This integer overflow check works as intended but Clang and GCC and warn > about it when compiling with W=1. > > include/linux/sunrpc/xdr.h:539:17: error: comparison is always false > due to limited range of data type [-Werror=type-limits] > > Use size_mul() to prevent the integer overflow. It silences the warning > and it's cleaner as well. > > Reported-by: Dmitry Antipov <dmantipov@yandex.ru> > Closes: https://lore.kernel.org/all/20230601143332.255312-1-dmantipov@yandex.ru/ > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > --- > Btw, since the Clang developers are automatically CC'd, here is how I > silenced this type of false positive in Smatch: > > 1) Check that longs are 64 bit. > 2) Check that the right hand side has a SIZE_MAX. SIZE_MAX is defined > as -1UL so you want both the type and the value to match. > 3) Then on the other the other side, check that the type is uint. > > I'm looking at this code now in Smatch and it's kind of ugly, and also > there are some other places where I need to apply the same logic... > > include/linux/sunrpc/xdr.h | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/include/linux/sunrpc/xdr.h b/include/linux/sunrpc/xdr.h > index f89ec4b5ea16..dbf7620a2853 100644 > --- a/include/linux/sunrpc/xdr.h > +++ b/include/linux/sunrpc/xdr.h > @@ -775,9 +775,7 @@ xdr_stream_decode_uint32_array(struct xdr_stream *xdr, > > if (unlikely(xdr_stream_decode_u32(xdr, &len) < 0)) > return -EBADMSG; > - if (len > SIZE_MAX / sizeof(*p)) > - return -EBADMSG; > - p = xdr_inline_decode(xdr, len * sizeof(*p)); > + p = xdr_inline_decode(xdr, size_mul(len, sizeof(*p))); > if (unlikely(!p)) > return -EBADMSG; > if (array == NULL) Acked-by: Jeff Layton <jlayton@kernel.org>
On 6/30/23 15:13, Dan Carpenter wrote: > So here we have: > > if (u32_val > SOMETHING) { > > The condition is impossible when the code is compiled on a 64-bit > system This is exactly what the compilers warns about: $ cat test.c #include <stdint.h> int f (int *p, unsigned n) { return n > SIZE_MAX / sizeof (*p); } $ gcc -Wextra -c test.c test.c: In function âfâ: test.c:5:12: warning: comparison is always false due to limited range of data type [-Wtype-limits] 5 | return n > SIZE_MAX / sizeof (*p); | ^ $ gcc -m32 -Wextra -c test.c $ clang -Wextra -c test.c test.c:5:12: warning: result of comparison of constant 4611686018427387903 with expression of type 'unsigned int' is always false [-Wtautological-constant-out-of-range-compare] return n > SIZE_MAX / sizeof (*p); ~ ^ ~~~~~~~~~~~~~~~~~~~~~~ 1 warning generated. $ clang -m32 -Wextra -c test.c $ Where is a bug here? What the compiler developers are intended to fix? How the compilers should behave in this case? Dmitry
diff --git a/include/linux/sunrpc/xdr.h b/include/linux/sunrpc/xdr.h index f89ec4b5ea16..dbf7620a2853 100644 --- a/include/linux/sunrpc/xdr.h +++ b/include/linux/sunrpc/xdr.h @@ -775,9 +775,7 @@ xdr_stream_decode_uint32_array(struct xdr_stream *xdr, if (unlikely(xdr_stream_decode_u32(xdr, &len) < 0)) return -EBADMSG; - if (len > SIZE_MAX / sizeof(*p)) - return -EBADMSG; - p = xdr_inline_decode(xdr, len * sizeof(*p)); + p = xdr_inline_decode(xdr, size_mul(len, sizeof(*p))); if (unlikely(!p)) return -EBADMSG; if (array == NULL)
This integer overflow check works as intended but Clang and GCC and warn about it when compiling with W=1. include/linux/sunrpc/xdr.h:539:17: error: comparison is always false due to limited range of data type [-Werror=type-limits] Use size_mul() to prevent the integer overflow. It silences the warning and it's cleaner as well. Reported-by: Dmitry Antipov <dmantipov@yandex.ru> Closes: https://lore.kernel.org/all/20230601143332.255312-1-dmantipov@yandex.ru/ Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> --- Btw, since the Clang developers are automatically CC'd, here is how I silenced this type of false positive in Smatch: 1) Check that longs are 64 bit. 2) Check that the right hand side has a SIZE_MAX. SIZE_MAX is defined as -1UL so you want both the type and the value to match. 3) Then on the other the other side, check that the type is uint. I'm looking at this code now in Smatch and it's kind of ugly, and also there are some other places where I need to apply the same logic... include/linux/sunrpc/xdr.h | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)