[analyzer] fix crash on binding to symbolic region with void * type

[pskrgag]

As reported in #103714 (comment). CSA crashes on trying to bind value to symbolic region with void *. This happens when such region gets passed as inline asm input and engine tries to bind UnknownVal to that region.

Fix it by changing type from void to char before calling GetElementZeroRegion

[llvmbot]

@llvm/pr-subscribers-clang

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

Author: Pavel Skripkin (pskrgag)

Changes

As reported in #103714 (comment). CSA crashes on trying to bind value to symbolic region with void *. This happens when such region gets passed as inline asm input and engine tries to bind UnknownVal to that region.

Fix it by changing type from void to char before calling GetElementZeroRegion

---

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

2 Files Affected:

• (modified) clang/lib/StaticAnalyzer/Core/RegionStore.cpp (+10-2)
• (modified) clang/test/Analysis/asm.cpp (+9)

diff --git a/clang/lib/StaticAnalyzer/Core/RegionStore.cpp b/clang/lib/StaticAnalyzer/Core/RegionStore.cpp
index ba29c123139016..a523daad28736f 100644
--- a/clang/lib/StaticAnalyzer/Core/RegionStore.cpp
+++ b/clang/lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -2380,8 +2380,16 @@ RegionStoreManager::bind(RegionBindingsConstRef B, Loc L, SVal V) {
 
   // Binding directly to a symbolic region should be treated as binding
   // to element 0.
-  if (const SymbolicRegion *SR = dyn_cast<SymbolicRegion>(R))
-    R = GetElementZeroRegion(SR, SR->getPointeeStaticType());
+  if (const SymbolicRegion *SR = dyn_cast<SymbolicRegion>(R)) {
+    // Symbolic region with void * type may appear as input for inline asm
+    // block. In such case CSA cannot reason about region content and just
+    // assumes it has UnknownVal()
+    QualType PT = SR->getPointeeStaticType();
+    if (PT->isVoidType())
+      PT = StateMgr.getContext().CharTy;
+
+    R = GetElementZeroRegion(SR, PT);
+  }
 
   assert((!isa<CXXThisRegion>(R) || !B.lookup(R)) &&
          "'this' pointer is not an l-value and is not assignable");
diff --git a/clang/test/Analysis/asm.cpp b/clang/test/Analysis/asm.cpp
index b17ab04994d249..0abaa96b0ae79e 100644
--- a/clang/test/Analysis/asm.cpp
+++ b/clang/test/Analysis/asm.cpp
@@ -40,3 +40,12 @@ void testInlineAsmMemcpyUninit(void)
     MyMemcpy(&a[1], &b[1], sizeof(b) - sizeof(b[1]));
     c = a[0]; // expected-warning{{Assigned value is garbage or undefined}}
 }
+
+void *globalPtr;
+
+void testNoCrash()
+{
+  // Use global pointer to make it symbolic. Then engine will try to bind
+  // value to first element of type void * and should not crash.
+  asm ("" : : "a"(globalPtr)); // no crash
+}

[pskrgag]

CC @steakhal @NagyDonat

[NagyDonat]

To me this solution seems to be a bit hacky -- I don't like that we need to scatter "handle void * as if it was char *" special cases in various parts of the analyzer (I vaguely recall that I have also seen similar hacks elsewhere).

I'd prefer solutions that are as generic as possible and ensure that void * is consistently handled like char * if that's what we want. (By the way, will the analyzer be able to use this UnknownVal bound to the void * pointer? If not, then just avoid the binding.)

These are not blocking issues, feel free to merge this commit to fix the crash (instead of waiting for a theoretically better solution).

[steakhal]

LGTM, thanks!

[steakhal]

Let's wait for the premerge tests before merging.

[pskrgag]

added missing declarations. should fix the CI

[NagyDonat]

Bikeshedding: let's delete that empty line.

Otherwise LGTM.