Szelethus created this revision.
Szelethus added reviewers: NoQ, vsavchenko, steakhal, martong, ASDenysPetrov.
Szelethus added a project: clang.
Herald added subscribers: manas, gamesh411, dkrupp, donat.nagy,
mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware, xazax.hun,
whisperity, yaxunl.
Szelethus requested review of this revision.
Herald added subscribers: cfe-commits, aheejin.
This is a rather common feedback we get from out leak checkers: bug reports are
really short, and are contain barely any usable information on what the
analyzer did to conclude that a leak actually happened.
This happens because of our bug report minimizing effort. We construct bug
reports by inspecting the `ExplodedNode`s that lead to the error from the
bottom up (from the error node all the way to the root of the exploded graph),
and mark entities that were the cause of a bug, or have interacted with it as
interesting. In order to make the bug report a bit less verbose, whenever we
find an entire function call (from `CallEnter` to `CallExitEnd`) that didn't
talk about any interesting entity, we prune it (click here
<https://www.youtube.com/watch?v=yh2qdnJjizE&ab_channel=LLVM> for more info on
bug report generation). Even if the event to highlight is exactly this lack of
interaction with interesting entities.
D105553 <https://reviews.llvm.org/D105553> generalized the visitor that creates
notes for these cases. This patch adds a new kind of `NoStateChangeVisitor`
that leaves notes in functions that took a piece of dynamically allocated
memory that later leaked as parameter, and didn't change its ownership status.
While there is some code to talk over in MallocChecker.cpp, the main thing to
discuss in my mind are the test cases, where I display where I want to see this
visitor end up. I hope to be able to reach a point sometime soon when I can run
on this on some real projects and post screenshots about it!
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D105819
Files:
clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
clang/test/Analysis/NewDeleteLeaks.cpp
clang/test/Analysis/self-assign.cpp
Index: clang/test/Analysis/self-assign.cpp
===================================================================
--- clang/test/Analysis/self-assign.cpp
+++ clang/test/Analysis/self-assign.cpp
@@ -1,4 +1,9 @@
-// RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core,cplusplus,unix.Malloc,debug.ExprInspection -analyzer-config eagerly-assume=false %s -verify -analyzer-output=text
+// RUN: %clang_analyze_cc1 -std=c++11 %s -verify -analyzer-output=text \
+// RUN: -analyzer-checker=core \
+// RUN: -analyzer-checker=cplusplus \
+// RUN: -analyzer-checker=unix.Malloc \
+// RUN: -analyzer-checker=debug.ExprInspection \
+// RUN: -analyzer-config eagerly-assume=false
extern "C" char *strdup(const char* s);
extern "C" void free(void* ptr);
@@ -28,18 +33,31 @@
free(str);
}
-StringUsed& StringUsed::operator=(const StringUsed &rhs) { // expected-note{{Assuming rhs == *this}} expected-note{{Assuming rhs == *this}} expected-note{{Assuming rhs != *this}}
- clang_analyzer_eval(*this == rhs); // expected-warning{{TRUE}} expected-warning{{UNKNOWN}} expected-note{{TRUE}} expected-note{{UNKNOWN}}
+StringUsed &StringUsed::operator=(const StringUsed &rhs) {
+ // expected-note@-1{{Assuming rhs == *this}}
+ // expected-note@-2{{Assuming rhs == *this}}
+ // expected-note@-3{{Assuming rhs != *this}}
+ clang_analyzer_eval(*this == rhs); // expected-warning{{TRUE}}
+ // expected-warning@-1{{UNKNOWN}}
+ // expected-note@-2{{TRUE}}
+ // expected-note@-3{{UNKNOWN}}
free(str); // expected-note{{Memory is released}}
- str = strdup(rhs.str); // expected-warning{{Use of memory after it is freed}} expected-note{{Use of memory after it is freed}}
-// expected-note@-1{{Memory is allocated}}
+ str = strdup(rhs.str); // expected-warning{{Use of memory after it is freed}}
+ // expected-note@-1{{Use of memory after it is freed}}
+ // expected-note@-2{{Memory is allocated}}
return *this;
}
-StringUsed& StringUsed::operator=(StringUsed &&rhs) { // expected-note{{Assuming rhs == *this}} expected-note{{Assuming rhs != *this}}
- clang_analyzer_eval(*this == rhs); // expected-warning{{TRUE}} expected-warning{{UNKNOWN}} expected-note{{TRUE}} expected-note{{UNKNOWN}}
+StringUsed &StringUsed::operator=(StringUsed &&rhs) {
+ // expected-note@-1{{Assuming rhs == *this}}
+ // expected-note@-2{{Assuming rhs != *this}}
+ clang_analyzer_eval(*this == rhs); // expected-warning{{TRUE}}
+ // expected-warning@-1{{UNKNOWN}}
+ // expected-note@-2{{TRUE}}
+ // expected-note@-3{{UNKNOWN}}
str = rhs.str;
- rhs.str = nullptr; // expected-warning{{Potential memory leak}} expected-note{{Potential memory leak}}
+ rhs.str = nullptr; // expected-warning{{Potential memory leak}}
+ // expected-note@-1{{Potential memory leak}}
return *this;
}
@@ -63,15 +81,27 @@
free(str);
}
-StringUnused& StringUnused::operator=(const StringUnused &rhs) { // expected-note{{Assuming rhs == *this}} expected-note{{Assuming rhs == *this}} expected-note{{Assuming rhs != *this}}
- clang_analyzer_eval(*this == rhs); // expected-warning{{TRUE}} expected-warning{{UNKNOWN}} expected-note{{TRUE}} expected-note{{UNKNOWN}}
+StringUnused &StringUnused::operator=(const StringUnused &rhs) {
+ // expected-note@-1{{Assuming rhs == *this}}
+ // expected-note@-2{{Assuming rhs == *this}}
+ // expected-note@-3{{Assuming rhs != *this}}
+ clang_analyzer_eval(*this == rhs); // expected-warning{{TRUE}}
+ // expected-warning@-1{{UNKNOWN}}
+ // expected-note@-2{{TRUE}}
+ // expected-note@-3{{UNKNOWN}}
free(str); // expected-note{{Memory is released}}
- str = strdup(rhs.str); // expected-warning{{Use of memory after it is freed}} expected-note{{Use of memory after it is freed}}
+ str = strdup(rhs.str); // expected-warning{{Use of memory after it is freed}}
+ // expected-note@-1{{Use of memory after it is freed}}
return *this;
}
-StringUnused& StringUnused::operator=(StringUnused &&rhs) { // expected-note{{Assuming rhs == *this}} expected-note{{Assuming rhs != *this}}
- clang_analyzer_eval(*this == rhs); // expected-warning{{TRUE}} expected-warning{{UNKNOWN}} expected-note{{TRUE}} expected-note{{UNKNOWN}}
+StringUnused &StringUnused::operator=(StringUnused &&rhs) {
+ // expected-note@-1{{Assuming rhs == *this}}
+ // expected-note@-2{{Assuming rhs != *this}}
+ clang_analyzer_eval(*this == rhs); // expected-warning{{TRUE}}
+ // expected-warning@-1{{UNKNOWN}}
+ // expected-note@-2{{TRUE}}
+ // expected-note@-3{{UNKNOWN}}
str = rhs.str;
rhs.str = nullptr; // FIXME: An improved leak checker should warn here
return *this;
@@ -84,7 +114,8 @@
int main() {
StringUsed s1 ("test"), s2;
- s2 = s1; // expected-note{{Calling copy assignment operator for 'StringUsed'}} // expected-note{{Returned allocated memory}}
+ s2 = s1; // expected-note{{Calling copy assignment operator for 'StringUsed'}}
+ // expected-note@-1{{Returned allocated memory}}
s2 = std::move(s1); // expected-note{{Calling move assignment operator for 'StringUsed'}}
return 0;
}
Index: clang/test/Analysis/NewDeleteLeaks.cpp
===================================================================
--- /dev/null
+++ clang/test/Analysis/NewDeleteLeaks.cpp
@@ -0,0 +1,134 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus,unix -verify -analyzer-output=text %s
+
+#include "Inputs/system-header-simulator-for-malloc.h"
+
+//===----------------------------------------------------------------------===//
+// Report for which we expect NoOwnershipChangeVisitor to add a new note.
+//===----------------------------------------------------------------------===//
+
+bool coin();
+
+namespace memory_allocated_in_fn_call {
+
+void sink(int *P) {
+} // expected-note {{Returning without changing the ownership status of allocated memory}}
+
+void foo() {
+ sink(new int(5)); // expected-note {{Memory is allocated}}
+ // expected-note@-1 {{Calling 'sink'}}
+ // expected-note@-2 {{Returning from 'sink'}}
+} // expected-warning {{Potential memory leak [cplusplus.NewDeleteLeaks]}}
+// expected-note@-1 {{Potential memory leak}}
+
+} // namespace memory_allocated_in_fn_call
+
+namespace memory_passed_to_fn_call {
+
+void sink(int *P) {
+ if (coin()) // expected-note {{Assuming the condition is false}}
+ // expected-note@-1 {{Taking false branch}}
+ delete P;
+} // expected-note {{Returning without changing the ownership status of allocated memory}}
+
+void foo() {
+ int *ptr = new int(5); // expected-note {{Memory is allocated}}
+ sink(ptr); // expected-note {{Calling 'sink'}}
+ // expected-note@-1 {{Returning from 'sink'}}
+} // expected-warning {{Potential leak of memory pointed to by 'ptr' [cplusplus.NewDeleteLeaks]}}
+// expected-note@-1 {{Potential leak}}
+
+} // namespace memory_passed_to_fn_call
+
+namespace memory_shared_with_ptr_of_shorter_lifetime {
+
+void sink(int *P) {
+ int *Q = P;
+ if (coin()) // expected-note {{Assuming the condition is false}}
+ // expected-note@-1 {{Taking false branch}}
+ delete P;
+ (void)Q;
+} // expected-note {{Returning without changing the ownership status of allocated memory}}
+
+void foo() {
+ int *ptr = new int(5); // expected-note {{Memory is allocated}}
+ sink(ptr); // expected-note {{Calling 'sink'}}
+ // expected-note@-1 {{Returning from 'sink'}}
+} // expected-warning {{Potential leak of memory pointed to by 'ptr' [cplusplus.NewDeleteLeaks]}}
+// expected-note@-1 {{Potential leak}}
+
+} // namespace memory_shared_with_ptr_of_shorter_lifetime
+
+//===----------------------------------------------------------------------===//
+// Report for which we *do not* expect NoOwnershipChangeVisitor add a new note,
+// nor do we want it to.
+//===----------------------------------------------------------------------===//
+
+namespace memory_not_passed_to_fn_call {
+
+// TODO: We don't want a note here. We need to check whether the allocated
+// memory was actually passed into the function.
+void sink(int *P) {
+ if (coin()) // expected-note {{Assuming the condition is false}}
+ // expected-note@-1 {{Taking false branch}}
+ delete P;
+} // expected-note {{Returning without changing the ownership status of allocated memory}}
+
+void foo() {
+ int *ptr = new int(5); // expected-note {{Memory is allocated}}
+ int *q = nullptr;
+ sink(q); // expected-note {{Calling 'sink'}}
+ // expected-note@-1 {{Returning from 'sink'}}
+ (void)ptr;
+} // expected-warning {{Potential leak of memory pointed to by 'ptr' [cplusplus.NewDeleteLeaks]}}
+// expected-note@-1 {{Potential leak}}
+
+} // namespace memory_not_passed_to_fn_call
+
+namespace memory_shared_with_ptr_of_same_lifetime {
+
+void sink(int *P, int **Q) {
+ // NOTE: Not a job of NoOwnershipChangeVisitor, but maybe this could be
+ // highlighted still?
+ *Q = P;
+}
+
+void foo() {
+ int *ptr = new int(5); // expected-note {{Memory is allocated}}
+ int *q = nullptr;
+ sink(ptr, &q);
+} // expected-warning {{Potential leak of memory pointed to by 'q' [cplusplus.NewDeleteLeaks]}}
+// expected-note@-1 {{Potential leak}}
+
+} // namespace memory_shared_with_ptr_of_same_lifetime
+
+// 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 {
+
+void sink(int *P) {
+} // expected-note {{Returning without changing the ownership status of allocated memory}}
+
+void foo() {
+ int *ptr = new int(5); // expected-note {{Memory is allocated}}
+ sink(ptr); // expected-note {{Calling 'sink'}}
+ // expected-note@-1 {{Returning from 'sink'}}
+} // expected-warning {{Potential leak of memory pointed to by 'ptr' [cplusplus.NewDeleteLeaks]}}
+// expected-note@-1 {{Potential leak}}
+
+} // namespace memory_passed_into_fn_that_doesnt_intend_to_free
+
+namespace refkind_from_unoallocated_to_allocated {
+
+// RefKind of the symbol changed from nothing to Allocated. We don't want to
+// emit notes when the RefKind changes in the stack frame.
+static char *malloc_wrapper_ret() {
+ return (char *)malloc(12); // expected-note {{Memory is allocated}}
+}
+void use_ret() {
+ char *v;
+ v = malloc_wrapper_ret(); // expected-note {{Calling 'malloc_wrapper_ret'}}
+ // expected-note@-1 {{Returned allocated memory}}
+} // expected-warning {{Potential leak of memory pointed to by 'v' [unix.Malloc]}}
+// expected-note@-1 {{Potential leak of memory pointed to by 'v'}}
+
+} // namespace refkind_from_unoallocated_to_allocated
Index: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -48,6 +48,7 @@
#include "InterCheckerAPI.h"
#include "clang/AST/Attr.h"
#include "clang/AST/DeclCXX.h"
+#include "clang/AST/DeclTemplate.h"
#include "clang/AST/Expr.h"
#include "clang/AST/ExprCXX.h"
#include "clang/AST/ParentMap.h"
@@ -64,12 +65,15 @@
#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/DynamicExtent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState_Fwd.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/StoreRef.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h"
#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SetOperations.h"
#include "llvm/ADT/SmallString.h"
#include "llvm/ADT/StringExtras.h"
#include "llvm/Support/Compiler.h"
@@ -722,11 +726,138 @@
bool isArgZERO_SIZE_PTR(ProgramStateRef State, CheckerContext &C,
SVal ArgVal) const;
};
+} // end anonymous namespace
+
+//===----------------------------------------------------------------------===//
+// Definition of NoOwnershipChangeVisitor.
+//===----------------------------------------------------------------------===//
+
+namespace {
+class NoOwnershipChangeVisitor final : public NoStateChangeFuncVisitor {
+ SymbolRef Sym;
+ using OwnerSet = llvm::SmallPtrSet<const MemRegion *, 8>;
+
+ // Collect which entities point to the allocated memory, and could be
+ // responsible for deallocating it.
+ class OwnershipBindingsHandler : public StoreManager::BindingsHandler {
+ SymbolRef Sym;
+ OwnerSet &Owners;
+
+ public:
+ OwnershipBindingsHandler(SymbolRef Sym, OwnerSet &Owners)
+ : Sym(Sym), Owners(Owners) {}
+
+ bool HandleBinding(StoreManager &SMgr, Store Store, const MemRegion *Region,
+ SVal Val) override {
+ if (Val.getAsSymbol() == Sym)
+ Owners.insert(Region);
+ return true;
+ }
+ };
+
+protected:
+ OwnerSet getOwnersAtNode(const ExplodedNode *N) {
+ OwnerSet Ret;
+
+ ProgramStateRef State = N->getState();
+ OwnershipBindingsHandler Handler{Sym, Ret};
+ State->getStateManager().getStoreManager().iterBindings(State->getStore(),
+ Handler);
+ return Ret;
+ }
+
+ static const ExplodedNode *getCallExitEnd(const ExplodedNode *N) {
+ while (N && !N->getLocationAs<CallExitEnd>())
+ N = N->getFirstSucc();
+ return N;
+ }
+
+ virtual bool
+ wasModifiedBeforeCallExit(const ExplodedNode *CurrN,
+ const ExplodedNode *CallExitN) override {
+ if (CurrN->getLocationAs<CallEnter>())
+ return true;
+
+ // Its the state right *after* the call that is interesting. Any pointers
+ // inside the call that pointed to the allocated memory are of little
+ // consequence if their lifetime ends before within the function.
+ CallExitN = getCallExitEnd(CallExitN);
+ if (!CallExitN)
+ return true;
+
+ if (CurrN->getState()->get<RegionState>(Sym) !=
+ CallExitN->getState()->get<RegionState>(Sym))
+ return true;
+
+ OwnerSet CurrOwners = getOwnersAtNode(CurrN);
+ OwnerSet ExitOwners = getOwnersAtNode(CallExitN);
+
+ // Owners in the current set may be purged from the analyzer later on.
+ // If a variable is dead (is not referenced directly or indirectly after
+ // some point), it will be removed from the Store before the end of its
+ // actual lifetime.
+ // This means that that if the ownership status didn't change, CurrOwners
+ // must be a superset of, but not necessarily equal to ExitOwners.
+ return !llvm::set_is_subset(ExitOwners, CurrOwners);
+ }
+
+ static PathDiagnosticPieceRef emitNote(const ExplodedNode *N) {
+ PathDiagnosticLocation L = PathDiagnosticLocation::create(
+ N->getLocation(),
+ N->getState()->getStateManager().getContext().getSourceManager());
+ return std::make_shared<PathDiagnosticEventPiece>(
+ L, "Returning without changing the ownership status of allocated "
+ "memory");
+ }
+
+ virtual PathDiagnosticPieceRef
+ maybeEmitNoteForObjCSelf(PathSensitiveBugReport &R,
+ const ObjCMethodCall &Call,
+ const ExplodedNode *N) override {
+ // TODO: Implement.
+ return nullptr;
+ }
+
+ virtual PathDiagnosticPieceRef
+ maybeEmitNoteForCXXThis(PathSensitiveBugReport &R,
+ const CXXConstructorCall &Call,
+ const ExplodedNode *N) override {
+ // TODO: Implement.
+ return nullptr;
+ }
+
+ virtual PathDiagnosticPieceRef maybeEmitNoteForParameter(
+ PathSensitiveBugReport &R, const CallEvent &Call, const ExplodedNode *N,
+ ArrayRef<ParmVarDecl *> Parameters, unsigned ParamIdx) override {
+ // TODO: Check whether the allocated memory was actually passed into the
+ // function.
+ return emitNote(N);
+ }
+
+public:
+ NoOwnershipChangeVisitor(SymbolRef Sym)
+ : NoStateChangeFuncVisitor(bugreporter::TrackingKind::Thorough),
+ Sym(Sym) {}
+
+ void Profile(llvm::FoldingSetNodeID &ID) const override {
+ static int Tag = 0;
+ ID.AddPointer(&Tag);
+ ID.AddPointer(Sym);
+ }
+
+ void *getTag() const {
+ static int Tag = 0;
+ return static_cast<void *>(&Tag);
+ }
+};
+
+} // end anonymous namespace
//===----------------------------------------------------------------------===//
// Definition of MallocBugVisitor.
//===----------------------------------------------------------------------===//
+namespace {
/// The bug visitor which allows us to print extra diagnostics along the
/// BugReport path. For example, showing the allocation site of the leaked
/// region.
@@ -851,7 +982,6 @@
}
};
};
-
} // end anonymous namespace
// A map from the freed symbol to the symbol representing the return value of
@@ -2579,6 +2709,7 @@
AllocNode->getLocationContext()->getDecl());
R->markInteresting(Sym);
R->addVisitor<MallocBugVisitor>(Sym, true);
+ R->addVisitor<NoOwnershipChangeVisitor>(Sym);
C.emitReport(std::move(R));
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
