[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
cfe-commits at lists.llvm.org
Mon Jul 12 20:08:20 PDT 2021
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
More information about the cfe-commits
mailing list