[analyzer] Harden RegionStoreManager::bindArray

[marco-antognini-sonarsource]

Fixes #147686 by handling symbolic values similarly to bindStruct and handling constant values. The latter is actually more of a workaround: bindArray should not have to deal with such constants.

CPP-6688

[llvmbot]

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

@llvm/pr-subscribers-clang

Author: Marco Borgeaud (marco-antognini-sonarsource)

Changes

Fixes #147686 by completing handling of literals and handling symbolic values similarly to bindStruct.

CPP-6688

---

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

2 Files Affected:

• (modified) clang/lib/StaticAnalyzer/Core/RegionStore.cpp (+7-2)
• (modified) clang/test/Analysis/initializer.cpp (+41)

diff --git a/clang/lib/StaticAnalyzer/Core/RegionStore.cpp b/clang/lib/StaticAnalyzer/Core/RegionStore.cpp
index 02375b0c3469a..ebe1a264e40f8 100644
--- a/clang/lib/StaticAnalyzer/Core/RegionStore.cpp
+++ b/clang/lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -2654,14 +2654,19 @@ RegionStoreManager::bindArray(LimitedRegionBindingsConstRef B,
     SVal V = getBinding(B.asStore(), *MRV, R->getValueType());
     return bindAggregate(B, R, V);
   }
+  if (auto const *Value = Init.getAsInteger()) {
+    auto SafeValue = StateMgr.getBasicVals().getValue(*Value);
+    return bindAggregate(B, R, nonloc::ConcreteInt(SafeValue));
+  }
 
-  // Handle lazy compound values.
+  // Handle lazy compound values and symbolic values.
   if (std::optional LCV = Init.getAs<nonloc::LazyCompoundVal>()) {
     if (std::optional NewB = tryBindSmallArray(B, R, AT, *LCV))
       return *NewB;
-
     return bindAggregate(B, R, Init);
   }
+  if (isa<nonloc::SymbolVal>(Init))
+    return bindAggregate(B, R, Init);
 
   if (Init.isUnknown())
     return bindAggregate(B, R, UnknownVal());
diff --git a/clang/test/Analysis/initializer.cpp b/clang/test/Analysis/initializer.cpp
index 713e121168571..ee90705ac3d28 100644
--- a/clang/test/Analysis/initializer.cpp
+++ b/clang/test/Analysis/initializer.cpp
@@ -610,3 +610,44 @@ void top() {
   consume(parseMatchComponent());
 }
 } // namespace elementwise_copy_small_array_from_post_initializer_of_cctor
+
+namespace gh147686 {
+// The problem reported in https://github.com/llvm/llvm-project/issues/147686
+// is sensitive to the initializer form: using parenthesis to initialize m_ptr
+// resulted in crashes when analyzing *m_ptr = '\0'; but using braces is fine.
+
+struct A {
+  A() : m_ptr(m_buf) { *m_ptr = '\0'; } // no-crash
+  A(int overload) : m_ptr{m_buf} { *m_ptr = '\0'; }
+  A(char src) : m_ptr(m_buf) { *m_ptr = src; } // no-crash
+  A(char src, int overload) : m_ptr{m_buf} { *m_ptr = src; }
+  char m_buf[64] = {0};
+  char * m_ptr;
+};
+
+void test1() {
+  A a;
+  clang_analyzer_eval(a.m_buf[0] == 0); // expected-warning{{TRUE}}
+  // FIXME The next eval should result in TRUE.
+  clang_analyzer_eval(*a.m_ptr == 0); // expected-warning{{UNKNOWN}}
+}
+
+void test2() {
+  A a(314);
+  clang_analyzer_eval(a.m_buf[0] == 0); // expected-warning{{TRUE}}
+  clang_analyzer_eval(*a.m_ptr == 0); // expected-warning{{TRUE}}
+}
+
+void test3() {
+  A a(0);
+  clang_analyzer_eval(a.m_buf[0] == 0); // expected-warning{{TRUE}}
+  clang_analyzer_eval(*a.m_ptr == 0); // expected-warning{{TRUE}}
+}
+
+void test4() {
+  A a(0, 314);
+  clang_analyzer_eval(a.m_buf[0] == 0); // expected-warning{{TRUE}}
+  clang_analyzer_eval(*a.m_ptr == 0); // expected-warning{{TRUE}}
+}
+
+} // namespace gh147686

[balazs-benics-sonarsource]

I recall that I already reviewed this downstream.
The fix is not perfect, as in the test we shouldn't be in the bindArray handler in Store at all - as we don't elementwise copy any aggregates (aka arrays) in the example.
So this should be thought of a hotfix rather than a proper fix to the underlying issue, which is why are we binding an array here.

I'd recommend to mark the lines in the test with the no-crash where we previously crashed.
And also elaborate in the commit message why this is not a proper fix, but just a hotfix.

All in all, not crashing is strictly better than crashing - in almost all scenario.
This patch does not do anything fundamentally wrong, so I approved downstream, and I'd also approve it here, but there is also a conflict of interest so I'll let the rest of the maintainers decide.

[NagyDonat]

Thanks for the patch! I have some minor remarks inline but overall I like the patch and I completely agree with the maxim that "not crashing is strictly better than crashing".

Also thanks @steakhal for clarifying that this is a "hotfix" and covers situations that "do not belong to" bindArray. I agree that this should be mentioned in the commit message; in fact I think that we should also mention this in a FIXME source code comment, because it would be really valuable for those who try to understand the purpose of this method (It is called bindArray... but it also handles integer literals... maybe it's misnamed... ?).

[marco-antognini-sonarsource]

@NagyDonat I appreciate your patience on this PR. I've managed to find time to address your feedback. Let me know if you have more of course.

[NagyDonat]

LGTM, thanks for the updates!

It's a bit unfortunate that test3Bis doesn't show the ideal behavior, but this commit is a clear step forward and there is no expectation to fix everything with one commit.