llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang
@llvm/pr-subscribers-clang-static-analyzer-1
Author: Balázs Benics (steakhal)
<details>
<summary>Changes</summary>
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
---
Full diff: https://github.com/llvm/llvm-project/pull/178694.diff
2 Files Affected:
- (modified) clang/lib/StaticAnalyzer/Core/RegionStore.cpp (+8)
- (added) clang/test/Analysis/store-union-aggregates.c (+35)
``````````diff
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
+}
``````````
</details>
https://github.com/llvm/llvm-project/pull/178694
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
