Page MenuHomePhabricator

[analyzer] MallocChecker: Add notes from NoOwnershipChangeVisitor only when a function "intents", but doesn't change ownership, enable by default
ClosedPublic

Authored by Szelethus on Aug 26 2021, 2:47 AM.

Details

Summary

D105819 Added NoOwnershipChangeVisitor, but it is only registered when an off-by-default, hidden checker option was enabled. The reason behind this was that it grossly overestimated the set of functions that really needed a note:

std::string getTrainName(const Train *T) {
  return T->name;
} // note: Retuning without changing the ownership of or deallocating memory
// Umm... I mean duh? Nor would I expect this function to do anything like that...

void foo() {
  Train *T = new Train("Land Plane");
  print(getTrainName(T)); // note: calling getTrainName / returning from getTrainName
} // warn: Memory leak

This patch adds a heuristic that guesses that any function that has an explicit operator delete call could have be responsible for deallocating the memory that ended up leaking. This is waaaay too conservative (see the TODOs in the new function), but it safer to err on the side of too little than too much, and would allow us to enable the option by default *now*, and add refinements one-by-one.

Diff Detail

Event Timeline

Szelethus created this revision.Aug 26 2021, 2:47 AM
Szelethus requested review of this revision.Aug 26 2021, 2:47 AM
martong added inline comments.Aug 27 2021, 7:10 AM
clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
802

Sounds like a good idea to reuse isFreeingCall.

But interestingly delete is not listed there! Nor new in AllocatingMemFnMap. So, perhaps we need yet another abstraction. Maybe a function that iterates over FreeingMemFnMap collects the functions and then adds delete as well?

806

Good point!

clang/test/Analysis/NewDeleteLeaks.cpp
23

intent

BTW, why is this TODO here? It is obvious that sink does not have a delete, so we don't expect any warning. Or am I missing something?

Szelethus added inline comments.Sep 2 2021, 6:47 AM
clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
282–289

These serve to identify new/delete operators. I've done a lot of work to make this better, but I don't remember why isFreeingCall doesn't call these.

clang/test/Analysis/NewDeleteLeaks.cpp
23

Well, its debatable, I suppose, but in this case, sink is noteworthy, as no other function had the ability to take care of this memory.

https://lists.llvm.org/pipermail/cfe-dev/2021-June/068469.html

Kristof's case is interesting in a different manner. If taken literally, it doesn't satisfy our criteria of "there exists another execution path on which it did actually do ${Something}". We probably shouldn't emit a note every time the function simply accepts the value of interest by pointer, because this doesn't necessarily prove the intent to delete the pointer; there are a million other reasons to accept a pointer. However, Kristof's case does still deserve a note because sink() is the *only* function that had a chance to delete the pointer.

Szelethus updated this revision to Diff 370553.Sep 3 2021, 5:22 AM

indent->intent

martong accepted this revision.Sep 3 2021, 7:38 AM

Okay, LGTM! Thanks!

This revision is now accepted and ready to land.Sep 3 2021, 7:38 AM
NoQ accepted this revision.Sep 7 2021, 9:50 PM

This looks good!

I guess one way to make this even more conservative would be to match the variable inside the delete-expression to the one we expect to get deallocated.

This looks good!

I guess one way to make this even more conservative would be to match the variable inside the delete-expression to the one we expect to get deallocated.

Yeah, I thought about that, but one counter argument to that is that once an intent to do some deallocation is already there, its possible that the wrong variable was deleted, or the deallocation wasn't thorough enough.

Thanks for the reviews!

This revision was landed with ongoing or failed builds.Sep 13 2021, 6:02 AM
This revision was automatically updated to reflect the committed changes.