llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang Author: Donát Nagy (NagyDonat) <details> <summary>Changes</summary> This commit converts MallocChecker to the new checker family framework that was introduced in the recent commit 6833076a5d9f5719539a24e900037da5a3979289 -- and gets rid of some awkward unintended interactions between the checker frontends. --- Patch is 33.09 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/147080.diff 3 Files Affected: - (modified) clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp (+176-238) - (modified) clang/test/Analysis/new.cpp (+4-36) - (added) clang/test/Analysis/test-member-invalidation.cpp (+47) ``````````diff diff --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp index a33e61fabc2c1..9e7540eecc8ee 100644 --- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp @@ -333,11 +333,55 @@ template <typename T> static bool isStandardNewDelete(const T &FD) { return isStandardDelete(FD) || isStandardNew(FD); } +namespace { + //===----------------------------------------------------------------------===// -// Definition of the MallocChecker class. +// Utility classes that provide access to the bug types and can model that some +// of the bug types are shared by multiple checker frontends. //===----------------------------------------------------------------------===// -namespace { +#define BUGTYPE_PROVIDER(NAME, DEF) \ + struct NAME : virtual public CheckerFrontend { \ + BugType NAME##Bug{this, DEF, categories::MemoryError}; \ + }; + +BUGTYPE_PROVIDER(DoubleFree, "Double free") +// TODO: Remove DoubleDelete as a separate bug type and when it would be +// emitted, emit DoubleFree reports instead. (Note that DoubleFree is already +// used for all allocation families, not just malloc/free.) +BUGTYPE_PROVIDER(DoubleDelete, "Double delete") + +struct Leak : virtual public CheckerFrontend { + // Leaks should not be reported if they are post-dominated by a sink: + // (1) Sinks are higher importance bugs. + // (2) NoReturnFunctionChecker uses sink nodes to represent paths ending + // with __noreturn functions such as assert() or exit(). We choose not + // to report leaks on such paths. + BugType LeakBug{this, "Memory leak", categories::MemoryError, + /*SuppressOnSink=*/true}; +}; + +BUGTYPE_PROVIDER(UseFree, "Use-after-free") +BUGTYPE_PROVIDER(BadFree, "Bad free") +BUGTYPE_PROVIDER(FreeAlloca, "Free 'alloca()'") +BUGTYPE_PROVIDER(MismatchedDealloc, "Bad deallocator") +BUGTYPE_PROVIDER(OffsetFree, "Offset free") +BUGTYPE_PROVIDER(UseZeroAllocated, "Use of zero allocated") + +template <typename... BT_PROVIDERS> +struct DynMemFrontend : virtual public CheckerFrontend, public BT_PROVIDERS... { + template <typename T> const T *getAs() const { + if constexpr (std::is_same_v<T, CheckerFrontend>) + return static_cast<const T *>(this); + if constexpr ((std::is_same_v<T, BT_PROVIDERS> || ...)) + return static_cast<const T *>(this); + return nullptr; + } +}; + +//===----------------------------------------------------------------------===// +// Definition of the MallocChecker class. +//===----------------------------------------------------------------------===// class MallocChecker : public Checker<check::DeadSymbols, check::PointerEscape, @@ -355,26 +399,29 @@ class MallocChecker bool ShouldRegisterNoOwnershipChangeVisitor = false; - /// Many checkers are essentially built into this one, so enabling them will - /// make MallocChecker perform additional modeling and reporting. - enum CheckKind { - /// When a subchecker is enabled but MallocChecker isn't, model memory - /// management but do not emit warnings emitted with MallocChecker only - /// enabled. - CK_MallocChecker, - CK_NewDeleteChecker, - CK_NewDeleteLeaksChecker, - CK_MismatchedDeallocatorChecker, - CK_InnerPointerChecker, - CK_TaintedAllocChecker, - CK_NumCheckKinds - }; + // This checker family implements many bug types and frontends, and several + // bug types are shared between multiple frontends, so most of the frontends + // are declared with the helper class DynMemFrontend. + // FIXME: There is no clear reason for separating NewDelete vs NewDeleteLeaks + // while e.g. MallocChecker covers both non-leak and leak bugs together. It + // would be nice to redraw the boundaries between the frontends in a more + // logical way. + DynMemFrontend<DoubleFree, Leak, UseFree, BadFree, FreeAlloca, OffsetFree, + UseZeroAllocated> + MallocChecker; + DynMemFrontend<DoubleFree, DoubleDelete, UseFree, BadFree, OffsetFree, + UseZeroAllocated> + NewDeleteChecker; + DynMemFrontend<Leak> NewDeleteLeaksChecker; + DynMemFrontend<FreeAlloca, MismatchedDealloc> MismatchedDeallocatorChecker; + DynMemFrontend<UseFree> InnerPointerChecker; + // This last frontend is associated with a single bug type which is not used + // elsewhere and has a different bug category, so it's declared separately. + CheckerFrontendWithBugType TaintedAllocChecker{"Tainted Memory Allocation", + categories::TaintedData}; using LeakInfo = std::pair<const ExplodedNode *, const MemRegion *>; - bool ChecksEnabled[CK_NumCheckKinds] = {false}; - CheckerNameRef CheckNames[CK_NumCheckKinds]; - void checkPreCall(const CallEvent &Call, CheckerContext &C) const; void checkPostCall(const CallEvent &Call, CheckerContext &C) const; bool evalCall(const CallEvent &Call, CheckerContext &C) const; @@ -402,16 +449,19 @@ class MallocChecker const char *NL, const char *Sep) const override; private: - mutable std::unique_ptr<BugType> BT_DoubleFree[CK_NumCheckKinds]; - mutable std::unique_ptr<BugType> BT_DoubleDelete; - mutable std::unique_ptr<BugType> BT_Leak[CK_NumCheckKinds]; - mutable std::unique_ptr<BugType> BT_UseFree[CK_NumCheckKinds]; - mutable std::unique_ptr<BugType> BT_BadFree[CK_NumCheckKinds]; - mutable std::unique_ptr<BugType> BT_FreeAlloca[CK_NumCheckKinds]; - mutable std::unique_ptr<BugType> BT_MismatchedDealloc; - mutable std::unique_ptr<BugType> BT_OffsetFree[CK_NumCheckKinds]; - mutable std::unique_ptr<BugType> BT_UseZerroAllocated[CK_NumCheckKinds]; - mutable std::unique_ptr<BugType> BT_TaintedAlloc; + /// Helper method to handle the cases where there is no associated frontend + /// (just exit early) or the associated frontend is disabled (sink the + /// execution path and and then exit early). Intended to be called as + /// if (handleNullOrDisabled(Frontend, C)) + /// return; + static bool handleNullOrDisabled(const CheckerFrontend *F, + CheckerContext &C) { + if (F && F->isEnabled()) + return false; + if (F) + C.addSink(); + return true; + } #define CHECK_FN(NAME) \ void NAME(ProgramStateRef State, const CallEvent &Call, CheckerContext &C) \ @@ -439,7 +489,7 @@ class MallocChecker CheckerContext &C, bool ShouldFreeOnFail) const; using CheckFn = - std::function<void(const MallocChecker *, ProgramStateRef State, + std::function<void(const class MallocChecker *, ProgramStateRef State, const CallEvent &Call, CheckerContext &C)>; const CallDescriptionMap<CheckFn> PreFnMap{ @@ -773,14 +823,16 @@ class MallocChecker void checkEscapeOnReturn(const ReturnStmt *S, CheckerContext &C) const; ///@{ - /// Tells if a given family/call/symbol is tracked by the current checker. - /// Sets CheckKind to the kind of the checker responsible for this - /// family/call/symbol. - std::optional<CheckKind> getCheckIfTracked(AllocationFamily Family, - bool IsALeakCheck = false) const; - - std::optional<CheckKind> getCheckIfTracked(CheckerContext &C, SymbolRef Sym, - bool IsALeakCheck = false) const; + /// Returns a pointer to the checker frontend corresponding to the given + /// family or symbol. The template argument T may be either CheckerFamily or + /// a BUGTYPE_PROVIDER class; in the latter case the query is restricted to + /// frontends that descend from that PROVIDER class (i.e. can emit that bug + /// type). Note that this may return a frontend which is disabled. + template <class T> + const T *getRelevantFrontendAs(AllocationFamily Family) const; + + template <class T> + const T *getRelevantFrontendAs(CheckerContext &C, SymbolRef Sym) const; ///@} static bool SummarizeValue(raw_ostream &os, SVal V); static bool SummarizeRegion(ProgramStateRef State, raw_ostream &os, @@ -1558,7 +1610,7 @@ void MallocChecker::checkOwnershipAttr(ProgramStateRef State, if (!FD) return; if (ShouldIncludeOwnershipAnnotatedFunctions || - ChecksEnabled[CK_MismatchedDeallocatorChecker]) { + MismatchedDeallocatorChecker.isEnabled()) { // Check all the attributes, if there are any. // There can be multiple of these attributes. if (FD->hasAttrs()) @@ -1883,11 +1935,8 @@ void MallocChecker::reportTaintBug(StringRef Msg, ProgramStateRef State, llvm::ArrayRef<SymbolRef> TaintedSyms, AllocationFamily Family) const { if (ExplodedNode *N = C.generateNonFatalErrorNode(State, this)) { - if (!BT_TaintedAlloc) - BT_TaintedAlloc.reset(new BugType(CheckNames[CK_TaintedAllocChecker], - "Tainted Memory Allocation", - categories::TaintedData)); - auto R = std::make_unique<PathSensitiveBugReport>(*BT_TaintedAlloc, Msg, N); + auto R = + std::make_unique<PathSensitiveBugReport>(TaintedAllocChecker, Msg, N); for (const auto *TaintedSym : TaintedSyms) { R->markInteresting(TaintedSym); } @@ -1898,7 +1947,7 @@ void MallocChecker::reportTaintBug(StringRef Msg, ProgramStateRef State, void MallocChecker::checkTaintedness(CheckerContext &C, const CallEvent &Call, const SVal SizeSVal, ProgramStateRef State, AllocationFamily Family) const { - if (!ChecksEnabled[CK_TaintedAllocChecker]) + if (!TaintedAllocChecker.isEnabled()) return; std::vector<SymbolRef> TaintedSyms = taint::getTaintedSymbols(State, SizeSVal); @@ -2348,53 +2397,47 @@ MallocChecker::FreeMemAux(CheckerContext &C, const Expr *ArgExpr, RefState::getReleased(Family, ParentExpr)); } -std::optional<MallocChecker::CheckKind> -MallocChecker::getCheckIfTracked(AllocationFamily Family, - bool IsALeakCheck) const { +template <class T> +const T *MallocChecker::getRelevantFrontendAs(AllocationFamily Family) const { switch (Family.Kind) { case AF_Malloc: case AF_Alloca: case AF_Custom: - case AF_IfNameIndex: { - if (ChecksEnabled[CK_MallocChecker]) - return CK_MallocChecker; - return std::nullopt; - } + case AF_IfNameIndex: + return MallocChecker.getAs<T>(); case AF_CXXNew: case AF_CXXNewArray: { - if (IsALeakCheck) { - if (ChecksEnabled[CK_NewDeleteLeaksChecker]) - return CK_NewDeleteLeaksChecker; + const T *ND = NewDeleteChecker.getAs<T>(); + const T *NDL = NewDeleteLeaksChecker.getAs<T>(); + // Bugs corresponding to C++ new/delete allocations are split between these + // two frontends. + if constexpr (std::is_same_v<T, CheckerFrontend>) { + assert(ND && NDL && "Casting to CheckerFrontend always succeeds"); + // Prefer NewDelete unless it's disabled and NewDeleteLeaks is enabled. + return (!ND->isEnabled() && NDL->isEnabled()) ? NDL : ND; } - else { - if (ChecksEnabled[CK_NewDeleteChecker]) - return CK_NewDeleteChecker; - } - return std::nullopt; - } - case AF_InnerBuffer: { - if (ChecksEnabled[CK_InnerPointerChecker]) - return CK_InnerPointerChecker; - return std::nullopt; + assert(!(ND && NDL) && + "NewDelete and NewDeleteLeaks must not share a bug type"); + return ND ? ND : NDL; } - case AF_None: { + case AF_InnerBuffer: + return InnerPointerChecker.getAs<T>(); + case AF_None: assert(false && "no family"); - return std::nullopt; - } + return nullptr; } assert(false && "unhandled family"); - return std::nullopt; + return nullptr; } - -std::optional<MallocChecker::CheckKind> -MallocChecker::getCheckIfTracked(CheckerContext &C, SymbolRef Sym, - bool IsALeakCheck) const { +template <class T> +const T *MallocChecker::getRelevantFrontendAs(CheckerContext &C, + SymbolRef Sym) const { if (C.getState()->contains<ReallocSizeZeroSymbols>(Sym)) - return CK_MallocChecker; + return MallocChecker.getAs<T>(); const RefState *RS = C.getState()->get<RegionState>(Sym); assert(RS); - return getCheckIfTracked(RS->getAllocationFamily(), IsALeakCheck); + return getRelevantFrontendAs<T>(RS->getAllocationFamily()); } bool MallocChecker::SummarizeValue(raw_ostream &os, SVal V) { @@ -2490,21 +2533,11 @@ void MallocChecker::HandleNonHeapDealloc(CheckerContext &C, SVal ArgVal, SourceRange Range, const Expr *DeallocExpr, AllocationFamily Family) const { - - if (!ChecksEnabled[CK_MallocChecker] && !ChecksEnabled[CK_NewDeleteChecker]) { - C.addSink(); - return; - } - - std::optional<MallocChecker::CheckKind> CheckKind = getCheckIfTracked(Family); - if (!CheckKind) + const BadFree *Frontend = getRelevantFrontendAs<BadFree>(Family); + if (handleNullOrDisabled(Frontend, C)) return; if (ExplodedNode *N = C.generateErrorNode()) { - if (!BT_BadFree[*CheckKind]) - BT_BadFree[*CheckKind].reset(new BugType( - CheckNames[*CheckKind], "Bad free", categories::MemoryError)); - SmallString<100> buf; llvm::raw_svector_ostream os(buf); @@ -2526,7 +2559,7 @@ void MallocChecker::HandleNonHeapDealloc(CheckerContext &C, SVal ArgVal, printExpectedAllocName(os, Family); - auto R = std::make_unique<PathSensitiveBugReport>(*BT_BadFree[*CheckKind], + auto R = std::make_unique<PathSensitiveBugReport>(Frontend->BadFreeBug, os.str(), N); R->markInteresting(MR); R->addRange(Range); @@ -2536,25 +2569,20 @@ void MallocChecker::HandleNonHeapDealloc(CheckerContext &C, SVal ArgVal, void MallocChecker::HandleFreeAlloca(CheckerContext &C, SVal ArgVal, SourceRange Range) const { + const FreeAlloca *Frontend; - std::optional<MallocChecker::CheckKind> CheckKind; - - if (ChecksEnabled[CK_MallocChecker]) - CheckKind = CK_MallocChecker; - else if (ChecksEnabled[CK_MismatchedDeallocatorChecker]) - CheckKind = CK_MismatchedDeallocatorChecker; + if (MallocChecker.isEnabled()) + Frontend = &MallocChecker; + else if (MismatchedDeallocatorChecker.isEnabled()) + Frontend = &MismatchedDeallocatorChecker; else { C.addSink(); return; } if (ExplodedNode *N = C.generateErrorNode()) { - if (!BT_FreeAlloca[*CheckKind]) - BT_FreeAlloca[*CheckKind].reset(new BugType( - CheckNames[*CheckKind], "Free 'alloca()'", categories::MemoryError)); - auto R = std::make_unique<PathSensitiveBugReport>( - *BT_FreeAlloca[*CheckKind], + Frontend->FreeAllocaBug, "Memory allocated by 'alloca()' should not be deallocated", N); R->markInteresting(ArgVal.getAsRegion()); R->addRange(Range); @@ -2567,18 +2595,12 @@ void MallocChecker::HandleMismatchedDealloc(CheckerContext &C, const Expr *DeallocExpr, const RefState *RS, SymbolRef Sym, bool OwnershipTransferred) const { - - if (!ChecksEnabled[CK_MismatchedDeallocatorChecker]) { + if (!MismatchedDeallocatorChecker.isEnabled()) { C.addSink(); return; } if (ExplodedNode *N = C.generateErrorNode()) { - if (!BT_MismatchedDealloc) - BT_MismatchedDealloc.reset( - new BugType(CheckNames[CK_MismatchedDeallocatorChecker], - "Bad deallocator", categories::MemoryError)); - SmallString<100> buf; llvm::raw_svector_ostream os(buf); @@ -2612,8 +2634,8 @@ void MallocChecker::HandleMismatchedDealloc(CheckerContext &C, printOwnershipTakesList(os, C, DeallocExpr); } - auto R = std::make_unique<PathSensitiveBugReport>(*BT_MismatchedDealloc, - os.str(), N); + auto R = std::make_unique<PathSensitiveBugReport>( + MismatchedDeallocatorChecker.MismatchedDeallocBug, os.str(), N); R->markInteresting(Sym); R->addRange(Range); R->addVisitor<MallocBugVisitor>(Sym); @@ -2625,24 +2647,14 @@ void MallocChecker::HandleOffsetFree(CheckerContext &C, SVal ArgVal, SourceRange Range, const Expr *DeallocExpr, AllocationFamily Family, const Expr *AllocExpr) const { - - if (!ChecksEnabled[CK_MallocChecker] && !ChecksEnabled[CK_NewDeleteChecker]) { - C.addSink(); - return; - } - - std::optional<MallocChecker::CheckKind> CheckKind = getCheckIfTracked(Family); - if (!CheckKind) + const OffsetFree *Frontend = getRelevantFrontendAs<OffsetFree>(Family); + if (handleNullOrDisabled(Frontend, C)) return; ExplodedNode *N = C.generateErrorNode(); if (!N) return; - if (!BT_OffsetFree[*CheckKind]) - BT_OffsetFree[*CheckKind].reset(new BugType( - CheckNames[*CheckKind], "Offset free", categories::MemoryError)); - SmallString<100> buf; llvm::raw_svector_ostream os(buf); SmallString<20> AllocNameBuf; @@ -2672,7 +2684,7 @@ void MallocChecker::HandleOffsetFree(CheckerContext &C, SVal ArgVal, else os << "allocated memory"; - auto R = std::make_unique<PathSensitiveBugReport>(*BT_OffsetFree[*CheckKind], + auto R = std::make_unique<PathSensitiveBugReport>(Frontend->OffsetFreeBug, os.str(), N); R->markInteresting(MR->getBaseRegion()); R->addRange(Range); @@ -2681,27 +2693,16 @@ void MallocChecker::HandleOffsetFree(CheckerContext &C, SVal ArgVal, void MallocChecker::HandleUseAfterFree(CheckerContext &C, SourceRange Range, SymbolRef Sym) const { - - if (!ChecksEnabled[CK_MallocChecker] && !ChecksEnabled[CK_NewDeleteChecker] && - !ChecksEnabled[CK_InnerPointerChecker]) { - C.addSink(); - return; - } - - std::optional<MallocChecker::CheckKind> CheckKind = getCheckIfTracked(C, Sym); - if (!CheckKind) + const UseFree *Frontend = getRelevantFrontendAs<UseFree>(C, Sym); + if (handleNullOrDisabled(Frontend, C)) return; if (ExplodedNode *N = C.generateErrorNode()) { - if (!BT_UseFree[*CheckKind]) - BT_UseFree[*CheckKind].reset(new BugType( - CheckNames[*CheckKind], "Use-after-free", categories::MemoryError)); - AllocationFamily AF = C.getState()->get<RegionState>(Sym)->getAllocationFamily(); auto R = std::make_unique<PathSensitiveBugReport>( - *BT_UseFree[*CheckKind], + Frontend->UseFreeBug, AF.Kind == AF_InnerBuffer ? "Inner pointer of container used after re/deallocation" : "Use of memory after it is freed", @@ -2721,23 +2722,13 @@ void MallocChecker::HandleUseAfterFree(CheckerContext &C, SourceRange Range, void MallocChecker::HandleDoubleFree(CheckerContext &C, SourceRange Range, bool Released, SymbolRef Sym, SymbolRef PrevSym) const { - - if (!ChecksEnabled[CK_MallocChecker] && !ChecksEnabled[CK_NewDeleteChecker]) { - C.addSink(); - return; - } - - std::optional<MallocChecker::CheckKind> CheckKind = getCheckIfTracked(C, Sym); - if (!CheckKind) + const DoubleFree *Frontend = getRelevantFrontendAs<DoubleFree>(C, Sym); + if (handleNullOrDisabled(Frontend, C)) return; if (ExplodedNode *N = C.generateErrorNode()) { - if (!BT_DoubleFree[*CheckKind]) - BT_DoubleFree[*CheckKind].reset(new BugType( - CheckNames[*CheckKind], "Double free", categories::MemoryError)); - auto R = std::make_unique<PathSensitiveBugReport>( - *BT_DoubleFree[*CheckKind], + Frontend->DoubleFreeBug, (Released ? "Attempt to free released memory" : "Attempt to free non-owned memory"), N); @@ -2751,24 +2742,14 @@ void Ma... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/147080 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
