Page MenuHomePhabricator

[analyzer] CStringChecker: Fix overly eager assumption that memcmp arguments overlap.
ClosedPublic

Authored by NoQ on Dec 10 2019, 6:34 PM.

Details

Summary

While analyzing code

memcmp(a, NULL, n);

where a has an unconstrained symbolic value, the analyzer is currently emitting a warning about the first argument being a null pointer, even though we'd rather have it warn about the second argument.

This happens because CStringChecker first checks that the two argument buffers are in fact the same buffer, in order to take the fast path. This boils down to assuming a == NULL to true. Then the subsequent check for null pointer argument "discovers" that a is null.

This could have been fixed by reordering the checks (first check null arguments, then check for overlap) but i went a bit further and entirely removed the state split for "same buffer vs. different buffers". This state split is not well-justified: we cannot deduce from the presence of memcpy in the code that the buffers may in fact overlap. Instead, i conservatively assume that they don't overlap, unless they're already known to certainly overlap on the current execution path. This loses a bit of coverage but the lost path is where they do actually overlap. This path is in my opinion not only rare but also fairly useless, as it immediately introduces an aliasing problem that we aren't quite ready to deal with.

Diff Detail

Event Timeline

NoQ created this revision.Dec 10 2019, 6:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 10 2019, 6:34 PM
Charusso accepted this revision.Dec 10 2019, 6:45 PM

Could you write a test for strcmp please? I have found out the evalMemcmp behaves differently than evalStrcmp. However when the memcmp is going to be used like strcmp/strncmp the ideal would be to delegate the work to evalStrcmp. At the moment there is no connection between the two evaluation at all, which surprised me at first.

Other than that I really like the idea to remove overhead. Thanks!

This revision is now accepted and ready to land.Dec 10 2019, 6:45 PM
NoQ updated this revision to Diff 233249.Dec 10 2019, 6:51 PM

Fair point. Yeah, i wonder what the real difference is :/ Added tests.

NoQ added a comment.Dec 10 2019, 6:53 PM

Instead, i conservatively assume that they don't overlap, unless they're already known to certainly overlap on the current execution path. This loses a bit of coverage but the lost path is where they do actually overlap. This path is in my opinion not only rare but also fairly useless, as it immediately introduces an aliasing problem that we aren't quite ready to deal with.

Wait, no, that's not what i'm doing. I'm simply forgetting about the assumption that the buffers don't overlap. So never mind, we're not losing any coverage and we're not stepping into an aliasing problem.

xazax.hun accepted this revision.Dec 11 2019, 8:28 AM

Less state split, yay!

This revision was automatically updated to reflect the committed changes.