Index: clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h =================================================================== --- clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h +++ clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h @@ -56,6 +56,7 @@ namespace ento { enum CallEventKind { + CE_CXXDeallocator, CE_Function, CE_CXXMember, CE_CXXMemberOperator, @@ -1045,6 +1046,54 @@ } }; +/// Represents the memory deallocation call in a C++ delete-expression. +/// +/// This is a call to "operator delete". +// FIXME: CXXDeleteExpr isn't present for custom delete operators, or even for +// some those that are in the standard library, like the no-throw or align_val +// versions. +// Some pointers: +// http://lists.llvm.org/pipermail/cfe-dev/2020-April/065080.html +// clang/test/Analysis/cxx-dynamic-memory-analysis-order.cpp +// clang/unittests/StaticAnalyzer/CallEventTest.cpp +class CXXDeallocatorCall : public AnyFunctionCall { + friend class CallEventManager; + +protected: + CXXDeallocatorCall(const CXXDeleteExpr *E, ProgramStateRef St, + const LocationContext *LCtx) + : AnyFunctionCall(E, St, LCtx) {} + CXXDeallocatorCall(const CXXDeallocatorCall &Other) = default; + + void cloneTo(void *Dest) const override { + new (Dest) CXXDeallocatorCall(*this); + } + +public: + virtual const CXXDeleteExpr *getOriginExpr() const { + return cast(AnyFunctionCall::getOriginExpr()); + } + + const FunctionDecl *getDecl() const override { + return getOriginExpr()->getOperatorDelete(); + } + + unsigned getNumArgs() const override { return getDecl()->getNumParams(); } + + const Expr *getArgExpr(unsigned Index = 0) const override { + return getOriginExpr()->getArgument(); + } + + Kind getKind() const override { return CE_CXXDeallocator; } + virtual StringRef getKindAsString() const override { + return "CXXDeallocatorCall"; + } + + static bool classof(const CallEvent *CE) { + return CE->getKind() == CE_CXXDeallocator; + } +}; + /// Represents the ways an Objective-C message send can occur. // // Note to maintainers: OCM_Message should always be last, since it does not @@ -1370,6 +1419,12 @@ const LocationContext *LCtx) { return create(E, State, LCtx); } + + CallEventRef + getCXXDeallocatorCall(const CXXDeleteExpr *E, ProgramStateRef State, + const LocationContext *LCtx) { + return create(E, State, LCtx); + } }; template Index: clang/lib/StaticAnalyzer/Checkers/DeleteWithNonVirtualDtorChecker.cpp =================================================================== --- clang/lib/StaticAnalyzer/Checkers/DeleteWithNonVirtualDtorChecker.cpp +++ clang/lib/StaticAnalyzer/Checkers/DeleteWithNonVirtualDtorChecker.cpp @@ -92,6 +92,8 @@ "Logic error")); ExplodedNode *N = C.generateNonFatalErrorNode(); + if (!N) + return; auto R = std::make_unique(*BT, BT->getDescription(), N); // Mark region of problematic base class for later use in the BugVisitor. Index: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp =================================================================== --- clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp +++ clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp @@ -284,8 +284,8 @@ check::ConstPointerEscape, check::PreStmt, check::EndFunction, check::PreCall, check::PostCall, check::PostStmt, check::NewAllocator, - check::PreStmt, check::PostStmt, - check::PostObjCMessage, check::Location, eval::Assume> { + check::PostStmt, check::PostObjCMessage, + check::Location, eval::Assume> { public: /// In pessimistic mode, the checker assumes that it does not know which /// functions might free the memory. @@ -317,7 +317,6 @@ 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; void checkDeadSymbols(SymbolReaper &SymReaper, CheckerContext &C) const; @@ -1412,25 +1411,6 @@ return State; } -void MallocChecker::checkPreStmt(const CXXDeleteExpr *DE, - CheckerContext &C) const { - - if (!ChecksEnabled[CK_NewDeleteChecker]) - if (SymbolRef Sym = C.getSVal(DE->getArgument()).getAsSymbol()) - checkUseAfterFree(Sym, C, DE->getArgument()); - - if (!isStandardNewDelete(DE->getOperatorDelete())) - return; - - ProgramStateRef State = C.getState(); - bool IsKnownToBeAllocated; - State = FreeMemAux(C, DE->getArgument(), DE, State, - /*Hold*/ false, IsKnownToBeAllocated, - (DE->isArrayForm() ? AF_CXXNewArray : AF_CXXNew)); - - C.addTransition(State); -} - static bool isKnownDeallocObjCMethodName(const ObjCMethodCall &Call) { // If the first selector piece is one of the names below, assume that the // object takes ownership of the memory, promising to eventually deallocate it @@ -2620,7 +2600,27 @@ void MallocChecker::checkPreCall(const CallEvent &Call, CheckerContext &C) const { - if (const CXXDestructorCall *DC = dyn_cast(&Call)) { + if (const auto *DC = dyn_cast(&Call)) { + const CXXDeleteExpr *DE = DC->getOriginExpr(); + + if (!ChecksEnabled[CK_NewDeleteChecker]) + if (SymbolRef Sym = C.getSVal(DE->getArgument()).getAsSymbol()) + checkUseAfterFree(Sym, C, DE->getArgument()); + + if (!isStandardNewDelete(DC->getDecl())) + return; + + ProgramStateRef State = C.getState(); + bool IsKnownToBeAllocated; + State = FreeMemAux(C, DE->getArgument(), DE, State, + /*Hold*/ false, IsKnownToBeAllocated, + (DE->isArrayForm() ? AF_CXXNewArray : AF_CXXNew)); + + C.addTransition(State); + return; + } + + if (const auto *DC = dyn_cast(&Call)) { SymbolRef Sym = DC->getCXXThisVal().getAsSymbol(); if (!Sym || checkDoubleDelete(Sym, C)) return; Index: clang/lib/StaticAnalyzer/Core/ExprEngine.cpp =================================================================== --- clang/lib/StaticAnalyzer/Core/ExprEngine.cpp +++ clang/lib/StaticAnalyzer/Core/ExprEngine.cpp @@ -1648,8 +1648,10 @@ ExplodedNodeSet PreVisit; const auto *CDE = cast(S); getCheckerManager().runCheckersForPreStmt(PreVisit, Pred, S, *this); + ExplodedNodeSet PostVisit; + getCheckerManager().runCheckersForPreStmt(PostVisit, PreVisit, S, *this); - for (const auto i : PreVisit) + for (const auto i : PostVisit) VisitCXXDeleteExpr(CDE, i, Dst); Bldr.addNodes(Dst); Index: clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp =================================================================== --- clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp +++ clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp @@ -875,13 +875,18 @@ void ExprEngine::VisitCXXDeleteExpr(const CXXDeleteExpr *CDE, ExplodedNode *Pred, ExplodedNodeSet &Dst) { - StmtNodeBuilder Bldr(Pred, Dst, *currBldrCtx); - ProgramStateRef state = Pred->getState(); - Bldr.generateNode(CDE, Pred, state); + + CallEventManager &CEMgr = getStateManager().getCallEventManager(); + CallEventRef Call = CEMgr.getCXXDeallocatorCall( + CDE, Pred->getState(), Pred->getLocationContext()); + + ExplodedNodeSet DstPreCall; + getCheckerManager().runCheckersForPreCall(DstPreCall, Pred, *Call, *this); + + getCheckerManager().runCheckersForPostCall(Dst, DstPreCall, *Call, *this); } -void ExprEngine::VisitCXXCatchStmt(const CXXCatchStmt *CS, - ExplodedNode *Pred, +void ExprEngine::VisitCXXCatchStmt(const CXXCatchStmt *CS, ExplodedNode *Pred, ExplodedNodeSet &Dst) { const VarDecl *VD = CS->getExceptionDecl(); if (!VD) { Index: clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp =================================================================== --- clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp +++ clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp @@ -10,17 +10,18 @@ // //===----------------------------------------------------------------------===// -#include "clang/AST/Decl.h" -#include "clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h" #include "PrettyStackTraceLocationContext.h" #include "clang/AST/CXXInheritance.h" +#include "clang/AST/Decl.h" #include "clang/AST/DeclCXX.h" #include "clang/Analysis/Analyses/LiveVariables.h" #include "clang/Analysis/ConstructionContext.h" #include "clang/StaticAnalyzer/Core/CheckerManager.h" #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h" #include "llvm/ADT/SmallSet.h" #include "llvm/ADT/Statistic.h" +#include "llvm/Support/Compiler.h" #include "llvm/Support/SaveAndRestore.h" using namespace clang; @@ -819,6 +820,8 @@ return CIP_DisallowedOnce; break; } + case CE_CXXDeallocator: + LLVM_FALLTHROUGH; case CE_CXXAllocator: if (Opts.MayInlineCXXAllocator) break; Index: clang/test/Analysis/cxx-dynamic-memory-analysis-order.cpp =================================================================== --- clang/test/Analysis/cxx-dynamic-memory-analysis-order.cpp +++ clang/test/Analysis/cxx-dynamic-memory-analysis-order.cpp @@ -28,8 +28,8 @@ // Default behavior: Calls operator delete(ptr), or operator delete(ptr, // alignment), respectively. - // FIXME: All calls to operator new should be CXXAllocatorCall. - // FIXME: PostStmt should be present. + // FIXME: All calls to operator new should be CXXAllocatorCall, and calls to + // operator delete should be CXXDeallocatorCall. { int *p = new int; delete p; @@ -38,6 +38,9 @@ // CHECK-NEXT: PreStmt // CHECK-NEXT: PostStmt // CHECK-NEXT: PreStmt + // CHECK-NEXT: PreStmt + // CHECK-NEXT: PreCall (operator delete) [CXXDeallocatorCall] + // CHECK-NEXT: PostCall (operator delete) [CXXDeallocatorCall] p = new int; operator delete(p, 23542368); @@ -87,6 +90,9 @@ // CHECK-NEXT: PreStmt // CHECK-NEXT: PostStmt // CHECK-NEXT: PreStmt + // CHECK-NEXT: PreStmt + // CHECK-NEXT: PreCall (operator delete[]) [CXXDeallocatorCall] + // CHECK-NEXT: PostCall (operator delete[]) [CXXDeallocatorCall] p = new int[2]; operator delete[](p, 23542368); Index: clang/unittests/StaticAnalyzer/CMakeLists.txt =================================================================== --- clang/unittests/StaticAnalyzer/CMakeLists.txt +++ clang/unittests/StaticAnalyzer/CMakeLists.txt @@ -5,6 +5,7 @@ add_clang_unittest(StaticAnalysisTests AnalyzerOptionsTest.cpp CallDescriptionTest.cpp + CallEventTest.cpp StoreTest.cpp RegisterCustomCheckersTest.cpp SymbolReaperTest.cpp Index: clang/unittests/StaticAnalyzer/CallEventTest.cpp =================================================================== --- /dev/null +++ clang/unittests/StaticAnalyzer/CallEventTest.cpp @@ -0,0 +1,89 @@ +//===----------------------------------------------------------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h" +#include "CheckerRegistration.h" +#include "clang/Basic/LLVM.h" +#include "clang/Frontend/CompilerInstance.h" +#include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h" +#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" +#include "clang/StaticAnalyzer/Core/BugReporter/CommonBugCategories.h" +#include "clang/StaticAnalyzer/Core/Checker.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h" +#include "clang/StaticAnalyzer/Frontend/AnalysisConsumer.h" +#include "clang/StaticAnalyzer/Frontend/CheckerRegistry.h" +#include "clang/Tooling/Tooling.h" +#include "gtest/gtest.h" + +namespace clang { +namespace ento { +namespace { + +void reportBug(const CheckerBase *Checker, const CallEvent &Call, + CheckerContext &C, StringRef WarningMsg) { + C.getBugReporter().EmitBasicReport( + nullptr, Checker, "", categories::LogicError, WarningMsg, + PathDiagnosticLocation(Call.getOriginExpr(), C.getSourceManager(), + C.getLocationContext()), + {}); +} + +class CXXDeallocatorChecker : public Checker { + std::unique_ptr BT_uninitField; + +public: + CXXDeallocatorChecker() + : BT_uninitField(new BuiltinBug(this, "CXXDeallocator")) {} + + void checkPreCall(const CallEvent &Call, CheckerContext &C) const { + const auto *DC = dyn_cast(&Call); + if (!DC) { + return; + } + + SmallString<100> WarningBuf; + llvm::raw_svector_ostream WarningOS(WarningBuf); + WarningOS << "NumArgs: " << DC->getNumArgs(); + + reportBug(this, *DC, C, WarningBuf); + } +}; + +void addCXXDeallocatorChecker(AnalysisASTConsumer &AnalysisConsumer, + AnalyzerOptions &AnOpts) { + AnOpts.CheckersAndPackages = {{"test.CXXDeallocator", true}}; + AnalysisConsumer.AddCheckerRegistrationFn([](CheckerRegistry &Registry) { + Registry.addChecker("test.CXXDeallocator", + "Description", ""); + }); +} + +// TODO: What we should really be testing here is all the different varieties +// of delete operators, and wether the retrieval of their arguments works as +// intended. At the time of writing this file, CXXDeallocatorCall doesn't pick +// up on much of those due to the AST not containing CXXDeleteExpr for most of +// the standard/custom deletes. +TEST(CXXDeallocatorCall, SimpleDestructor) { + std::string Diags; + EXPECT_TRUE(runCheckerOnCode(R"( + struct A {}; + + void f() { + A *a = new A; + delete a; + } + )", + Diags)); + EXPECT_EQ(Diags, "test.CXXDeallocator:NumArgs: 1\n"); +} + +} // namespace +} // namespace ento +} // namespace clang