Szelethus created this revision.
Szelethus added reviewers: NoQ, steakhal, martong, balazske, ASDenysPetrov.
Szelethus added a project: clang.
Herald added subscribers: manas, gamesh411, dkrupp, donat.nagy,
mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware, xazax.hun,
yaxunl.
Szelethus requested review of this revision.
Herald added a subscriber: cfe-commits.
The problem with leak bug reports is that the most interesting event in the
code is likely the one that did //not// happen -- lack of ownership change and
lack of deallocation, which is often present within the same function that the
analyzer inlined anyway, but not on the path of execution on which the bug
occured. We struggle to understand that a function was responsible for freeing
the memory, but failed.
D105819 <https://reviews.llvm.org/D105819> added a new visitor to improve
memory leak bug reports. In addition to inspecting the ExplodedNodes of the bug
pat, the visitor tries to guess whether the function was supposed to free
memory, but failed to. Initially (in D108753
<https://reviews.llvm.org/D108753>), this was done by checking whether a
`CXXDeleteExpr` is present in the function. If so, we assume that the function
was at least party responsible, and prevent the analyzer from pruning bug
report notes in it. This patch improves this heuristic by recognizing all
deallocator functions that MallocChecker itself recognizes, by reusing
`MallocChecker::isFreeingCall`.
However, we are only able to match a `CallEvent` against a `CallDescription`.
`CallEvent`s are created during symbolic execution, providing a lot of
additional information about the call, but the grand idea behind this visitor
that it checks code that was //not// executed smybolically (well, not on the
path of execution on which the bug was found). For this reason, I added a set
of functions to allow matching a `CallExpr` against a `CallDescription`.
I am aware that this idea may induce strong opinions -- after all, I'm adding a
a new interface called `.*Imprecise`, discourage people from using it in the
comments, while we already have one too many confusing interfaces in the
analyzer, not to mention that `CallDescription` was meant to be among the
better documented, newcomer friendlier and more intuitive tools we have. Also,
its possible that matching function calls in syntactic-only cases should be
done by `StdLibraryFunctionChecker::Signature` instead.
I argue for this patch because
- A lot of checkers use `CallDescriptionMap` already, and we either never
intend to change to `Signature`, or it would be an enormous effort upfront
- Leak checkers like `StreamChecker` could benefit from this as well
- The comments I believe resolve the unintuitiveness of the new interface well
- In the case of this patch, matching `CallEvent`s for plain C function calls
is pretty much matching against a `CallExpr` anyways.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D118880
Files:
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h
clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
clang/lib/StaticAnalyzer/Core/CallDescription.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/Core/CallDescription.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Core/CallDescription.cpp
+++ clang/lib/StaticAnalyzer/Core/CallDescription.cpp
@@ -13,6 +13,7 @@
//===----------------------------------------------------------------------===//
#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
+#include "clang/AST/Decl.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
#include "llvm/ADT/ArrayRef.h"
@@ -61,15 +62,32 @@
if (!FD)
return false;
+ return matchesImpl(FD, Call.getNumArgs(), Call.parameters().size());
+}
+
+bool ento::CallDescription::matchesImprecise(const CallExpr &CE) const {
+ const auto *FD = dyn_cast_or_null<FunctionDecl>(CE.getCalleeDecl());
+ if (!FD)
+ return false;
+
+ return matchesImpl(FD, CE.getNumArgs(), FD->param_size());
+}
+
+bool ento::CallDescription::matchesImpl(const FunctionDecl *Callee,
+ size_t ArgCount,
+ size_t ParamCount) const {
+ const auto *FD = Callee;
+ if (!FD)
+ return false;
+
if (Flags & CDF_MaybeBuiltin) {
return CheckerContext::isCLibraryFunction(FD, getFunctionName()) &&
- (!RequiredArgs || *RequiredArgs <= Call.getNumArgs()) &&
- (!RequiredParams || *RequiredParams <= Call.parameters().size());
+ (!RequiredArgs || *RequiredArgs <= ArgCount) &&
+ (!RequiredParams || *RequiredParams <= ParamCount);
}
if (!II.hasValue()) {
- II = &Call.getState()->getStateManager().getContext().Idents.get(
- getFunctionName());
+ II = &FD->getASTContext().Idents.get(getFunctionName());
}
const auto MatchNameOnly = [](const CallDescription &CD,
@@ -86,11 +104,11 @@
};
const auto ExactMatchArgAndParamCounts =
- [](const CallEvent &Call, const CallDescription &CD) -> bool {
- const bool ArgsMatch =
- !CD.RequiredArgs || *CD.RequiredArgs == Call.getNumArgs();
+ [](size_t ArgCount, size_t ParamCount,
+ const CallDescription &CD) -> bool {
+ const bool ArgsMatch = !CD.RequiredArgs || *CD.RequiredArgs == ArgCount;
const bool ParamsMatch =
- !CD.RequiredParams || *CD.RequiredParams == Call.parameters().size();
+ !CD.RequiredParams || *CD.RequiredParams == ParamCount;
return ArgsMatch && ParamsMatch;
};
@@ -122,7 +140,7 @@
};
// Let's start matching...
- if (!ExactMatchArgAndParamCounts(Call, *this))
+ if (!ExactMatchArgAndParamCounts(ArgCount, ParamCount, *this))
return false;
if (!MatchNameOnly(*this, FD))
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));
}
Index: clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h
===================================================================
--- clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h
+++ clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h
@@ -99,6 +99,26 @@
/// calls.
bool matches(const CallEvent &Call) const;
+ /// Returns true if the CallEvent is a call to a function that matches
+ /// the CallDescription.
+ ///
+ /// When available, always prefer matching with a CallEvent! This function
+ /// exists only when that is not available, for example, when _only_
+ /// syntactic check is done on a piece of code.
+ ///
+ /// Also, StdLibraryFunctionsChecker::Signature is likely a better candicade
+ /// for syntactic only matching if you are writing a new checker. This is
+ /// handy if a CallDescriptionMap is already there.
+ ///
+ /// The function is imprecise because CallEvent understands the precise
+ /// argument count better (see comments for CallEvent::getNumArgs).
+ bool matchesImprecise(const CallExpr &CE) const;
+
+private:
+ bool matchesImpl(const FunctionDecl *Callee, size_t ArgCount,
+ size_t ParamCount) const;
+
+public:
/// Returns true whether the CallEvent matches on any of the CallDescriptions
/// supplied.
///
@@ -156,6 +176,18 @@
return nullptr;
}
+
+ /// ALWAYS prefer lookup with a CallEvent, when available. See comments above
+ /// CallDescription::matchesImprecise.
+ LLVM_NODISCARD const T *lookupImprecise(const CallExpr &Call) const {
+ // Slow path: linear lookup.
+ // TODO: Implement some sort of fast path.
+ for (const std::pair<CallDescription, T> &I : LinearMap)
+ if (I.first.matchesImprecise(Call))
+ return &I.second;
+
+ return nullptr;
+ }
};
/// An immutable set of CallDescriptions.
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
