https://github.com/steakhal created
https://github.com/llvm/llvm-project/pull/178694
Preserve existing default bindings in setImplicitDefaultValue to avoid
incorrectly overwriting the probably more accurate default binding when
initializing array elements with union members.
You can read the comments in the store-union-aggregates.c, but here's what
really matters:
We eventually bind the union field, using the "bindAggregate", which just
translates to a default binding at offset 0, binding
"compoundVal{&Element{global_buf}}".
So we will have a default binding at offset 0, by the time we realize that the
second element of the array "buffers" didn't have an explicit initializer
spelled, thus we want to default bind zero in "setImplicitDefaultValue".
This effectively replaces our original default binding with an incorrect one;
so the next time we access "buffers[0].buffer_ptr" we end up loading a NULL
ptr; and show a null deref FP.
I didn't dive too deep into why "bindAggregate" handles binding unions, but
changing that seems risky and I also don't immediately see how a proper fix
would look; because unlike with structs, we can't just dispatch the bind
operations by each field, because in union these members overlap.
Unions have a long list of issues anyway, and I'm not too inclined to fix them
right now. This workaround looks like the sweet spot to me.
Assisted-by: claude
rdar://167136849
From b6040aa4e4ce453b2e74b18bc302e2090640e91b Mon Sep 17 00:00:00 2001
From: Balazs Benics <[email protected]>
Date: Thu, 29 Jan 2026 16:54:27 +0100
Subject: [PATCH] [analyzer] Avoid overwriting the default binding of union
aggregates in incomplete array initializers
Preserve existing default bindings in setImplicitDefaultValue to avoid
incorrectly overwriting the probably more accurate default binding when
initializing array elements with union members.
You can read the comments in the store-union-aggregates.c, but here's
what really matters:
We eventually bind the union field, using the "bindAggregate", which
just translates to a default binding at offset 0, binding
"compoundVal{&Element{global_buf}}".
So we will have a default binding at offset 0, by the time we realize
that the second element of the array "buffers" didn't have an explicit
initializer spelled, thus we want to default bind zero in
"setImplicitDefaultValue".
This effectively replaces our original default binding with an incorrect
one; so the next time we access "buffers[0].buffer_ptr" we end up
loading a NULL ptr; and show a null deref FP.
I didn't dive too deep into why "bindAggregate" handles binding unions, but
changing that seems risky and I also don't immediately see how a proper
fix would look; because unlike with structs, we can't just dispatch
the bind operations by each field, because in union these members overlap.
Unions have a long list of issues anyway, and I'm not too inclined to
fix them right now. This workaround looks like the sweet spot to me.
Assisted-by: claude
rdar://167136849
---
clang/lib/StaticAnalyzer/Core/RegionStore.cpp | 8 +++++
clang/test/Analysis/store-union-aggregates.c | 35 +++++++++++++++++++
2 files changed, 43 insertions(+)
create mode 100644 clang/test/Analysis/store-union-aggregates.c
diff --git a/clang/lib/StaticAnalyzer/Core/RegionStore.cpp
b/clang/lib/StaticAnalyzer/Core/RegionStore.cpp
index 4f4824a3616ce..ccdbbe96f3b13 100644
--- a/clang/lib/StaticAnalyzer/Core/RegionStore.cpp
+++ b/clang/lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -2566,6 +2566,14 @@
RegionStoreManager::setImplicitDefaultValue(LimitedRegionBindingsConstRef B,
if (B.hasExhaustedBindingLimit())
return B;
+ // Prefer to keep the previous default binding if we had one; that is likely
a
+ // better choice than setting some arbitrary new default value.
+ // This isn't ideal (more of a hack), but better than dropping the more
+ // accurate default binding.
+ if (B.getDefaultBinding(R).has_value()) {
+ return B;
+ }
+
SVal V;
if (Loc::isLocType(T))
diff --git a/clang/test/Analysis/store-union-aggregates.c
b/clang/test/Analysis/store-union-aggregates.c
new file mode 100644
index 0000000000000..4b084cc080929
--- /dev/null
+++ b/clang/test/Analysis/store-union-aggregates.c
@@ -0,0 +1,35 @@
+// RUN: %clang_analyze_cc1 -verify %s
+
+// expected-no-diagnostics
+
+struct SizedBufferPtr {
+ union {
+ int* buffer_ptr;
+ } /*anonymous*/;
+ int size;
+};
+
+extern int global_buf_size;
+extern int global_buf[];
+
+void analysis_entry_point_rdar167136849(void) {
+ // When binding the initial value for "buffers".
+ // We will first bind the first array element: buffers[0] <-
cv{cv{&Elem{global_buf, 0}}}
+ // This is of type "struct SizedBufferPtr" so we dispatch this bind to bind
by fields.
+ // So we bind: buffers[0].union anonymous <- cv{&Elem{global_buf, 0}}
+ // Because that is "isUnionType", its bound using "bindAggregate", thus add
a default binding.
+ // After this we have in the Store: { default at 0: cv{&Elem{global_buf, 0}}
}
+ // We then bind the second field: buffers[0].size <- reg{global_buf_size}
+ // After this we have in the Store: { default at 0: cv{&Elem{global_buf,
0}}, direct at 64: reg{global_buf_size} }
+ // We are done with the initializer expression, so we realize that
+ // the second element of "buffers" didn't have an explicit initializer,
+ // so we need to bind a suitable default value using
"setImplicitDefaultValue".
+ // That happens to add a default binding 0 to "buffer", which overwrites the
previous default binding that "bindAggregate" added.
+ // After this we falsely conclude that "buffers[0].buffer_ptr" is a nullptr.
+
+ struct SizedBufferPtr buffers[2] = {
+ {.buffer_ptr = global_buf, .size = global_buf_size},
+ // [1] is not explicitly initialized, so it will be defaulted to zero.
+ };
+ *buffers[0].buffer_ptr = 2; // no-warning: writing indirectly to
"global_buf" is fine
+}
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
