[analyzer] MallocChecker refactoring of calls checkers

[devnexen]

Summary

• Splitting some by family to reduce the list of function identifiers.

[devnexen]

Splitting from https://reviews.llvm.org/D45149 as requested.

[NoQ]

@xazax.hun: do you think the approach you took in the Valist checker is applicable here? Did you like how it ended up working? Cause i'd love to see CallDescription and initializer lists used for readability here. And i'd love to see branches replaced with map lookups. And i'm not sure if anybody has written code that has all the stuff that i'd love to see.

[xazax.hun]

In D45458#1062342, @NoQ wrote:

@xazax.hun: do you think the approach you took in the Valist checker is applicable here? Did you like how it ended up working? Cause i'd love to see CallDescription and initializer lists used for readability here. And i'd love to see branches replaced with map lookups. And i'm not sure if anybody has written code that has all the stuff that i'd love to see.

Overall the CallDescription interface works really well if you do not need anything more advanced, e.g. supporting overloads. I can imagine that its current implementation is slightly less efficient since the lazy initialization logic of the identifiers are repeated for every identifier but that also makes it less bug-prone (do not have the implicit assumption that a set of identifiers should always exist together).

It would be great to extend the interface in the future, but one of the reasons I was reluctant to do so yet is performance. E.g.: having a simple check that a checker is interested only in global C functions is better than having this check repeated in every call description object in an array, or making it possible to do a StringSwitch like optimization with multiple statically known CallDescription objects would also be awesome.

But I think most of the time these tests would not be used on the hot paths anyways, so as long its current expressive power is sufficient, I prefer code that is utilizing this tool. It tends to make the code slightly shorter.

lib/StaticAnalyzer/Checkers/MallocChecker.cpp

