Skip to contentSkip to conversation

sval_type_max() sadness

PATCH John Levon 2 participants 2 messages Dec 11, 2019 – Dec 12, 2019

lore
Share & tools
lore

John Levon

author
#0 lore raw

Robert (Cc:ed) found a false positive from smatch, which boils down to:

```
#include <limits.h>

int
main(int argc, char *argv[])
{
	long double sec = 0;
	// also __int128 !

        if (sec > (float)INT_MAX) {
		0;
	}
}
```
$ ./src/smatch/smatch -m32 test.smatch.c 
test.smatch.c:9 main() warn: impossible condition '(sec > 2147483647) =>
(s32min-s32max > s32max)'

A cheesy fix here is:

$ git diff
smatch_type.c +5 -1
index 305a0b5c..1e289363 100644
@@ -411,7 +411,11 @@ sval_t sval_type_max(struct symbol *base_type)
                base_type = &llong_ctype;
        ret.type = base_type;
 
-       ret.value = (~0ULL) >> (64 - type_positive_bits(base_type));
+       int pos_bits = type_positive_bits(base_type);
+       if (pos_bits > 63)
+               pos_bits = 63;
+
+       ret.value = (~0ULL) >> (64 - pos_bits);
        return ret;
 }
 

The issue being that for these types, type_positive_bits() is 64 or more. If only there were a tool to warn about negative shift operands :)

But type_positive_bits() is used all over, so I have no idea if there's a lot of other problems.

Or, if this patch is OK on its own, I can add a couple of tests and send a proper patch over.

Thoughts?

thanks
john

#1 lore raw

Thanks for the test case!

Of course, the history here is that the Linux kernel doesn't have floats so I never added support for it. I have this friend who is an expert on floats and people consult him when they are designing chips. He was explaining his work to me and I realized that I don't have the foggiest clue how floats work at all...

Anyway, I've tried to add floats to Smatch. It turned out not so hard as I imagined, but I know there are places that I missed. Probably it changes from a false positive into a crashing bug now. :/ The fix is almost always going to be to run valgrind to see which function crashes and then add a check:

	if (type_is_fp(estate_type(state)))
		return false;

Do a pull and take a look. Tell me what you think. I will hack on this tomorrow as well.

regards,
can carpenter