Szelethus updated this revision to Diff 405979.
Szelethus edited the summary of this revision.
Szelethus added a comment.
Move `CallDescription` specific changes to D119004
<https://reviews.llvm.org/D119004>.
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,24 @@
} // 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
namespace memory_shared_with_ptr_of_shorter_lifetime {
@@ -123,6 +142,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,18 +799,38 @@
return "";
}
+ bool isFreeingCallImprecise(const CallExpr &Call) const {
+ if (Checker->FreeingMemFnMap.lookupImprecise(Call) ||
+ Checker->ReallocatingMemFnMap.lookupImprecise(Call))
+ return true;
+
+ if (const auto *Func = dyn_cast<FunctionDecl>(Call.getCalleeDecl()))
+ return MallocChecker::isFreeingOwnershipAttrCall(Func);
+
+ return false;
+ }
+
bool doesFnIntendToHandleOwnership(const Decl *Callee, ASTContext &ACtx) {
using namespace clang::ast_matchers;
const FunctionDecl *FD = dyn_cast<FunctionDecl>(Callee);
if (!FD)
return false;
- // TODO: Operator delete is hardly the only deallocator -- Can we reuse
+ // TODO: Operator delete is hardly the only deallocatr -- Can we reuse
// isFreeingCall() or something thats already here?
- auto Deallocations = match(
- stmt(hasDescendant(cxxDeleteExpr().bind("delete"))
- ), *FD->getBody(), ACtx);
+ 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 (isFreeingCallImprecise(*Call))
+ return true;
+ }
+ }
// TODO: Ownership my change with an attempt to store the allocated memory.
- return !Deallocations.empty();
+ return false;
}
virtual bool
@@ -874,9 +899,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;
@@ -1061,12 +1086,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)
@@ -1076,6 +1097,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<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))
@@ -2748,7 +2779,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
