NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a.sidorin, george.karpenkov, szepet.
Herald added a subscriber: rnkovacs.
This patch is roughly based on the discussion we've had in
http://lists.llvm.org/pipermail/cfe-dev/2017-December/056314.html about how our
support for C++ operator new() should provide useful callbacks to the checkers.
As in, this patch tries to do a minimal necessary thing while avoiding all
decisions that we didn't have a consensus over.
This patch also absorbs https://reviews.llvm.org/D36708.
In the discussion, we've been mentioning PreAllocator/PostAllocator callbacks.
However, we already sort of have them: they are PreCall/PostCall callbacks for
the allocator call, which are readily available in the `c++-allocator-inlining`
mode.
This patch also causes no change in the behavior of the pre/post `CXXNewExpr`
callbacks. They remain broken in the `c++-allocator-inlining` mode for now.
Missing in the current system of callbacks, however, is a callback that would
be called before construction while providing the //casted// return value of
operator new (in the sense of https://reviews.llvm.org/D41250: operator new
returns `void *`, but we need to have `C *`). The user can subscribe on the
allocator's `PostCall` and perform the cast manually, but that's an unpleasant
thing to do. So it sounds like a good idea to provide a callback that would
contain the relevant information.
A similar "Pre" callback doesn't seem necessary - there's not much additional
information we can provide compared to PreCall.
In this patch, I add the callback (with a lot of code that is the usual
boilerplate around the callback, though there's still a questionable TODO in
`CheckNewAllocatorContext::runChecker()`) and modify MallocChecker to make use
of it when `c++-allocator-inlining` mode is turned on. Specifically:
- If the allocator (operator new) call is evaluated conservatively,
`checkNewAllocator()` provides the return value of the allocator that is
semantically equivalent to the value in `checkPostStmt<CXXNewExpr>()` that
we've been using earlier. Without the cast, the value is different and the
checker crashes.
- If the allocator call is inlined, the checker does not treat it as an
allocator, but instead as a simple inlined function. That is, it does not start
tracking the return value here, and in particular avoids crashes. This policy
has already been there because previously we could inline the operator when it
was written without operator syntax - see tests in
`test/Analysis/NewDelete-custom.cpp`. Whether this policy was correct or not is
a matter of discussion. For now it means that in `c++-allocator-inlining` mode
we'd have new negatives (i.e. lost positives) on code that uses custom
allocators which do not boil down to simple malloc.
The `std::nothrow_t` tests in `NewDelete-custom.cpp` are affected in a
surprising manner. I'd fix them in a follow-up commit.
https://reviews.llvm.org/D41406
Files:
include/clang/StaticAnalyzer/Core/Checker.h
include/clang/StaticAnalyzer/Core/CheckerManager.h
lib/StaticAnalyzer/Checkers/MallocChecker.cpp
lib/StaticAnalyzer/Core/CheckerManager.cpp
lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
test/Analysis/NewDelete-custom.cpp
test/Analysis/new-ctor-malloc.cpp
Index: test/Analysis/new-ctor-malloc.cpp
===================================================================
--- /dev/null
+++ test/Analysis/new-ctor-malloc.cpp
@@ -0,0 +1,18 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection,unix.Malloc -analyzer-config c++-allocator-inlining=true -std=c++11 -verify %s
+
+void clang_analyzer_eval(bool);
+
+typedef __typeof__(sizeof(int)) size_t;
+
+void *malloc(size_t size);
+
+void *operator new(size_t size) throw() {
+ void *x = malloc(size);
+ if (!x)
+ return nullptr;
+ return x;
+}
+
+void checkNewAndConstructorInlining() {
+ int *s = new int;
+} // expected-warning {{Potential leak of memory pointed to by 's'}}
Index: test/Analysis/NewDelete-custom.cpp
===================================================================
--- test/Analysis/NewDelete-custom.cpp
+++ test/Analysis/NewDelete-custom.cpp
@@ -1,8 +1,10 @@
// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.NewDelete,unix.Malloc -std=c++11 -fblocks -verify %s
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.NewDelete,cplusplus.NewDeleteLeaks,unix.Malloc -std=c++11 -DLEAKS -fblocks -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.NewDelete,cplusplus.NewDeleteLeaks,unix.Malloc -std=c++11 -DLEAKS=1 -fblocks -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.NewDelete,unix.Malloc -std=c++11 -analyzer-config c++-allocator-inlining=true -DALLOCATOR_INLINING=1 -fblocks -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.NewDelete,cplusplus.NewDeleteLeaks,unix.Malloc -std=c++11 -analyzer-config c++-allocator-inlining=true -DLEAKS=1 -DALLOCATOR_INLINING=1 -fblocks -verify %s
#include "Inputs/system-header-simulator-cxx.h"
-#ifndef LEAKS
+#if !LEAKS
// expected-no-diagnostics
#endif
@@ -26,7 +28,7 @@
C *c3 = ::new C;
}
-#ifdef LEAKS
+#if LEAKS && !ALLOCATOR_INLINING
// expected-warning@-2{{Potential leak of memory pointed to by 'c3'}}
#endif
@@ -37,7 +39,7 @@
void testNewExprArray() {
int *p = new int[0];
}
-#ifdef LEAKS
+#if LEAKS && !ALLOCATOR_INLINING
// expected-warning@-2{{Potential leak of memory pointed to by 'p'}}
#endif
@@ -50,30 +52,30 @@
void testNewExpr() {
int *p = new int;
}
-#ifdef LEAKS
+#if LEAKS && !ALLOCATOR_INLINING
// expected-warning@-2{{Potential leak of memory pointed to by 'p'}}
#endif
//----- Custom NoThrow placement operators
void testOpNewNoThrow() {
void *p = operator new(0, std::nothrow);
}
-#ifdef LEAKS
+#if LEAKS
// expected-warning@-2{{Potential leak of memory pointed to by 'p'}}
#endif
void testNewExprNoThrow() {
int *p = new(std::nothrow) int;
}
-#ifdef LEAKS
+#if LEAKS
// expected-warning@-2{{Potential leak of memory pointed to by 'p'}}
#endif
//----- Custom placement operators
void testOpNewPlacement() {
void *p = operator new(0, 0.1); // no warn
-}
+}
void testNewExprPlacement() {
int *p = new(0.1) int; // no warn
Index: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
===================================================================
--- lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -469,8 +469,13 @@
CNE->getType()));
}
- getCheckerManager().runCheckersForPostCall(Dst, DstPostValue,
- *Call, *this);
+ ExplodedNodeSet DstPostPostCallCallback;
+ getCheckerManager().runCheckersForPostCall(DstPostPostCallCallback,
+ DstPostValue, *Call, *this);
+ for (auto I : DstPostPostCallCallback) {
+ getCheckerManager().runCheckersForNewAllocator(
+ CNE, peekCXXNewAllocatorValue(I->getState()), Dst, I, *this);
+ }
}
bool ExprEngine::isStandardGlobalOperatorNewFunction(const CXXNewExpr *CNE) {
Index: lib/StaticAnalyzer/Core/CheckerManager.cpp
===================================================================
--- lib/StaticAnalyzer/Core/CheckerManager.cpp
+++ lib/StaticAnalyzer/Core/CheckerManager.cpp
@@ -469,6 +469,46 @@
expandGraphWithCheckers(C, Dst, Src);
}
+namespace {
+ struct CheckNewAllocatorContext {
+ typedef std::vector<CheckerManager::CheckNewAllocatorFunc> CheckersTy;
+ const CheckersTy &Checkers;
+ const CXXNewExpr *NE;
+ SVal Target;
+ bool WasInlined;
+ ExprEngine &Eng;
+
+ CheckersTy::const_iterator checkers_begin() { return Checkers.begin(); }
+ CheckersTy::const_iterator checkers_end() { return Checkers.end(); }
+
+ CheckNewAllocatorContext(const CheckersTy &Checkers, const CXXNewExpr *NE,
+ SVal Target, bool WasInlined, ExprEngine &Eng)
+ : Checkers(Checkers), NE(NE), Target(Target), WasInlined(WasInlined),
+ Eng(Eng) {}
+
+ void runChecker(CheckerManager::CheckNewAllocatorFunc checkFn,
+ NodeBuilder &Bldr, ExplodedNode *Pred) {
+ // TODO: Does this deserve a custom program point? For now we're re-using
+ // PostImplicitCall because we're guaranteed to use the non-implicit
+ // PostStmt for the PostCall callback, because we have some sort of
+ // call site (CXXNewExpr) in this scenario.
+ ProgramPoint L = PostImplicitCall(NE->getOperatorNew(), NE->getLocStart(),
+ Pred->getLocationContext());
+ CheckerContext C(Bldr, Eng, Pred, L);
+ checkFn(NE, Target, C);
+ }
+ };
+}
+
+void CheckerManager::runCheckersForNewAllocator(
+ const CXXNewExpr *NE, SVal Target, ExplodedNodeSet &Dst, ExplodedNode *Pred,
+ ExprEngine &Eng, bool WasInlined) {
+ ExplodedNodeSet Src;
+ Src.insert(Pred);
+ CheckNewAllocatorContext C(NewAllocatorCheckers, NE, Target, WasInlined, Eng);
+ expandGraphWithCheckers(C, Dst, Src);
+}
+
/// \brief Run checkers for live symbols.
void CheckerManager::runCheckersForLiveSymbols(ProgramStateRef state,
SymbolReaper &SymReaper) {
@@ -711,6 +751,10 @@
BranchConditionCheckers.push_back(checkfn);
}
+void CheckerManager::_registerForNewAllocator(CheckNewAllocatorFunc checkfn) {
+ NewAllocatorCheckers.push_back(checkfn);
+}
+
void CheckerManager::_registerForLiveSymbols(CheckLiveSymbolsFunc checkfn) {
LiveSymbolsCheckers.push_back(checkfn);
}
Index: lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===================================================================
--- lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -162,6 +162,7 @@
check::PreCall,
check::PostStmt<CallExpr>,
check::PostStmt<CXXNewExpr>,
+ check::NewAllocator,
check::PreStmt<CXXDeleteExpr>,
check::PostStmt<BlockExpr>,
check::PostObjCMessage,
@@ -207,6 +208,8 @@
void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
void checkPostStmt(const CallExpr *CE, CheckerContext &C) const;
void checkPostStmt(const CXXNewExpr *NE, CheckerContext &C) const;
+ void checkNewAllocator(const CXXNewExpr *NE, SVal Target,
+ CheckerContext &C) const;
void checkPreStmt(const CXXDeleteExpr *DE, CheckerContext &C) const;
void checkPostObjCMessage(const ObjCMethodCall &Call, CheckerContext &C) const;
void checkPostStmt(const BlockExpr *BE, CheckerContext &C) const;
@@ -281,10 +284,16 @@
bool isStandardNewDelete(const FunctionDecl *FD, ASTContext &C) const;
///@}
+ /// \brief Process C++ operator new()'s allocation, which is the part of C++
+ /// new-expression that goes before the constructor.
+ void processNewAllocation(const CXXNewExpr *NE, CheckerContext &C,
+ SVal Target) const;
+
/// \brief Perform a zero-allocation check.
ProgramStateRef ProcessZeroAllocation(CheckerContext &C, const Expr *E,
const unsigned AllocationSizeArg,
- ProgramStateRef State) const;
+ ProgramStateRef State,
+ Optional<SVal> RetVal = None) const;
ProgramStateRef MallocMemReturnsAttr(CheckerContext &C,
const CallExpr *CE,
@@ -300,7 +309,7 @@
AllocationFamily Family = AF_Malloc);
static ProgramStateRef addExtentSize(CheckerContext &C, const CXXNewExpr *NE,
- ProgramStateRef State);
+ ProgramStateRef State, SVal Target);
// Check if this malloc() for special flags. At present that means M_ZERO or
// __GFP_ZERO (in which case, treat it like calloc).
@@ -311,7 +320,8 @@
/// Update the RefState to reflect the new memory allocation.
static ProgramStateRef
MallocUpdateRefState(CheckerContext &C, const Expr *E, ProgramStateRef State,
- AllocationFamily Family = AF_Malloc);
+ AllocationFamily Family = AF_Malloc,
+ Optional<SVal> RetVal = None);
ProgramStateRef FreeMemAttr(CheckerContext &C, const CallExpr *CE,
const OwnershipAttr* Att,
@@ -949,13 +959,15 @@
}
// Performs a 0-sized allocations check.
-ProgramStateRef MallocChecker::ProcessZeroAllocation(CheckerContext &C,
- const Expr *E,
- const unsigned AllocationSizeArg,
- ProgramStateRef State) const {
+ProgramStateRef MallocChecker::ProcessZeroAllocation(
+ CheckerContext &C, const Expr *E, const unsigned AllocationSizeArg,
+ ProgramStateRef State, Optional<SVal> retVal) const {
if (!State)
return nullptr;
+ if (!retVal)
+ retVal = C.getSVal(E);
+
const Expr *Arg = nullptr;
if (const CallExpr *CE = dyn_cast<CallExpr>(E)) {
@@ -988,8 +1000,7 @@
State->assume(SvalBuilder.evalEQ(State, *DefArgVal, Zero));
if (TrueState && !FalseState) {
- SVal retVal = State->getSVal(E, C.getLocationContext());
- SymbolRef Sym = retVal.getAsLocSymbol();
+ SymbolRef Sym = retVal->getAsLocSymbol();
if (!Sym)
return State;
@@ -1050,9 +1061,9 @@
return false;
}
-void MallocChecker::checkPostStmt(const CXXNewExpr *NE,
- CheckerContext &C) const {
-
+void MallocChecker::processNewAllocation(const CXXNewExpr *NE,
+ CheckerContext &C,
+ SVal Target) const {
if (NE->getNumPlacementArgs())
for (CXXNewExpr::const_arg_iterator I = NE->placement_arg_begin(),
E = NE->placement_arg_end(); I != E; ++I)
@@ -1072,37 +1083,47 @@
// MallocUpdateRefState() instead of MallocMemAux() which breakes the
// existing binding.
State = MallocUpdateRefState(C, NE, State, NE->isArray() ? AF_CXXNewArray
- : AF_CXXNew);
- State = addExtentSize(C, NE, State);
- State = ProcessZeroAllocation(C, NE, 0, State);
+ : AF_CXXNew, Target);
+ State = addExtentSize(C, NE, State, Target);
+ State = ProcessZeroAllocation(C, NE, 0, State, Target);
C.addTransition(State);
}
+void MallocChecker::checkPostStmt(const CXXNewExpr *NE,
+ CheckerContext &C) const {
+ if (!C.getAnalysisManager().getAnalyzerOptions().mayInlineCXXAllocator())
+ processNewAllocation(NE, C, C.getSVal(NE));
+}
+
+void MallocChecker::checkNewAllocator(const CXXNewExpr *NE, SVal Target,
+ CheckerContext &C) const {
+ processNewAllocation(NE, C, Target);
+}
+
// Sets the extent value of the MemRegion allocated by
// new expression NE to its size in Bytes.
//
ProgramStateRef MallocChecker::addExtentSize(CheckerContext &C,
const CXXNewExpr *NE,
- ProgramStateRef State) {
+ ProgramStateRef State,
+ SVal Target) {
if (!State)
return nullptr;
SValBuilder &svalBuilder = C.getSValBuilder();
SVal ElementCount;
- const LocationContext *LCtx = C.getLocationContext();
const SubRegion *Region;
if (NE->isArray()) {
const Expr *SizeExpr = NE->getArraySize();
ElementCount = State->getSVal(SizeExpr, C.getLocationContext());
// Store the extent size for the (symbolic)region
// containing the elements.
- Region = (State->getSVal(NE, LCtx))
- .getAsRegion()
+ Region = Target.getAsRegion()
->getAs<SubRegion>()
->getSuperRegion()
->getAs<SubRegion>();
} else {
ElementCount = svalBuilder.makeIntVal(1, true);
- Region = (State->getSVal(NE, LCtx)).getAsRegion()->getAs<SubRegion>();
+ Region = Target.getAsRegion()->getAs<SubRegion>();
}
assert(Region);
@@ -1263,18 +1284,20 @@
ProgramStateRef MallocChecker::MallocUpdateRefState(CheckerContext &C,
const Expr *E,
ProgramStateRef State,
- AllocationFamily Family) {
+ AllocationFamily Family,
+ Optional<SVal> retVal) {
if (!State)
return nullptr;
// Get the return value.
- SVal retVal = State->getSVal(E, C.getLocationContext());
+ if (!retVal)
+ retVal = State->getSVal(E, C.getLocationContext());
// We expect the malloc functions to return a pointer.
- if (!retVal.getAs<Loc>())
+ if (!retVal->getAs<Loc>())
return nullptr;
- SymbolRef Sym = retVal.getAsLocSymbol();
+ SymbolRef Sym = retVal->getAsLocSymbol();
assert(Sym);
// Set the symbol's state to Allocated.
Index: include/clang/StaticAnalyzer/Core/CheckerManager.h
===================================================================
--- include/clang/StaticAnalyzer/Core/CheckerManager.h
+++ include/clang/StaticAnalyzer/Core/CheckerManager.h
@@ -303,6 +303,13 @@
ExplodedNodeSet &Dst, ExplodedNode *Pred,
ExprEngine &Eng);
+ /// \brief Run checkers between C++ operator new and constructor calls.
+ void runCheckersForNewAllocator(const CXXNewExpr *NE, SVal Target,
+ ExplodedNodeSet &Dst,
+ ExplodedNode *Pred,
+ ExprEngine &Eng,
+ bool wasInlined = false);
+
/// \brief Run checkers for live symbols.
///
/// Allows modifying SymbolReaper object. For example, checkers can explicitly
@@ -437,6 +444,9 @@
typedef CheckerFn<void (const Stmt *, CheckerContext &)>
CheckBranchConditionFunc;
+
+ typedef CheckerFn<void (const CXXNewExpr *, SVal, CheckerContext &)>
+ CheckNewAllocatorFunc;
typedef CheckerFn<void (SymbolReaper &, CheckerContext &)>
CheckDeadSymbolsFunc;
@@ -494,6 +504,8 @@
void _registerForBranchCondition(CheckBranchConditionFunc checkfn);
+ void _registerForNewAllocator(CheckNewAllocatorFunc checkfn);
+
void _registerForLiveSymbols(CheckLiveSymbolsFunc checkfn);
void _registerForDeadSymbols(CheckDeadSymbolsFunc checkfn);
@@ -603,6 +615,8 @@
std::vector<CheckBranchConditionFunc> BranchConditionCheckers;
+ std::vector<CheckNewAllocatorFunc> NewAllocatorCheckers;
+
std::vector<CheckLiveSymbolsFunc> LiveSymbolsCheckers;
std::vector<CheckDeadSymbolsFunc> DeadSymbolsCheckers;
Index: include/clang/StaticAnalyzer/Core/Checker.h
===================================================================
--- include/clang/StaticAnalyzer/Core/Checker.h
+++ include/clang/StaticAnalyzer/Core/Checker.h
@@ -283,6 +283,22 @@
}
};
+class NewAllocator {
+ template <typename CHECKER>
+ static void _checkNewAllocator(void *checker, const CXXNewExpr *NE,
+ SVal Target, CheckerContext &C) {
+ ((const CHECKER *)checker)->checkNewAllocator(NE, Target, C);
+ }
+
+public:
+ template <typename CHECKER>
+ static void _register(CHECKER *checker, CheckerManager &mgr) {
+ mgr._registerForNewAllocator(
+ CheckerManager::CheckNewAllocatorFunc(checker,
+ _checkNewAllocator<CHECKER>));
+ }
+};
+
class LiveSymbols {
template <typename CHECKER>
static void _checkLiveSymbols(void *checker, ProgramStateRef state,
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
