This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] Implementation of potential undefbehavior.ZeroAllocDereference checker.
ClosedPublic

Authored by ayartsev on Mar 11 2015, 3:21 PM.

Details

Summary

Proposed patch implements undefbehavior.ZeroAllocDereference checker as part of unix.Malloc and cplusplus.NewDelete checkers.
The patch doesn't handle zero-size realloc()s due to specifics of current handling of realloc() by unix.Malloc that need to be discussed.
Please review!

Diff Detail

Event Timeline

ayartsev updated this revision to Diff 21772.Mar 11 2015, 3:21 PM
ayartsev retitled this revision from to [analyzer] Implementation of potential undefbehavior.ZeroAllocDereference checker..
ayartsev updated this object.
ayartsev edited the test plan for this revision. (Show Details)
ayartsev added a reviewer: zaks.anna.
ayartsev added subscribers: jordan_rose, krememek, Unknown Object (MLST).
zaks.anna edited edge metadata.Mar 12 2015, 5:01 PM

**As a rule of thumb, checkers should be stateless.

http://clang-analyzer.llvm.org/checker_dev_manual.html

When you introduce mutable members you are most likely making a mistake. The state should track properties of symbols; specifically to check with symbol corresponds to a '0' allocation.

The specific example that might break with your patch (depending on the order in which the states are being explored) is something along these lines:
if (b)

s= 10;

else

s = 0;

p = malloc(s);
if (b)
*p = 1;

When the checker explores "malloc(s)" along the "s=0" path, the expression will be added to the set. If "*p = 1" along the "s=10" path is explored later on, we are going to produce a false positive.

Please, provide better testing so the cases like this one are exposed.

Aaa, I see, thank you!

ayartsev updated this revision to Diff 22226.Mar 18 2015, 4:20 PM
ayartsev edited edge metadata.

Updated the patch, made the checker stateless, please review!

zaks.anna added inline comments.Mar 20 2015, 6:38 PM
lib/StaticAnalyzer/Checkers/MallocChecker.cpp
67–68

I think you could just fold it into the Kind, by adding AllocatedOfSizeZero or do we think that Relinquished or Escaped should be treated differently if they were zero allocated..?

850

"ProcessZeroAllocation" ? We are not checking anything here.

895

It should not be possible to have non allocated symbol here.. Is it? Maybe we should assert?

1860

I's call this "Use of zero allocated" or "Zero allocation"

2311

this seems unrelated to the patch. Can it be submitted separately with a testcase that it is trying to address?

Thanks for review!

lib/StaticAnalyzer/Checkers/MallocChecker.cpp
67–68

Implemented with a new AllocatedOfSizeZero kind. Theoretically Relinquished may be treated differently if we would like to track usage of zero-allocated memory after relinquish but there are no cases yet.

850

Fixed!

895

Agree, done!

1860

Done!

2311

Cleaned.

ayartsev updated this revision to Diff 22411.Mar 21 2015, 6:35 AM

New patch with comments addressed, please review!

.

lib/StaticAnalyzer/Checkers/MallocChecker.cpp
895

Pardon, currently zero-allocated realloc do not attach a RefState so it is still early to assert for now.

Hi Anton,

Have you tested the patch on anything but the regression tests? If yes,
what are the results? Did this catch any issues? Are there any false
positives? Since this will be a turned on by default new warning, I'd like
make sure we test on real code before committing.

Other than testing, this looks good to me. Thank you!
Anna.

Yes, please, commit!

Thank you,
Anna.

Committed as r234889.

ayartsev accepted this revision.Aug 25 2015, 2:30 PM
ayartsev added a reviewer: ayartsev.
This revision is now accepted and ready to land.Aug 25 2015, 2:30 PM
ayartsev closed this revision.Aug 25 2015, 2:30 PM