This revision was automatically updated to reflect the committed changes.
Closed by commit rGd832078904c6: [analyzer] Improve
NoOwnershipChangeVisitor's understanding of deallocators (authored by
Szelethus).
Herald added a project: All.
Changed prior to commit:
https://reviews.llvm.org/D118880?vs=407121&id=412644#toc
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D118880/new/
https://reviews.llvm.org/D118880
Files:
clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
clang/test/Analysis/NewDeleteLeaks.cpp
Index: clang/test/Analysis/NewDeleteLeaks.cpp
===================================================================
--- clang/test/Analysis/NewDeleteLeaks.cpp
+++ clang/test/Analysis/NewDeleteLeaks.cpp
@@ -35,7 +35,9 @@
} // namespace memory_allocated_in_fn_call
-namespace memory_passed_to_fn_call {
+// Realize that sink() intends to deallocate memory, assume that it should've
+// taken care of the leaked object as well.
+namespace memory_passed_to_fn_call_delete {
void sink(int *P) {
if (coin()) // ownership-note {{Assuming the condition is false}}
@@ -50,7 +52,41 @@
} // 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_passed_to_fn_call_delete
+
+namespace memory_passed_to_fn_call_free {
+
+void sink(int *P) {
+ if (coin()) // ownership-note {{Assuming the condition is false}}
+ // ownership-note@-1 {{Taking false branch}}
+ free(P);
+} // ownership-note {{Returning without deallocating memory or storing the pointer for later deallocation}}
+
+void foo() {
+ int *ptr = (int *)malloc(sizeof(int)); // expected-note {{Memory is allocated}}
+ sink(ptr); // ownership-note {{Calling 'sink'}}
+ // ownership-note@-1 {{Returning from 'sink'}}
+} // expected-warning {{Potential leak of memory pointed to by 'ptr' [unix.Malloc]}}
+// expected-note@-1 {{Potential leak}}
+
+} // namespace memory_passed_to_fn_call_free
+
+// Function pointers cannot be resolved syntactically.
+namespace memory_passed_to_fn_call_free_through_fn_ptr {
+void (*freeFn)(void *) = free;
+
+void sink(int *P) {
+ if (coin())
+ freeFn(P);
+}
+
+void foo() {
+ int *ptr = (int *)malloc(sizeof(int)); // expected-note {{Memory is allocated}}
+ sink(ptr);
+} // expected-warning {{Potential leak of memory pointed to by 'ptr' [unix.Malloc]}}
+// expected-note@-1 {{Potential leak}}
+
+} // namespace memory_passed_to_fn_call_free_through_fn_ptr
namespace memory_shared_with_ptr_of_shorter_lifetime {
@@ -123,6 +159,24 @@
} // namespace memory_passed_into_fn_that_doesnt_intend_to_free
+namespace memory_passed_into_fn_that_doesnt_intend_to_free2 {
+
+void bar();
+
+void sink(int *P) {
+ // Correctly realize that calling bar() doesn't mean that this function would
+ // like to deallocate anything.
+ bar();
+}
+
+void foo() {
+ int *ptr = new int(5); // expected-note {{Memory is allocated}}
+ sink(ptr);
+} // 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_free2
+
namespace refkind_from_unoallocated_to_allocated {
// RefKind of the symbol changed from nothing to Allocated. We don't want to
Index: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -398,6 +398,9 @@
};
bool isFreeingCall(const CallEvent &Call) const;
+ static bool isFreeingOwnershipAttrCall(const FunctionDecl *Func);
+
+ friend class NoOwnershipChangeVisitor;
CallDescriptionMap<CheckFn> AllocatingMemFnMap{
{{"alloca", 1}, &MallocChecker::checkAlloca},
@@ -742,7 +745,9 @@
namespace {
class NoOwnershipChangeVisitor final : public NoStateChangeFuncVisitor {
+ // The symbol whose (lack of) ownership change we are interested in.
SymbolRef Sym;
+ const MallocChecker &Checker;
using OwnerSet = llvm::SmallPtrSet<const MemRegion *, 8>;
// Collect which entities point to the allocated memory, and could be
@@ -794,6 +799,29 @@
return "";
}
+ /// Syntactically checks whether the callee is a deallocating function. Since
+ /// we have no path-sensitive information on this call (we would need a
+ /// CallEvent instead of a CallExpr for that), its possible that a
+ /// deallocation function was called indirectly through a function pointer,
+ /// but we are not able to tell, so this is a best effort analysis.
+ /// See namespace `memory_passed_to_fn_call_free_through_fn_ptr` in
+ /// clang/test/Analysis/NewDeleteLeaks.cpp.
+ bool isFreeingCallAsWritten(const CallExpr &Call) const {
+ if (Checker.FreeingMemFnMap.lookupAsWritten(Call) ||
+ Checker.ReallocatingMemFnMap.lookupAsWritten(Call))
+ return true;
+
+ if (const auto *Func =
+ llvm::dyn_cast_or_null<FunctionDecl>(Call.getCalleeDecl()))
+ return MallocChecker::isFreeingOwnershipAttrCall(Func);
+
+ return false;
+ }
+
+ /// Heuristically guess whether the callee intended to free memory. This is
+ /// done syntactically, because we are trying to argue about alternative
+ /// paths of execution, and as a consequence we don't have path-sensitive
+ /// information.
bool doesFnIntendToHandleOwnership(const Decl *Callee, ASTContext &ACtx) {
using namespace clang::ast_matchers;
const FunctionDecl *FD = dyn_cast<FunctionDecl>(Callee);
@@ -807,13 +835,21 @@
if (!FD || !FD->hasBody())
return false;
- // TODO: Operator delete is hardly the only deallocator -- Can we reuse
- // isFreeingCall() or something thats already here?
- auto Deallocations = match(
- stmt(hasDescendant(cxxDeleteExpr().bind("delete"))
- ), *FD->getBody(), ACtx);
- // TODO: Ownership my change with an attempt to store the allocated memory.
- return !Deallocations.empty();
+ auto Matches = match(findAll(stmt(anyOf(cxxDeleteExpr().bind("delete"),
+ callExpr().bind("call")))),
+ *FD->getBody(), ACtx);
+ for (BoundNodes Match : Matches) {
+ if (Match.getNodeAs<CXXDeleteExpr>("delete"))
+ return true;
+
+ if (const auto *Call = Match.getNodeAs<CallExpr>("call"))
+ if (isFreeingCallAsWritten(*Call))
+ return true;
+ }
+ // TODO: Ownership might change with an attempt to store the allocated
+ // memory, not only through deallocation. Check for attempted stores as
+ // well.
+ return false;
}
virtual bool
@@ -882,9 +918,9 @@
}
public:
- NoOwnershipChangeVisitor(SymbolRef Sym)
- : NoStateChangeFuncVisitor(bugreporter::TrackingKind::Thorough),
- Sym(Sym) {}
+ NoOwnershipChangeVisitor(SymbolRef Sym, const MallocChecker *Checker)
+ : NoStateChangeFuncVisitor(bugreporter::TrackingKind::Thorough), Sym(Sym),
+ Checker(*Checker) {}
void Profile(llvm::FoldingSetNodeID &ID) const override {
static int Tag = 0;
@@ -1069,12 +1105,8 @@
// Methods of MallocChecker and MallocBugVisitor.
//===----------------------------------------------------------------------===//
-bool MallocChecker::isFreeingCall(const CallEvent &Call) const {
- if (FreeingMemFnMap.lookup(Call) || ReallocatingMemFnMap.lookup(Call))
- return true;
-
- const auto *Func = dyn_cast<FunctionDecl>(Call.getDecl());
- if (Func && Func->hasAttrs()) {
+bool MallocChecker::isFreeingOwnershipAttrCall(const FunctionDecl *Func) {
+ if (Func->hasAttrs()) {
for (const auto *I : Func->specific_attrs<OwnershipAttr>()) {
OwnershipAttr::OwnershipKind OwnKind = I->getOwnKind();
if (OwnKind == OwnershipAttr::Takes || OwnKind == OwnershipAttr::Holds)
@@ -1084,6 +1116,16 @@
return false;
}
+bool MallocChecker::isFreeingCall(const CallEvent &Call) const {
+ if (FreeingMemFnMap.lookup(Call) || ReallocatingMemFnMap.lookup(Call))
+ return true;
+
+ if (const auto *Func = dyn_cast_or_null<FunctionDecl>(Call.getDecl()))
+ return isFreeingOwnershipAttrCall(Func);
+
+ return false;
+}
+
bool MallocChecker::isMemCall(const CallEvent &Call) const {
if (FreeingMemFnMap.lookup(Call) || AllocatingMemFnMap.lookup(Call) ||
ReallocatingMemFnMap.lookup(Call))
@@ -2756,7 +2798,7 @@
R->markInteresting(Sym);
R->addVisitor<MallocBugVisitor>(Sym, true);
if (ShouldRegisterNoOwnershipChangeVisitor)
- R->addVisitor<NoOwnershipChangeVisitor>(Sym);
+ R->addVisitor<NoOwnershipChangeVisitor>(Sym, this);
C.emitReport(std::move(R));
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
