[PATCH] D105819: [analyzer] MallocChecker: Add a visitor to leave a note on functions that could have, but did not change ownership on leaked memory

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

This is amazing, great progress!

I added the parent revision because it didn't compile on pre-merge checks otherwise.



================
Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:801
+    // must be a superset of, but not necessarily equal to ExitOwners.
+    return !llvm::set_is_subset(ExitOwners, CurrOwners);
+  }
----------------
Can you also comment on what's, generally, the default scenario / motivating example where this is necessary? What makes you hunt down store bindings that didn't cause an escape to happen (given that an escape would have been a state change)? IIUC this is for the situation when the callee stores the pointer in a caller-local variable and in this case you don't want to claim that ownership didn't change?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:809-810
+    return std::make_shared<PathDiagnosticEventPiece>(
+        L, "Returning without changing the ownership status of allocated "
+           "memory");
+  }
----------------
I suggest: `"Returning without deallocating memory or storing the pointer for later deallocation"`. Still a bit flimsy but less jargony.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:832-833
+      ArrayRef<ParmVarDecl *> Parameters, unsigned ParamIdx) override {
+    // TODO: Check whether the allocated memory was actually passed into the
+    // function.
+    return emitNote(N);
----------------
How difficult would it be to re-use this part as well?


================
Comment at: clang/test/Analysis/NewDeleteLeaks.cpp:104-106
+// TODO: We don't want a note here. sink() doesn't seem like a function that
+// even attempts to take care of any memory ownership problems.
+namespace memory_passed_into_fn_that_doesnt_intend_to_free {
----------------
How do you think this is different from the very first test, `memory_allocated_in_fn_call()`? Is this just because of how the pointer is put into a variable first? This function is kinda still the only place that could free memory.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105819/new/

https://reviews.llvm.org/D105819