[analyzer] Use AllocaRegion in MallocChecker (#72402)

...to model the results of alloca() and _alloca() calls. Previously it
acted as if these functions were returning memory from the heap, which
led to alpha.security.ArrayBoundV2 producing incorrect messages.

Author: NagyDonat

clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h

@@ -215,6 +215,15 @@ class SValBuilder {
                                                 const LocationContext *LCtx,
                                                 QualType type, unsigned Count);
 
+  /// Create an SVal representing the result of an alloca()-like call, that is,
+  /// an AllocaRegion on the stack.
+  ///
+  /// After calling this function, it's a good idea to set the extent of the
+  /// returned AllocaRegion.
+  loc::MemRegionVal getAllocaRegionVal(const Expr *E,
+                                       const LocationContext *LCtx,
+                                       unsigned Count);
+
   DefinedOrUnknownSVal getDerivedRegionValueSymbolVal(
       SymbolRef parentSymbol, const TypedValueRegion *region);
 

clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp

@@ -81,22 +81,20 @@ bool BuiltinFunctionChecker::evalCall(const CallEvent &Call,
 
   case Builtin::BI__builtin_alloca_with_align:
   case Builtin::BI__builtin_alloca: {
-    // FIXME: Refactor into StoreManager itself?
-    MemRegionManager& RM = C.getStoreManager().getRegionManager();
-    const AllocaRegion* R =
-      RM.getAllocaRegion(CE, C.blockCount(), C.getLocationContext());
-
-    // Set the extent of the region in bytes. This enables us to use the
-    // SVal of the argument directly. If we save the extent in bits, we
-    // cannot represent values like symbol*8.
-    auto Size = Call.getArgSVal(0);
-    if (Size.isUndef())
-      return true; // Return true to model purity.
-
-    state = setDynamicExtent(state, R, Size.castAs<DefinedOrUnknownSVal>(),
-                             C.getSValBuilder());
+    SValBuilder &SVB = C.getSValBuilder();
+    const loc::MemRegionVal R =
+        SVB.getAllocaRegionVal(CE, C.getLocationContext(), C.blockCount());
 
-    C.addTransition(state->BindExpr(CE, LCtx, loc::MemRegionVal(R)));
+    // Set the extent of the region in bytes. This enables us to use the SVal
+    // of the argument directly. If we saved the extent in bits, it'd be more
+    // difficult to reason about values like symbol*8.
+    auto Size = Call.getArgSVal(0);
+    if (auto DefSize = Size.getAs<DefinedOrUnknownSVal>()) {
+      // This `getAs()` is mostly paranoia, because core.CallAndMessage reports
+      // undefined function arguments (unless it's disabled somehow).
+      state = setDynamicExtent(state, R.getRegion(), *DefSize, SVB);
+    }
+    C.addTransition(state->BindExpr(CE, LCtx, R));
     return true;
   }
 

clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp

@@ -1728,13 +1728,15 @@ ProgramStateRef MallocChecker::MallocMemAux(CheckerContext &C,
     return nullptr;
 
   // Bind the return value to the symbolic value from the heap region.
-  // TODO: We could rewrite post visit to eval call; 'malloc' does not have
-  // side effects other than what we model here.
+  // TODO: move use of this functions to an EvalCall callback, becasue
+  // BindExpr() should'nt be used elsewhere.
   unsigned Count = C.blockCount();
-  SValBuilder &svalBuilder = C.getSValBuilder();
+  SValBuilder &SVB = C.getSValBuilder();
   const LocationContext *LCtx = C.getPredecessor()->getLocationContext();
-  DefinedSVal RetVal = svalBuilder.getConjuredHeapSymbolVal(CE, LCtx, Count)
-      .castAs<DefinedSVal>();
+  DefinedSVal RetVal =
+      ((Family == AF_Alloca) ? SVB.getAllocaRegionVal(CE, LCtx, Count)
+                             : SVB.getConjuredHeapSymbolVal(CE, LCtx, Count)
+                                   .castAs<DefinedSVal>());
   State = State->BindExpr(CE, C.getLocationContext(), RetVal);
 
   // Fill the region with the initialization value.
@@ -1746,7 +1748,7 @@ ProgramStateRef MallocChecker::MallocMemAux(CheckerContext &C,
 
   // Set the region's extent.
   State = setDynamicExtent(State, RetVal.getAsRegion(),
-                           Size.castAs<DefinedOrUnknownSVal>(), svalBuilder);
+                           Size.castAs<DefinedOrUnknownSVal>(), SVB);
 
   return MallocUpdateRefState(C, CE, State, Family);
 }

clang/lib/StaticAnalyzer/Core/SValBuilder.cpp

@@ -231,6 +231,14 @@ SValBuilder::getConjuredHeapSymbolVal(const Expr *E,
   return loc::MemRegionVal(MemMgr.getSymbolicHeapRegion(sym));
 }
 
+loc::MemRegionVal SValBuilder::getAllocaRegionVal(const Expr *E,
+                                                  const LocationContext *LCtx,
+                                                  unsigned VisitCount) {
+  const AllocaRegion *R =
+      getRegionManager().getAllocaRegion(E, VisitCount, LCtx);
+  return loc::MemRegionVal(R);
+}
+
 DefinedSVal SValBuilder::getMetadataSymbolVal(const void *symbolTag,
                                               const MemRegion *region,
                                               const Expr *expr, QualType type,

clang/test/Analysis/malloc.c

@@ -266,13 +266,21 @@ void CheckUseZeroAllocated1(void) {
 }
 
 char CheckUseZeroAllocated2(void) {
+  // NOTE: The `AllocaRegion` that models the return value of `alloca()`
+  // doesn't have an associated symbol, so the current implementation of
+  // `MallocChecker::checkUseZeroAllocated()` cannot provide warnings for it.
+  // However, other checkers like core.uninitialized.UndefReturn (that
+  // activates in these TCs) or the array bound checkers provide more generic,
+  // but still sufficient warnings in these cases, so I think it isn't
+  // important to cover this in MallocChecker.
   char *p = alloca(0);
-  return *p; // expected-warning {{Use of memory allocated with size zero}}
+  return *p; // expected-warning {{Undefined or garbage value returned to caller}}
 }
 
 char CheckUseZeroWinAllocated2(void) {
+  // Note: Same situation as `CheckUseZeroAllocated2()`.
   char *p = _alloca(0);
-  return *p; // expected-warning {{Use of memory allocated with size zero}}
+  return *p; // expected-warning {{Undefined or garbage value returned to caller}}
 }
 
 void UseZeroAllocated(int *p) {
@@ -727,6 +735,11 @@ void paramFree(int *p) {
   myfoo(p); // expected-warning {{Use of memory after it is freed}}
 }
 
+void allocaFree(void) {
+  int *p = alloca(sizeof(int));
+  free(p); // expected-warning {{Memory allocated by alloca() should not be deallocated}}
+}
+
 int* mallocEscapeRet(void) {
   int *p = malloc(12);
   return p; // no warning

clang/test/Analysis/memory-model.cpp

@@ -97,7 +97,7 @@ void symbolic_malloc() {
 
 void symbolic_alloca() {
   int *a = (int *)alloca(12);
-  clang_analyzer_dump(a);             // expected-warning {{Element{HeapSymRegion{conj}}
+  clang_analyzer_dump(a);             // expected-warning {{Element{alloca{}}
   clang_analyzer_dumpExtent(a);       // expected-warning {{12 S64b}}
   clang_analyzer_dumpElementCount(a); // expected-warning {{3 S64b}}
 }

clang/test/Analysis/out-of-bounds-diagnostics.c

@@ -107,12 +107,8 @@ void *alloca(size_t size);
 int allocaRegion(void) {
   int *mem = (int*)alloca(2*sizeof(int));
   mem[3] = -2;
-  // expected-warning@-1 {{Out of bound access to memory after the end of the heap area}}
-  // expected-note@-2 {{Access of the heap area at index 3, while it holds only 2 'int' elements}}
-  // FIXME: this should be
-  //   {{Out of bound access to memory after the end of the memory returned by 'alloca'}}
-  //   {{Access of the memory returned by 'alloca' at index 3, while it holds only 2 'int' elements}}
-  // but apparently something models 'alloca' as if it was allocating on the heap
+  // expected-warning@-1 {{Out of bound access to memory after the end of the memory returned by 'alloca'}}
+  // expected-note@-2 {{Access of the memory returned by 'alloca' at index 3, while it holds only 2 'int' elements}}
   return *mem;
 }