Ordering conditions

Some times Smatch cares about what order you put conditions in:

Example one:

ret = read(&val);
if (val > max || ret < 0)
        return -EINVAL;

Smatch will complain that “val” is used without being initialized. This code works but it is wrong. It should check for failure before checking if “val” is invalid.

Example B:

if (val * 4 > sizeof(buf) ||
    val > UINT_MAX / 4)
        return -EINVAL;
memcpy(buf, src, val * 4);

This code is correct because it checks that “val” is valid and it checks for integer overflows. Unfortunately, the conditions are in a nonsensical order and Smatch ignores comparisons where they have a potential integer overflow. Even though this code is correct, I am not going to lift a finger to silence the false positive. Shame upon you for writing code in backwards order!

Example 3:

if (x != y ||
    x > sizeof(buf))
        return -EINVAL;
memcpy(buf, src, y);

This code is correct but Smatch will give a false positive warning for it. There are three ways to silence the false positive. 1) Check for “if (y > sizeof(buf))” instead of “x”. 2) Reverse the order of the conditions. 3) Fix Smatch. The only correct way to address this is option #3 which is to fix Smatch. Unfortunately, that’s easier said than done.

What we need to do is say, “If we learn that x > sizeof(buf), then we need to update all the values which are equal to x to also be > sizeof(buf)”. There are three ways to know that values are equal.

1) Using the smatch_ssa.c module. The SSA module only tracks assignments, not == equality. Also smatch_extra.c isn’t written to support SSA data so that can’t work here.

2) Using estate_related() data. But the truth is that the estate_related() stuff was written a long time ago. I would have written it as a separate module now. It’s a mess. There is no way to get any visibility and track what estate_related() data is doing which makes it hard to debug. I want to delete it and replace it with something else.

3) Using the smatch_comparison.c module. The problem here is that I worry that using smatch_comparison.c would be slow. I don’t have any data to support that, but my gut says it would be slow.

So I guess what I want to do is write a replacement for estate_related() and use it here. Eventually I will silence this warning. But for now there isn’t an easy way.