This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] Improve NoOwnershipChangeVisitor's understanding of deallocators
ClosedPublic

Authored by Szelethus on Feb 3 2022, 1:34 AM.

Details

Summary

The problem with leak bug reports is that the most interesting event in the code is likely the one that did not happen -- lack of ownership change and lack of deallocation, which is often present within the same function that the analyzer inlined anyway, but not on the path of execution on which the bug occured. We struggle to understand that a function was responsible for freeing the memory, but failed.

D105819 added a new visitor to improve memory leak bug reports. In addition to inspecting the ExplodedNodes of the bug pat, the visitor tries to guess whether the function was supposed to free memory, but failed to. Initially (in D108753), this was done by checking whether a CXXDeleteExpr is present in the function. If so, we assume that the function was at least party responsible, and prevent the analyzer from pruning bug report notes in it. This patch improves this heuristic by recognizing all deallocator functions that MallocChecker itself recognizes, by reusing MallocChecker::isFreeingCall.

Diff Detail

Event Timeline

Szelethus created this revision.Feb 3 2022, 1:34 AM
Szelethus requested review of this revision.Feb 3 2022, 1:34 AM
Szelethus updated this revision to Diff 405979.Feb 4 2022, 9:08 AM
Szelethus edited the summary of this revision. (Show Details)

Move CallDescription specific changes to D119004.

I like it.
nits here and there;

clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h
102

CallExpr

clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
750

What about having a const reference, or a const pointer to const?

802

You should probably add an example of when is it imprecise and an explanation of why we have this function.

807
818–820

I guess, this gets fixed by this patch.

820–832

Without braces, it would be slightly more compact & readable IMO.

832

Could you clarify what this wants to denote? I'm just curious.

1104
clang/test/Analysis/NewDeleteLeaks.cpp
152

I guess we would not have the warning if bar would accept the pointer P, since that way it would escape.
Am I correct?

Szelethus updated this revision to Diff 407121.Feb 9 2022, 4:30 AM
Szelethus marked 8 inline comments as done.

Fixes according to reviewer comments.

clang/test/Analysis/NewDeleteLeaks.cpp
152

That is correct, but as an earlier todo states, we don't currently employ any heuristics to guess whether bar was responsible for storing P (as opposed to deallocating it, which is what this patch is about), but failed to.

steakhal accepted this revision.Feb 9 2022, 5:09 AM

I'm content. Thanks for the patch.

This revision is now accepted and ready to land.Feb 9 2022, 5:09 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 3 2022, 2:28 AM