Page MenuHomePhabricator

[Analyzer] Improve invalid dereference bug reporting in DereferenceChecker.
ClosedPublic

Authored by balazske on Jul 24 2020, 5:15 AM.

Details

Summary

Report undefined pointer dereference in similar way as null pointer dereference.

Diff Detail

Event Timeline

balazske created this revision.Jul 24 2020, 5:15 AM
balazske marked an inline comment as done.Jul 24 2020, 5:23 AM

Not sure if this is a good solution. There are few tests that test the undefined reference case. But the added code should work the same way as in the null pointer case?

clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp
135

Add "results in a " here?

balazske added a comment.EditedJul 27 2020, 12:46 AM

Change of the existing error messages and bug category is not necessary but it would be improvement because more uniform error messages. (And not the "logic error" is the best category for this case, if this value is used at all. Other checkers have bad values too.)

I would add one more test for the undefined case. Like a local array variable that is uninitialized. That could mirror some of the null-dereference cases.

clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp
135

IMHO I would add the "results in a " part here, as this is part of the process of creating the diagnostic anyway.

We definitely need more tests. The idea overall however is amazing, thanks!

clang/test/Analysis/misc-ps-region-store.m
1160

Is this the entire message? Because traditionally, analyzer warnings start with an upper case and don't terminate.

balazske updated this revision to Diff 283856.Aug 7 2020, 2:58 AM

NFC changes, added some tests.

NoQ added a comment.Aug 7 2020, 11:19 AM

Thanks, excellent!

Let's not change warning text without good reasons. Too few of us speak English well enough to check the articles. The original text seems concise and on-point.

balazske updated this revision to Diff 284283.Aug 10 2020, 2:20 AM

Preserve original error messages.

balazske marked an inline comment as done.Aug 10 2020, 2:25 AM

Do the null pointer and invalid pointer dereference belong to the same checker, that is called NullDereference?

clang/test/Analysis/misc-ps-region-store.m
1160

Full message is:
Access to field 'tail' results in a dereference of an undefined pointer value (loaded from variable 'items') [core.NullDereference]

Do the null pointer and invalid pointer dereference belong to the same checker, that is called NullDereference?

I think both SA and Tidy auto-appends the checker's name into the emitted diagnostic. However, @baloghadamsoftware did implement multiple checkers in the same translation unit, so maybe he knows where to override this.

(I can tell you where to override it in the context of Tidy, and it's horribly tricky and ugly...)

NoQ added a comment.Aug 10 2020, 8:28 PM

Do the null pointer and invalid pointer dereference belong to the same checker, that is called NullDereference?

Yup. And that's bad.

Note that the only reason to have checker names is to allow users to enable/disable the checkers. Given that enabling/disabling core checkers was never supported to begin with, this wasn't much of an issue. Now that we're moving into the direction of allowing users to silence core checkers without disabling their modeling benefits, this becomes much more of a problem and there's a number of checker name inconsistencies that we'll have to revisit. Another famous inconsistency is having a popular case of null dereference, "calling a C++ method on a null pointer", is in fact checked by the core.CallAndMessage checker rather than by core.NullDereference.

NoQ accepted this revision.Aug 10 2020, 10:08 PM

Also thanks, let's land!

This revision is now accepted and ready to land.Aug 10 2020, 10:08 PM
balazske updated this revision to Diff 284588.Aug 10 2020, 11:55 PM
balazske marked an inline comment as done.

Rebase (bug category is LogicError).