[analyzer][StackAddrEscapeChecker] Fix assert failure for alloca regions

[necto]

Fixes #107852

Make it explicit that the checker skips alloca regions to avoid the risk of producing false positives for code with advanced memory management.
StackAddrEscapeChecker already used this strategy when it comes to malloc'ed regions, so this change relaxes the assertion and explicitly silents the issues related to memory regions generated with alloca.

[llvmbot]

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

@llvm/pr-subscribers-clang

Author: Arseniy Zaostrovnykh (necto)

Changes

Fixes #107852

Make it explicit that the checker skips alloca regions to avoid the risc of producing false positives for code that has advnaced memory management.
StackAddrEscapeChecker already used this strategy when it comes to malloc'ed regions, so this change relaxes the assertion and explicitly silents the issues related to memory regions generated with alloca.

---

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

2 Files Affected:

• (modified) clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp (+4)
• (modified) clang/test/Analysis/stack-addr-ps.cpp (+28-1)

diff --git a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
index d8c52941b19366..a76639bb86b208 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
@@ -337,6 +337,10 @@ static std::optional<std::string> printReferrer(const MemRegion *Referrer) {
       // warn_bind_ref_member_to_parameter or
       // warn_init_ptr_member_to_parameter_addr
       return std::nullopt;
+    } else if (isa<AllocaRegion>(Referrer)) {
+      // Skip alloca() regions, they indicate advanced memory management
+      // and higher likelihood of CSA false positives.
+      return std::nullopt;
     } else {
       assert(false && "Unexpected referrer region type.");
       return std::nullopt;
diff --git a/clang/test/Analysis/stack-addr-ps.cpp b/clang/test/Analysis/stack-addr-ps.cpp
index 35f38fbbfbefdc..1f8b62824772e1 100644
--- a/clang/test/Analysis/stack-addr-ps.cpp
+++ b/clang/test/Analysis/stack-addr-ps.cpp
@@ -1,4 +1,9 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -verify %s -Wno-undefined-bool-conversion
+// RUN: %clang_analyze_cc1 \
+// RUN:   -analyzer-checker=core,debug.ExprInspection,unix.Malloc \
+// RUN:   -verify %s \
+// RUN:   -Wno-undefined-bool-conversion
+// unix.Malloc is necessary to model __builtin_alloca,
+// which could trigger an "unexpected region" bug in StackAddrEscapeChecker.
 
 typedef __INTPTR_TYPE__ intptr_t;
 
@@ -846,3 +851,25 @@ void top(char **p) {
   foo(); // no-warning FIXME: p binding is reclaimed before the function end
 }
 } // namespace early_reclaim_dead_limitation
+
+using size_t = decltype(sizeof(int));
+void * malloc(size_t size);
+void free(void*);
+
+namespace alloca_region_pointer {
+void callee(char **pptr) {
+  char local;
+  *pptr = &local;
+}
+
+void top_alloca_no_crash() {
+  char **pptr = (char**)__builtin_alloca(sizeof(char*));
+  callee(pptr);
+}
+
+void top_malloc_no_crash_fn() {
+  char **pptr = (char**)malloc(sizeof(char*));
+  callee(pptr);
+  free(pptr);
+}
+} // namespace alloca_region_pointer

[steakhal]

Please mark the line in the test with no-crash where previously crashed.

Speaking of the fix, I think anything is better than a crash, but I wonder if we could do more.
To me, once an alloca region goes out of scope, that should behave just as if a regular stack variable, and raise an issue. What is it not the case here?

[necto]

Please mark the line in the test with no-crash where previously crashed.

Done in b2ed9f9

Speaking of the fix, I think anything is better than a crash, but I wonder if we could do more. To me, once an alloca region goes out of scope, that should behave just as if a regular stack variable, and raise an issue. What is it not the case here?

In this case, it is not an alloca region that went out of scope but a regular stack variable. However, unlike other test cases, it is an alloca region that keeps the pointer to the expired stack variable.

Just like with regions from malloc, alloca regions are harder to name. I could imagine naming them after the source location, which works for both malloc and alloca, but that requires substantially more engineering and quality control than this fix. Moreover, the use of explicit memory management primitives such as malloc and, even more so, alloca signals to me that the code likely does something non-trivial with memory, so CSA is prone to false positives because it is not tuned to this case.