Szelethus updated this revision to Diff 358883.
Szelethus added a comment.
- Fixes according to reviewer comments!
- Rebase on D105553 <https://reviews.llvm.org/D105553>.
- Primitively check whether the allocated memory was actually passed into the
function.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D105819/new/
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,130 @@
+// 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 {
+
+void sink(int *P) {
+ if (coin())
+ delete P;
+}
+
+void foo() {
+ int *ptr = new int(5); // expected-note {{Memory is allocated}}
+ int *q = nullptr;
+ sink(q);
+ (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,146 @@
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 deallocating memory or storing the pointer for "
+ "later deallocation");
+ }
+
+ 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
+ maybeEmitNoteForParameters(PathSensitiveBugReport &R, const CallEvent &Call,
+ const ExplodedNode *N) override {
+ // TODO: Factor the logic of "what constitutes as an entity being passed
+ // into a function call" out by reusing the code in
+ // NoStoreFuncVisitor::maybeEmitNoteForParameters, maybe by incorporating
+ // the printing technology in UninitializedObject's FieldChainInfo.
+ ArrayRef<ParmVarDecl *> Parameters = Call.parameters();
+ for (unsigned I = 0; I < Call.getNumArgs() && I < Parameters.size(); ++I) {
+ SVal V = Call.getArgSVal(I);
+ if (V.getAsSymbol() == Sym)
+ return emitNote(N);
+ }
+ return nullptr;
+ }
+
+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 +990,6 @@
}
};
};
-
} // end anonymous namespace
// A map from the freed symbol to the symbol representing the return value of
@@ -2579,6 +2717,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