class MallocChecker : public Checker<check::DeadSymbols,
                                      check::PreStmt<CXXDeleteExpr>,
                                      check::PostStmt<BlockExpr>,
                                      check::PostObjCMessage,
                                      check::Location,
                                      eval::Assume>
 {
 public:
   MallocChecker()
-      : II_alloca(nullptr), II_win_alloca(nullptr), II_malloc(nullptr),
-        II_free(nullptr), II_realloc(nullptr), II_calloc(nullptr),
-        II_valloc(nullptr), II_reallocf(nullptr), II_strndup(nullptr),
-        II_strdup(nullptr), II_win_strdup(nullptr), II_kmalloc(nullptr),
-        II_if_nameindex(nullptr), II_if_freenameindex(nullptr),
-        II_wcsdup(nullptr), II_win_wcsdup(nullptr), II_g_malloc(nullptr),
-        II_g_malloc0(nullptr), II_g_realloc(nullptr), II_g_try_malloc(nullptr), 
-        II_g_try_malloc0(nullptr), II_g_try_realloc(nullptr), 
-        II_g_free(nullptr), II_g_memdup(nullptr), II_g_malloc_n(nullptr), 
-        II_g_malloc0_n(nullptr), II_g_realloc_n(nullptr), 
-        II_g_try_malloc_n(nullptr), II_g_try_malloc0_n(nullptr), 
-        II_g_try_realloc_n(nullptr) {}
+      : II_kmalloc(nullptr), II_if_nameindex(nullptr),
+        II_if_freenameindex(nullptr), II_valloc(nullptr),
+        II_realloc(nullptr), II_calloc(nullptr),
+        II_reallocarray(nullptr), II_recallocarray(nullptr),
+        II_strndup(nullptr), II_g_memdup(nullptr),
+        II_g_malloc0(nullptr), II_g_try_malloc0(nullptr),
+        II_g_malloc0_n(nullptr), II_g_try_malloc0_n(nullptr),
+        II_reallocf(nullptr), II_malloc(nullptr) {}
 
   /// In pessimistic mode, the checker assumes that it does not know which
   /// functions might free the memory.
   enum CheckKind {
     CK_MallocChecker,
     CK_NewDeleteChecker,
     CK_NewDeleteLeaksChecker,
     CK_MismatchedDeallocatorChecker,
private:
   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 IdentifierInfo *II_alloca, *II_win_alloca, *II_malloc, *II_free,
-                         *II_realloc, *II_calloc, *II_valloc, *II_reallocf,
-                         *II_strndup, *II_strdup, *II_win_strdup, *II_kmalloc,
-                         *II_if_nameindex, *II_if_freenameindex, *II_wcsdup,
-                         *II_win_wcsdup, *II_g_malloc, *II_g_malloc0, 
-                         *II_g_realloc, *II_g_try_malloc, *II_g_try_malloc0, 
-                         *II_g_try_realloc, *II_g_free, *II_g_memdup, 
-                         *II_g_malloc_n, *II_g_malloc0_n, *II_g_realloc_n, 
-                         *II_g_try_malloc_n, *II_g_try_malloc0_n, 
-                         *II_g_try_realloc_n;
   mutable Optional<uint64_t> KernelZeroFlagVal;
 
+  mutable llvm::SmallSet<IdentifierInfo *, 20> AllocFunctions;
+  mutable llvm::SmallSet<IdentifierInfo *, 4> FreeFunctions;
+
+  mutable llvm::SmallSet<IdentifierInfo *, 2> AllocaFunctions;
+  mutable llvm::SmallSet<IdentifierInfo *, 3> SimpleMallocFunctions;
+
+  mutable llvm::SmallSet<IdentifierInfo *, 4> NZeroMallocFunctions;
+
+  mutable llvm::SmallSet<IdentifierInfo *, 3> NReallocFunctions;
+  mutable llvm::SmallSet<IdentifierInfo *, 3> ReallocFunctions;
+
+  mutable llvm::SmallSet<IdentifierInfo *, 4> StrdupFunctions;
+
+  mutable IdentifierInfo *II_kmalloc, *II_if_nameindex, *II_if_freenameindex,
+                         *II_valloc, *II_realloc, *II_calloc, *II_reallocarray,
+                         *II_recallocarray, *II_strndup, *II_g_memdup,
+                         *II_g_malloc0, *II_g_try_malloc0, *II_g_malloc0_n,
+                         *II_g_try_malloc0_n, *II_reallocf, *II_malloc;
+
   void initIdentifierInfo(ASTContext &C) const;
 
   /// \brief Determine family of a deallocation expression.
   AllocationFamily getAllocationFamily(CheckerContext &C, const Stmt *S) const;
 
   /// \brief Print names of allocators and deallocators.
   ///
   /// \returns true on success.
ProgramStateRef FreeMemAux(CheckerContext &C, const Expr *Arg,
                              ProgramStateRef State,
                              bool Hold,
                              bool &ReleasedAllocated,
                              bool ReturnsNullOnFailure = false) const;
 
   ProgramStateRef ReallocMemAux(CheckerContext &C, const CallExpr *CE,
                                 bool FreesMemOnFailure,
                                 ProgramStateRef State, 
-                                bool SuffixWithN = false) const;
+                                bool SuffixWithN = false,
+                                bool ZeroVal = false) const;
   static SVal evalMulForBufferSize(CheckerContext &C, const Expr *Blocks,
                                    const Expr *BlockBytes);
   static ProgramStateRef CallocMem(CheckerContext &C, const CallExpr *CE,
                                    ProgramStateRef State);
 
   ///\brief Check if the memory associated with this symbol was released.
   bool isReleased(SymbolRef Sym, CheckerContext &C) const;
 
public:
   bool VisitSymbol(SymbolRef sym) override {
     state = state->remove<RegionState>(sym);
     return true;
   }
 };
 } // end anonymous namespace
 
 void MallocChecker::initIdentifierInfo(ASTContext &Ctx) const {
-  if (II_malloc)
+  if (!AllocFunctions.empty())
     return;
-  II_alloca = &Ctx.Idents.get("alloca");
-  II_malloc = &Ctx.Idents.get("malloc");
-  II_free = &Ctx.Idents.get("free");
-  II_realloc = &Ctx.Idents.get("realloc");
-  II_reallocf = &Ctx.Idents.get("reallocf");
-  II_calloc = &Ctx.Idents.get("calloc");
-  II_valloc = &Ctx.Idents.get("valloc");
-  II_strdup = &Ctx.Idents.get("strdup");
-  II_strndup = &Ctx.Idents.get("strndup");
-  II_wcsdup = &Ctx.Idents.get("wcsdup");
-  II_kmalloc = &Ctx.Idents.get("kmalloc");
-  II_if_nameindex = &Ctx.Idents.get("if_nameindex");
-  II_if_freenameindex = &Ctx.Idents.get("if_freenameindex");
+  AllocFunctions.insert(&Ctx.Idents.get("malloc"));
+  AllocFunctions.insert(&Ctx.Idents.get("realloc"));
+  AllocFunctions.insert(&Ctx.Idents.get("reallocf"));
+  AllocFunctions.insert(&Ctx.Idents.get("calloc"));
+  AllocFunctions.insert(&Ctx.Idents.get("valloc"));
+  AllocFunctions.insert(&Ctx.Idents.get("kmalloc"));
+
+  AllocaFunctions.insert(&Ctx.Idents.get("alloca"));
+  AllocaFunctions.insert(&Ctx.Idents.get("_alloca"));
+
+  SimpleMallocFunctions.insert(&Ctx.Idents.get("malloc"));
+  SimpleMallocFunctions.insert(&Ctx.Idents.get("g_malloc"));
+  SimpleMallocFunctions.insert(&Ctx.Idents.get("g_try_malloc"));
+
+  NZeroMallocFunctions.insert(&Ctx.Idents.get("g_malloc_n"));
+  NZeroMallocFunctions.insert(&Ctx.Idents.get("g_try_malloc_n"));
+  NZeroMallocFunctions.insert(&Ctx.Idents.get("g_malloc0_n"));
+  NZeroMallocFunctions.insert(&Ctx.Idents.get("g_try_malloc0_n"));
+
+  NReallocFunctions.insert(&Ctx.Idents.get("g_realloc_n"));
+  NReallocFunctions.insert(&Ctx.Idents.get("g_try_realloc_n"));
+  NReallocFunctions.insert(&Ctx.Idents.get("reallocarray"));
+
+  ReallocFunctions.insert(&Ctx.Idents.get("realloc"));
+  ReallocFunctions.insert(&Ctx.Idents.get("g_realloc"));
+  ReallocFunctions.insert(&Ctx.Idents.get("g_try_realloc"));
+
+  StrdupFunctions.insert(&Ctx.Idents.get("strdup"));
+  StrdupFunctions.insert(&Ctx.Idents.get("wcsdup"));
+  StrdupFunctions.insert(&Ctx.Idents.get("_strdup"));
+  StrdupFunctions.insert(&Ctx.Idents.get("_wcsdup"));
+
+  FreeFunctions.insert(&Ctx.Idents.get("free"));
 
   //MSVC uses `_`-prefixed instead, so we check for them too.
-  II_win_strdup = &Ctx.Idents.get("_strdup");
-  II_win_wcsdup = &Ctx.Idents.get("_wcsdup");
-  II_win_alloca = &Ctx.Idents.get("_alloca");
+
+  AllocaFunctions.insert(&Ctx.Idents.get("_alloca"));
 
   // Glib
-  II_g_malloc = &Ctx.Idents.get("g_malloc");
+  AllocFunctions.insert(&Ctx.Idents.get("g_malloc"));
+  AllocFunctions.insert(&Ctx.Idents.get("g_malloc0"));
+  AllocFunctions.insert(&Ctx.Idents.get("g_realloc"));
+  AllocFunctions.insert(&Ctx.Idents.get("g_try_malloc"));
+  AllocFunctions.insert(&Ctx.Idents.get("g_try_malloc0"));
+  AllocFunctions.insert(&Ctx.Idents.get("g_try_realloc"));
+  AllocFunctions.insert(&Ctx.Idents.get("g_memdup"));
+  AllocFunctions.insert(&Ctx.Idents.get("g_malloc_n"));
+  AllocFunctions.insert(&Ctx.Idents.get("g_malloc0_n"));
+  AllocFunctions.insert(&Ctx.Idents.get("g_realloc_n"));
+  AllocFunctions.insert(&Ctx.Idents.get("g_try_malloc_n"));
+  AllocFunctions.insert(&Ctx.Idents.get("g_try_malloc0_n"));
+  AllocFunctions.insert(&Ctx.Idents.get("g_try_realloc_n"));
+
+  FreeFunctions.insert(&Ctx.Idents.get("g_free"));
+
+  // BSD
+  AllocFunctions.insert(&Ctx.Idents.get("reallocarray"));
+  AllocFunctions.insert(&Ctx.Idents.get("recallocarray"));
+
+  FreeFunctions.insert(&Ctx.Idents.get("freezero"));
+
+  II_kmalloc = &Ctx.Idents.get("kmalloc");
+  II_valloc = &Ctx.Idents.get("valloc");
+  II_if_nameindex = &Ctx.Idents.get("if_nameindex");
+  II_if_freenameindex = &Ctx.Idents.get("if_freenameindex");
+  II_realloc = &Ctx.Idents.get("realloc");
+  II_calloc = &Ctx.Idents.get("calloc");
+  II_reallocarray = &Ctx.Idents.get("reallocarray");
+  II_recallocarray = &Ctx.Idents.get("recallocarray");
+  II_reallocf = &Ctx.Idents.get("reallocf");
   II_g_malloc0 = &Ctx.Idents.get("g_malloc0");
-  II_g_realloc = &Ctx.Idents.get("g_realloc");
-  II_g_try_malloc = &Ctx.Idents.get("g_try_malloc");
   II_g_try_malloc0 = &Ctx.Idents.get("g_try_malloc0");
-  II_g_try_realloc = &Ctx.Idents.get("g_try_realloc");
-  II_g_free = &Ctx.Idents.get("g_free");
-  II_g_memdup = &Ctx.Idents.get("g_memdup");
-  II_g_malloc_n = &Ctx.Idents.get("g_malloc_n");
   II_g_malloc0_n = &Ctx.Idents.get("g_malloc0_n");
-  II_g_realloc_n = &Ctx.Idents.get("g_realloc_n");
-  II_g_try_malloc_n = &Ctx.Idents.get("g_try_malloc_n");
   II_g_try_malloc0_n = &Ctx.Idents.get("g_try_malloc0_n");
-  II_g_try_realloc_n = &Ctx.Idents.get("g_try_realloc_n");
+  II_strndup = &Ctx.Idents.get("strndup");
+  II_g_memdup = &Ctx.Idents.get("g_memdup");
+  II_malloc = &Ctx.Idents.get("malloc");
+
 }
 
 bool MallocChecker::isMemFunction(const FunctionDecl *FD, ASTContext &C) const {
   if (isCMemFunction(FD, C, AF_Malloc, MemoryOperationKind::MOK_Any))
     return true;
 
   if (isCMemFunction(FD, C, AF_IfNameIndex, MemoryOperationKind::MOK_Any))
     return true;
bool MallocChecker::isCMemFunction(const FunctionDecl *FD,
   bool CheckAlloc = (MemKind == MemoryOperationKind::MOK_Any ||
                      MemKind == MemoryOperationKind::MOK_Allocate);
 
   if (FD->getKind() == Decl::Function) {
     const IdentifierInfo *FunI = FD->getIdentifier();
     initIdentifierInfo(C);
 
     if (Family == AF_Malloc && CheckFree) {
-      if (FunI == II_free || FunI == II_realloc || FunI == II_reallocf || 
-          FunI == II_g_free)
+      if (FreeFunctions.count(FunI) == 1 ||
+          FunI == II_realloc || FunI == II_reallocf)
         return true;
     }
 
     if (Family == AF_Malloc && CheckAlloc) {
-      if (FunI == II_malloc || FunI == II_realloc || FunI == II_reallocf ||
-          FunI == II_calloc || FunI == II_valloc || FunI == II_strdup ||
-          FunI == II_win_strdup || FunI == II_strndup || FunI == II_wcsdup ||
-          FunI == II_win_wcsdup || FunI == II_kmalloc ||
-          FunI == II_g_malloc || FunI == II_g_malloc0 || 
-          FunI == II_g_realloc || FunI == II_g_try_malloc || 
-          FunI == II_g_try_malloc0 || FunI == II_g_try_realloc ||
-          FunI == II_g_memdup || FunI == II_g_malloc_n || 
-          FunI == II_g_malloc0_n || FunI == II_g_realloc_n || 
-          FunI == II_g_try_malloc_n || FunI == II_g_try_malloc0_n || 
-          FunI == II_g_try_realloc_n)
+      if (AllocFunctions.count(FunI) == 1 ||
+          AllocaFunctions.count(FunI) == 1 ||
+          StrdupFunctions.count(FunI) ||
+          FunI == II_if_nameindex)
         return true;
     }
 
     if (Family == AF_IfNameIndex && CheckFree) {
       if (FunI == II_if_freenameindex)
         return true;
     }
 
     if (Family == AF_IfNameIndex && CheckAlloc) {
       if (FunI == II_if_nameindex)
         return true;
     }
 
     if (Family == AF_Alloca && CheckAlloc) {
-      if (FunI == II_alloca || FunI == II_win_alloca)
+      if (AllocaFunctions.count(FunI) == 1)
         return true;
     }
   }
 
   if (Family != AF_Malloc)
     return false;
 
   if (IsOptimistic && FD->hasAttrs()) {
void MallocChecker::checkPostStmt(const CallExpr *CE, CheckerContext &C) const {
 
   ProgramStateRef State = C.getState();
   bool ReleasedAllocatedMemory = false;
 
   if (FD->getKind() == Decl::Function) {
     initIdentifierInfo(C.getASTContext());
     IdentifierInfo *FunI = FD->getIdentifier();
 
-    if (FunI == II_malloc || FunI == II_g_malloc || FunI == II_g_try_malloc) {
+    if (SimpleMallocFunctions.count(FunI) == 1) {
       if (CE->getNumArgs() < 1)
         return;
       if (CE->getNumArgs() < 3) {
         State = MallocMemAux(C, CE, CE->getArg(0), UndefinedVal(), State);
         if (CE->getNumArgs() == 1)
           State = ProcessZeroAllocation(C, CE, 0, State);
       } else if (CE->getNumArgs() == 3) {
         llvm::Optional<ProgramStateRef> MaybeState =
if (SimpleMallocFunctions.count(FunI) == 1) {
         State = MaybeState.getValue();
       else
         State = MallocMemAux(C, CE, CE->getArg(0), UndefinedVal(), State);
     } else if (FunI == II_valloc) {
       if (CE->getNumArgs() < 1)
         return;
       State = MallocMemAux(C, CE, CE->getArg(0), UndefinedVal(), State);
       State = ProcessZeroAllocation(C, CE, 0, State);
-    } else if (FunI == II_realloc || FunI == II_g_realloc || 
-               FunI == II_g_try_realloc) {
+    } else if (ReallocFunctions.count(FunI) == 1) {
       State = ReallocMemAux(C, CE, false, State);
       State = ProcessZeroAllocation(C, CE, 1, State);
     } else if (FunI == II_reallocf) {
       State = ReallocMemAux(C, CE, true, State);
       State = ProcessZeroAllocation(C, CE, 1, State);
     } else if (FunI == II_calloc) {
       State = CallocMem(C, CE, State);
       State = ProcessZeroAllocation(C, CE, 0, State);
       State = ProcessZeroAllocation(C, CE, 1, State);
-    } else if (FunI == II_free || FunI == II_g_free) {
+    } else if (FunI == II_recallocarray) {
+      State = ReallocMemAux(C, CE, false, State, true, true);
+      State = ProcessZeroAllocation(C, CE, 0, State);
+      State = ProcessZeroAllocation(C, CE, 1, State);
+    } else if (FreeFunctions.count(FunI) == 1) {
       State = FreeMemAux(C, CE, State, 0, false, ReleasedAllocatedMemory);
-    } else if (FunI == II_strdup || FunI == II_win_strdup ||
-               FunI == II_wcsdup || FunI == II_win_wcsdup) {
+    } else if (StrdupFunctions.count(FunI) == 1) {
       State = MallocUpdateRefState(C, CE, State);
     } else if (FunI == II_strndup) {
       State = MallocUpdateRefState(C, CE, State);
-    } else if (FunI == II_alloca || FunI == II_win_alloca) {
+    } else if (AllocaFunctions.count(FunI) == 1) {
       if (CE->getNumArgs() < 1)
         return;
       State = MallocMemAux(C, CE, CE->getArg(0), UndefinedVal(), State,
                            AF_Alloca);
       State = ProcessZeroAllocation(C, CE, 0, State);
     } else if (isStandardNewDelete(FD, C.getASTContext())) {
       // Process direct calls to operator new/new[]/delete/delete[] functions
       // as distinct from new/new[]/delete/delete[] expressions that are
if (SimpleMallocFunctions.count(FunI) == 1) {
       SVal zeroVal = svalBuilder.makeZeroVal(svalBuilder.getContext().CharTy);
       State = MallocMemAux(C, CE, CE->getArg(0), zeroVal, State);
       State = ProcessZeroAllocation(C, CE, 0, State);
     } else if (FunI == II_g_memdup) {
       if (CE->getNumArgs() < 2)
         return;
       State = MallocMemAux(C, CE, CE->getArg(1), UndefinedVal(), State);
       State = ProcessZeroAllocation(C, CE, 1, State);
-    } else if (FunI == II_g_malloc_n || FunI == II_g_try_malloc_n || 
-               FunI == II_g_malloc0_n || FunI == II_g_try_malloc0_n) {
+    } else if (NZeroMallocFunctions.count(FunI) == 1) {
       if (CE->getNumArgs() < 2)
         return;
       SVal Init = UndefinedVal();
       if (FunI == II_g_malloc0_n || FunI == II_g_try_malloc0_n) {
         SValBuilder &SB = C.getSValBuilder();
         Init = SB.makeZeroVal(SB.getContext().CharTy);
       }
       SVal TotalSize = evalMulForBufferSize(C, CE->getArg(0), CE->getArg(1));
       State = MallocMemAux(C, CE, TotalSize, Init, State);
       State = ProcessZeroAllocation(C, CE, 0, State);
       State = ProcessZeroAllocation(C, CE, 1, State);
-    } else if (FunI == II_g_realloc_n || FunI == II_g_try_realloc_n) {
+    } else if (NReallocFunctions.count(FunI) == 1) {
       if (CE->getNumArgs() < 3)
         return;
       State = ReallocMemAux(C, CE, false, State, true);
       State = ProcessZeroAllocation(C, CE, 1, State);
       State = ProcessZeroAllocation(C, CE, 2, State);
     }
   }