[clang] [analyzer] [MallocChecker] Fix Store modification in `checkPreCall` (PR #109337)

[via cfe-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang

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

Author: Pavel Skripkin (pskrgag)

<details>
<summary>Changes</summary>

This is small follow-up for #<!-- -->106081

While trying to add sanity check for `Enviroment` and `Store` being consistent during `checkPostCall` and `checkPreCall` I found out that `MallocChecker` still violates that rule. 

The problem lies in `FreeMemAux`, which invalidates freed region while being called from `preGetdelim`. This invalidation was added to prevent "use of uninitialized memory" for malloc and friends while `unix.Malloc` is disabled.

Fix adds another bool flag to `FreeMemAux` to control invalidation from call-site and set it to false in `preGetdelim`



---
Full diff: https://github.com/llvm/llvm-project/pull/109337.diff


2 Files Affected:

- (modified) clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp (+30-25) 
- (modified) clang/test/Analysis/NewDelete-intersections.mm (+9) 


``````````diff
diff --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
index 81ec8e1b516986..7a265d3bbcda47 100644
--- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -668,7 +668,8 @@ class MallocChecker
   [[nodiscard]] ProgramStateRef
   FreeMemAux(CheckerContext &C, const CallEvent &Call, ProgramStateRef State,
              unsigned Num, bool Hold, bool &IsKnownToBeAllocated,
-             AllocationFamily Family, bool ReturnsNullOnFailure = false) const;
+             AllocationFamily Family, bool Invalidate = true,
+             bool ReturnsNullOnFailure = false) const;
 
   /// Models memory deallocation.
   ///
@@ -694,7 +695,8 @@ class MallocChecker
   [[nodiscard]] ProgramStateRef
   FreeMemAux(CheckerContext &C, const Expr *ArgExpr, const CallEvent &Call,
              ProgramStateRef State, bool Hold, bool &IsKnownToBeAllocated,
-             AllocationFamily Family, bool ReturnsNullOnFailure = false,
+             AllocationFamily Family, bool Invalidate = true,
+             bool ReturnsNullOnFailure = false,
              std::optional<SVal> ArgValOpt = {}) const;
 
   // TODO: Needs some refactoring, as all other deallocation modeling
@@ -1474,9 +1476,10 @@ void MallocChecker::preGetdelim(ProgramStateRef State, const CallEvent &Call,
   // We do not need this value here, as FreeMemAux will take care
   // of reporting any violation of the preconditions.
   bool IsKnownToBeAllocated = false;
-  State = FreeMemAux(C, Call.getArgExpr(0), Call, State, false,
-                     IsKnownToBeAllocated, AllocationFamily(AF_Malloc), false,
-                     LinePtr);
+  State =
+      FreeMemAux(C, Call.getArgExpr(0), Call, State, false,
+                 IsKnownToBeAllocated, AllocationFamily(AF_Malloc),
+                 /*Invalidate=*/false, /*ReturnsNullOnFailure=*/false, LinePtr);
   if (State)
     C.addTransition(State);
 }
@@ -1793,6 +1796,7 @@ void MallocChecker::checkPostObjCMessage(const ObjCMethodCall &Call,
   ProgramStateRef State = FreeMemAux(C, Call.getArgExpr(0), Call, C.getState(),
                                      /*Hold=*/true, IsKnownToBeAllocatedMemory,
                                      AllocationFamily(AF_Malloc),
+                                     /*Invalidate=*/false,
                                      /*ReturnsNullOnFailure=*/true);
 
   C.addTransition(State);
@@ -1986,12 +1990,11 @@ ProgramStateRef MallocChecker::FreeMemAttr(CheckerContext &C,
   return State;
 }
 
-ProgramStateRef MallocChecker::FreeMemAux(CheckerContext &C,
-                                          const CallEvent &Call,
-                                          ProgramStateRef State, unsigned Num,
-                                          bool Hold, bool &IsKnownToBeAllocated,
-                                          AllocationFamily Family,
-                                          bool ReturnsNullOnFailure) const {
+ProgramStateRef
+MallocChecker::FreeMemAux(CheckerContext &C, const CallEvent &Call,
+                          ProgramStateRef State, unsigned Num, bool Hold,
+                          bool &IsKnownToBeAllocated, AllocationFamily Family,
+                          bool Invalidate, bool ReturnsNullOnFailure) const {
   if (!State)
     return nullptr;
 
@@ -1999,7 +2002,8 @@ ProgramStateRef MallocChecker::FreeMemAux(CheckerContext &C,
     return nullptr;
 
   return FreeMemAux(C, Call.getArgExpr(Num), Call, State, Hold,
-                    IsKnownToBeAllocated, Family, ReturnsNullOnFailure);
+                    IsKnownToBeAllocated, Family, Invalidate,
+                    ReturnsNullOnFailure);
 }
 
 /// Checks if the previous call to free on the given symbol failed - if free
@@ -2134,12 +2138,11 @@ static void printExpectedDeallocName(raw_ostream &os, AllocationFamily Family) {
   }
 }
 
-ProgramStateRef
-MallocChecker::FreeMemAux(CheckerContext &C, const Expr *ArgExpr,
-                          const CallEvent &Call, ProgramStateRef State,
-                          bool Hold, bool &IsKnownToBeAllocated,
-                          AllocationFamily Family, bool ReturnsNullOnFailure,
-                          std::optional<SVal> ArgValOpt) const {
+ProgramStateRef MallocChecker::FreeMemAux(
+    CheckerContext &C, const Expr *ArgExpr, const CallEvent &Call,
+    ProgramStateRef State, bool Hold, bool &IsKnownToBeAllocated,
+    AllocationFamily Family, bool Invalidate, bool ReturnsNullOnFailure,
+    std::optional<SVal> ArgValOpt) const {
 
   if (!State)
     return nullptr;
@@ -2299,13 +2302,15 @@ MallocChecker::FreeMemAux(CheckerContext &C, const Expr *ArgExpr,
   // that.
   assert(!RsBase || (RsBase && RsBase->getAllocationFamily() == Family));
 
-  // Assume that after memory is freed, it contains unknown values. This
-  // conforts languages standards, since reading from freed memory is considered
-  // UB and may result in arbitrary value.
-  State = State->invalidateRegions({location}, Call.getOriginExpr(),
-                                   C.blockCount(), C.getLocationContext(),
-                                   /*CausesPointerEscape=*/false,
-                                   /*InvalidatedSymbols=*/nullptr);
+  if (Invalidate) {
+    // Assume that after memory is freed, it contains unknown values. This
+    // conforts languages standards, since reading from freed memory is
+    // considered UB and may result in arbitrary value.
+    State = State->invalidateRegions({location}, Call.getOriginExpr(),
+                                     C.blockCount(), C.getLocationContext(),
+                                     /*CausesPointerEscape=*/false,
+                                     /*InvalidatedSymbols=*/nullptr);
+  }
 
   // Normal free.
   if (Hold)
diff --git a/clang/test/Analysis/NewDelete-intersections.mm b/clang/test/Analysis/NewDelete-intersections.mm
index e897f48b839620..29f4d35d868b2e 100644
--- a/clang/test/Analysis/NewDelete-intersections.mm
+++ b/clang/test/Analysis/NewDelete-intersections.mm
@@ -17,6 +17,7 @@
 
 #include "Inputs/system-header-simulator-cxx.h"
 #include "Inputs/system-header-simulator-objc.h"
+#include "Inputs/system-header-simulator.h"
 
 typedef __typeof__(sizeof(int)) size_t;
 extern "C" void *malloc(size_t);
@@ -86,3 +87,11 @@ void testStandardPlacementNewAfterDelete() {
   delete p;
   p = new (p) int; // newdelete-warning{{Use of memory after it is freed}}
 }
+
+void testGetLine(FILE *f) {
+  char *buffer = (char *)malloc(10);
+  char *old = buffer;
+  size_t n = 0;
+  getline(&buffer, &n, f);
+  int a = *old; // no-warn
+}

``````````

</details>


https://github.com/llvm/llvm-project/pull/109337