Skip to content

[analyzer] Conversion to CheckerFamily: MallocChecker #147080

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 10 commits into from
Jul 8, 2025

Conversation

NagyDonat
Copy link
Contributor

This commit converts MallocChecker to the new checker family framework that was introduced in the recent commit
6833076 -- and gets rid of some awkward unintended interactions between the checker frontends.

This commit converts MallocChecker to the new checker family framework
that was introduced in the recent commit
6833076 -- and gets rid of some
awkward unintended interactions between the checker frontends.
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:static analyzer labels Jul 4, 2025
@llvmbot
Copy link
Member

llvmbot commented Jul 4, 2025

@llvm/pr-subscribers-clang-static-analyzer-1

Author: Donát Nagy (NagyDonat)

Changes

This commit converts MallocChecker to the new checker family framework that was introduced in the recent commit
6833076 -- 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 --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]

@llvmbot
Copy link
Member

llvmbot commented Jul 4, 2025

@llvm/pr-subscribers-clang

Author: Donát Nagy (NagyDonat)

Changes

This commit converts MallocChecker to the new checker family framework that was introduced in the recent commit
6833076 -- 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 --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]

Comment on lines 2684 to 2689

if (!ChecksEnabled[CK_MallocChecker] && !ChecksEnabled[CK_NewDeleteChecker] &&
!ChecksEnabled[CK_InnerPointerChecker]) {
C.addSink();
return;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure that the intention behind this block (and analogous blocks) is that it tries to create a sink node when the checker part that would create a bug report is disabled -- but this isn't the actual behavior of this code fragment.

As a concrete example, in the test file new.cpp there was a symbol (Sym) allocated with operator new, NewDeleteChecker was disabled, but MallocChecker (the checker part unix.Malloc) was enabled. In this situation the revision before this PR:

  • (1) didn't create a sink because ChecksEnabled[CK_MallocChecker] was true;
  • (2) returned a bit later when getCheckIfTracked returned std::nullopt.

After this PR getRelevantFrontendAs<> will find the frontend that corresponds to the UseFree bug type and the given Symbol (that is, NewDeleteChecker) and handleNullOrDisabled will recognize that there is a relevant frontend, but it's disabled, so it will create a sink before the early return.

@NagyDonat NagyDonat changed the title [analyzer] Connversion to CheckerFamily: MallocChecker [analyzer] Conversion to CheckerFamily: MallocChecker Jul 4, 2025
Comment on lines +330 to +333
// See also test-member-invalidation.cpp which validates that calling an
// unknown destructor invalidates the members of an object. This behavior
// cannot be tested in this file because here `MallocChecker.cpp` sinks
// execution paths that refer to members of a deleted object.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact I think it would be better to just remove this testcase, because the original claim that this tests "Invalidate Region even in case of default description" is a big lie – the destructor of this class is a declared but not defined opaque function, so the test just validates that the default execution of an opaque (non-const) method invalidates the members of this. This is a very basic behavior of the analyzer engine, so I don't think that it deserves an explicit test, especially in a case where it is (often) superseded by the more accurate modeling done by MallocChecker.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will remove this test in a follow-up change.

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];
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's say goodbye to Zerro, the younger brother of Zorro... 😅

@@ -2812,20 +2782,11 @@ void MallocChecker::HandleFunctionPtrFree(CheckerContext &C, SVal ArgVal,
SourceRange Range,
const Expr *FreeExpr,
AllocationFamily Family) const {
if (!ChecksEnabled[CK_MallocChecker]) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a functional change -- calling delete on a function pointer will generate a bug report even if MallocChecker is not enabled.

NagyDonat added 5 commits July 7, 2025 17:30
...because it would crash by dereferencing nullopt.
`handleNullOrDisabled` creates a sink when there is a checker frontend that _would report_ the situation but it's disabled. However, this is inappropriate for `Leak`s because that bug report is a non-fatal error so it should not be replaced by a sink.
I hope that I can address these FIXMEs in a follow-up commit.
@NagyDonat
Copy link
Contributor Author

@gamesh411 I applied all the changes that we discussed in the interactive review. Please accept the PR formally (or add any suggestions that you realized since then).

I'll probably merge this PR on 9th of July if there is no other feedback.

Copy link

github-actions bot commented Jul 8, 2025

✅ With the latest revision this PR passed the C/C++ code formatter.

Copy link
Contributor

@steakhal steakhal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the reviews already done.
I think as I skimmed through, I didn't have any major concerns, maybe a couple of questions.

Comment on lines 374 to 377
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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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);
if constexpr (std::is_same_v<T, CheckerFrontend> || (std::is_same_v<T, BT_PROVIDERS> || ...))
return static_cast<const T *>(this);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 6f4cc0c

@NagyDonat NagyDonat merged commit 39bc052 into llvm:main Jul 8, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:static analyzer clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants