Fix RegionStore assertion failure after #127602

[balazs-benics-sonarsource]

Basically, we may leave the loop because if exhaust the fields, array elements or other subobjects to initialize.
In that case, the Bindings may be in an exhausted state, thus no further addBinding calls are allowed.

Let's harden the code by sprinkling some early exists in the recursive dispatcher functions.
And to actually fix the issue, I added a check guarding the single unguarded addBinding right after a loop I mentioned.

Fixes #129211

[llvmbot]

@llvm/pr-subscribers-clang

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

Author: Balázs Benics (balazs-benics-sonarsource)

Changes

Basically, we may leave the loop because if exhaust the fields, array elements or other subobjects to initialize.
In that case, the Bindings may be in an exhausted state, thus no further addBinding calls are allowed.

Let's harden the code by sprinkling some early exists in the recursive dispatcher functions.
And to actually fix the issue, I added a check guarding the single unguarded addBinding right after a loop I mentioned.

Fixes #129211

---

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

2 Files Affected:

• (modified) clang/lib/StaticAnalyzer/Core/RegionStore.cpp (+25)
• (modified) clang/test/Analysis/region-store.cpp (+29)

diff --git a/clang/lib/StaticAnalyzer/Core/RegionStore.cpp b/clang/lib/StaticAnalyzer/Core/RegionStore.cpp
index 620fc117c6789..550a276c66c71 100644
--- a/clang/lib/StaticAnalyzer/Core/RegionStore.cpp
+++ b/clang/lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -2570,6 +2570,9 @@ RegionStoreManager::bind(LimitedRegionBindingsConstRef B, Loc L, SVal V) {
 LimitedRegionBindingsRef
 RegionStoreManager::setImplicitDefaultValue(LimitedRegionBindingsConstRef B,
                                             const MemRegion *R, QualType T) {
+  if (B.hasExhaustedBindingLimit())
+    return B;
+
   SVal V;
 
   if (Loc::isLocType(T))
@@ -2596,6 +2599,8 @@ RegionStoreManager::setImplicitDefaultValue(LimitedRegionBindingsConstRef B,
 std::optional<LimitedRegionBindingsRef> RegionStoreManager::tryBindSmallArray(
     LimitedRegionBindingsConstRef B, const TypedValueRegion *R,
     const ArrayType *AT, nonloc::LazyCompoundVal LCV) {
+  if (B.hasExhaustedBindingLimit())
+    return B.withValuesEscaped(LCV);
 
   auto CAT = dyn_cast<ConstantArrayType>(AT);
 
@@ -2632,6 +2637,8 @@ RegionStoreManager::bindArray(LimitedRegionBindingsConstRef B,
                               const TypedValueRegion *R, SVal Init) {
   llvm::TimeTraceScope TimeScope("RegionStoreManager::bindArray",
                                  [R]() { return R->getDescriptiveName(); });
+  if (B.hasExhaustedBindingLimit())
+    return B.withValuesEscaped(Init);
 
   const ArrayType *AT =cast<ArrayType>(Ctx.getCanonicalType(R->getValueType()));
   QualType ElementTy = AT->getElementType();
@@ -2698,6 +2705,9 @@ RegionStoreManager::bindVector(LimitedRegionBindingsConstRef B,
                                const TypedValueRegion *R, SVal V) {
   llvm::TimeTraceScope TimeScope("RegionStoreManager::bindVector",
                                  [R]() { return R->getDescriptiveName(); });
+  if (B.hasExhaustedBindingLimit())
+    return B.withValuesEscaped(V);
+
   QualType T = R->getValueType();
   const VectorType *VT = T->castAs<VectorType>(); // Use castAs for typedefs.
 
@@ -2722,6 +2732,9 @@ RegionStoreManager::bindVector(LimitedRegionBindingsConstRef B,
     if (VI == VE)
       break;
 
+    if (NewB.hasExhaustedBindingLimit())
+      return NewB.withValuesEscaped(VI, VE);
+
     NonLoc Idx = svalBuilder.makeArrayIndex(index);
     const ElementRegion *ER = MRMgr.getElementRegion(ElemType, Idx, R, Ctx);
 
@@ -2758,6 +2771,9 @@ RegionStoreManager::getUniqueDefaultBinding(nonloc::LazyCompoundVal LCV) const {
 std::optional<LimitedRegionBindingsRef> RegionStoreManager::tryBindSmallStruct(
     LimitedRegionBindingsConstRef B, const TypedValueRegion *R,
     const RecordDecl *RD, nonloc::LazyCompoundVal LCV) {
+  if (B.hasExhaustedBindingLimit())
+    return B.withValuesEscaped(LCV);
+
   // If we try to copy a Conjured value representing the value of the whole
   // struct, don't try to element-wise copy each field.
   // That would unnecessarily bind Derived symbols slicing off the subregion for
@@ -2822,6 +2838,9 @@ RegionStoreManager::bindStruct(LimitedRegionBindingsConstRef B,
                                const TypedValueRegion *R, SVal V) {
   llvm::TimeTraceScope TimeScope("RegionStoreManager::bindStruct",
                                  [R]() { return R->getDescriptiveName(); });
+  if (B.hasExhaustedBindingLimit())
+    return B.withValuesEscaped(V);
+
   QualType T = R->getValueType();
   assert(T->isStructureOrClassType());
 
@@ -2931,6 +2950,9 @@ RegionStoreManager::bindStruct(LimitedRegionBindingsConstRef B,
     ++VI;
   }
 
+  if (NewB.hasExhaustedBindingLimit())
+    return NewB.withValuesEscaped(VI, VE);
+
   // There may be fewer values in the initialize list than the fields of struct.
   if (FI != FE) {
     NewB = NewB.addBinding(R, BindingKey::Default,
@@ -2945,6 +2967,9 @@ RegionStoreManager::bindAggregate(LimitedRegionBindingsConstRef B,
                                   const TypedRegion *R, SVal Val) {
   llvm::TimeTraceScope TimeScope("RegionStoreManager::bindAggregate",
                                  [R]() { return R->getDescriptiveName(); });
+  if (B.hasExhaustedBindingLimit())
+    return B.withValuesEscaped(Val);
+
   // Remove the old bindings, using 'R' as the root of all regions
   // we will invalidate. Then add the new binding.
   return removeSubRegionBindings(B, R).addBinding(R, BindingKey::Default, Val);
diff --git a/clang/test/Analysis/region-store.cpp b/clang/test/Analysis/region-store.cpp
index 9e80a2e688575..cb3313cbbb313 100644
--- a/clang/test/Analysis/region-store.cpp
+++ b/clang/test/Analysis/region-store.cpp
@@ -386,3 +386,32 @@ void tooManyFnArgumentsWhenInlining() {
     10,11,12,13,14,15,16,17,18,19,
   });
 }
+
+void gh129211_assertion() {
+  struct Clazz {
+    int b;
+    int : 0;
+  };
+
+  Clazz d[][5][5] = {
+    {
+      {{}, {}, {}, {}, {}},
+      {{}, {}, {}, {}, {}},
+      {{}, {}, {}, {}, {}},
+      {{}, {}, {}, {}, {}},
+      {{}, {}, {}, {}, {}}
+    },
+    {
+      {{}, {}, {}, {}, {}},
+      {{}, {}, {}, {}, {}},
+      {{}, {}, {}, {}, {}},
+      {{}, {}, {}, {}, {}},
+      {{}, {}, {}, {}, {}},
+    },
+    {
+      {{}, {}, {}, {}, {}},
+      {{}, {}, {}, {}, {}},
+      {{}, {}, {}, {}},
+    }
+  }; // no-crash
+}

[mikaelholmen]

I've verified that this patch solves the problem I saw. Thanks!

[steakhal]

FYI @necto @NagyDonat