Page MenuHomePhabricator

[analyzer][MallocChecker] Make NewDeleteLeaks depend on DynamicMemoryModeling rather than NewDelete

Authored by Szelethus on Apr 4 2020, 1:06 PM.



If you remember the mail [1] I sent out about how I envision the future of the already existing checkers to look dependencywise, one my main points was that no checker that emits diagnostics should be a dependency. This is more problematic for some checkers (ahem, RetainCount [2]) more than for others, like this one.

The MallocChecker family is a mostly big monolithic modeling class some small reporting checkers that only come to action when we are constructing a warning message, after the actual bug was detected. The implication of this is that NewDeleteChecker doesn't really do anything to depend on, so this change was relatively simple.

The only thing that complicates this change is that FreeMemAux (MallocCheckers method that models general memory deallocation) returns after calling a bug reporting method, regardless whether the report was ever emitted (which may not always happen, for instance, if the checker responsible for the report isn't enabled). This return unfortunately happens before cleaning up the maps in the GDM keeping track of the state of symbols (whether they are released, whether that release was successful, etc). What this means is that upon disabling some checkers, we would never clean up the map and that could've lead to false positives, e.g.:

error: 'warning' diagnostics seen but not expected: 
  File clang/test/Analysis/ Line 66: Potential leak of memory pointed to by 'p'
  File clang/test/Analysis/ Line 73: Potential leak of memory pointed to by 'p'
  File clang/test/Analysis/ Line 77: Potential leak of memory pointed to by 'p'

error: 'warning' diagnostics seen but not expected: 
  File clang/test/Analysis/NewDelete-checker-test.cpp Line 111: Undefined or garbage value returned to caller
  File clang/test/Analysis/NewDelete-checker-test.cpp Line 200: Potential leak of memory pointed to by 'p'

error: 'warning' diagnostics seen but not expected: 
  File clang/test/Analysis/new.cpp Line 137: Potential leak of memory pointed to by 'x'

There two possible approaches I had in mind:

  • Make bug reporting methods of MallocChecker returns whether they succeeded, and proceed with the rest of FreeMemAux if not,
  • Halt execution with a sink node upon failure. I decided to go with this, as described in the code.

As you can see from the removed/changed test files, before the big checker dependency effort landed, there were tests to check for all the weird configurations of enabled/disabled checkers and their messy interactions, I largely repurposed these.


Diff Detail

Event Timeline

Szelethus created this revision.Apr 4 2020, 1:06 PM
Szelethus edited the summary of this revision. (Show Details)Apr 4 2020, 1:08 PM

Btw this branch is about 7 months old, and I just now remembered why I haven't rushed publishing it.

My ultimate idea (as opposed to the two detailed in the summary) was to introduce a subchecker system in MallocChecker, which also strongly ties into D67336. The core of the checker (unix.DynamicMemoryModeling) would be responsible for dispatching from regular checker callbacks upon seeing a call to a memory management function, and providing the general utilities for modeling it. Smaller subcheckers would then use this interface to implement the modeling of specific functions. For instance, the we could create NewDeleteChecker, a subchecker of DynamicMemoryModeling, which would get control when stumbling upon a call to a new/delete operator.

This would allow us to spread the implementation into smaller files, which is great, but upon taking a look at D68165, you can see that the bulk of the code isn't really in these modeling functions. For such a change to really make sense, the responsibility of bug emission (like ReportFunctionPointerFree) should be moved into the subcheckers as well. This would achieve my goal of smaller, self-contained subcheckers implementing something that we previously did with giants like MallocChecker is now. As to how I should actually pull this off (make an event about memory being deallocated to the subcheckers, make them check for bugs? make an event about finding a leak, double delete, etc?) is a different issue, but I'll see what makes more sense, if I choose to go forth with this project.

I do now believe that though that all of these changes would be orthogonal to this one. Just thought I'd share :^).

balazske added inline comments.Apr 16 2020, 6:26 AM
195 ↗(On Diff #255070)
if (!State)
  State = getState();

is better? (I put the same comment into some (other?) patch already but maybe it disappeared?)

Gentle ping :^)

martong added inline comments.May 21 2020, 4:40 AM
194 ↗(On Diff #255070)

I like the joke, but this comment does not have any value, could you please remove?

195 ↗(On Diff #255070)

+1 for balazske's suggestion.


This sentence is too bloated, I'd rather remove it.

AFAIU, the general rule of thumb is that if we have found a bug then we terminate further analysis, period. This is independent of whether we emit the warning or not, that actually depends on whether the corresponding subchecker is enabled or not.


This seems to be inverse logic to me.
I'd expect that in a function called Report... we do stuff that is related to reporting only. That is why I think it would be better to have the condition and addSink before calling Report.... That way reporting and modeling would be even more separated.

Szelethus updated this revision to Diff 265709.May 22 2020, 5:33 AM
Szelethus marked 7 inline comments as done.

Fix according to reviewer comments! Thanks!

195 ↗(On Diff #255070)

Landed this part of the change in D77866.


Very good point, but I think the actual problem lies in the name of the method. ReportBadFree in particular is called from multiple places, and I like how this is the function that takes over once we find a bug, because a bad free (which in this context means the deallocation of a non-heap allocated object) can never be a non-fatal error, so it make sense that the sink is solved here.

martong accepted this revision.May 25 2020, 6:14 AM

Looks good.


Okay, makes sense, the new name fits this.

This revision is now accepted and ready to land.May 25 2020, 6:14 AM
This revision was automatically updated to reflect the committed changes.