[clang] [analyzer] Fix handling of zero-sized elements in ArrayBound (PR #218712)
Donát Nagy via cfe-commits
cfe-commits at lists.llvm.org
Wed Aug 26 06:03:37 PDT 2026
https://github.com/NagyDonat updated https://github.com/llvm/llvm-project/pull/218712
>From c6ae70468accc1aa05e4b8a37e72c59a77e4de7d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <donat.nagy at ericsson.com>
Date: Tue, 25 Aug 2026 15:47:25 +0200
Subject: [PATCH 1/4] Use plural for zero bytes
---
clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp | 2 +-
clang/test/Analysis/ArrayBound/verbose-tests.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp
index c4054ee8cf5c0..e41457ccc2786 100644
--- a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp
@@ -324,7 +324,7 @@ static BugDescription describeInvalidAccess(bounds::CheckResult Res,
Out << ' ' << SU.asElementName();
- if (*ExtentN > 1)
+ if (*ExtentN != 1)
Out << "s";
}
diff --git a/clang/test/Analysis/ArrayBound/verbose-tests.c b/clang/test/Analysis/ArrayBound/verbose-tests.c
index f4619fcc14006..96f4d29ae0777 100644
--- a/clang/test/Analysis/ArrayBound/verbose-tests.c
+++ b/clang/test/Analysis/ArrayBound/verbose-tests.c
@@ -478,6 +478,6 @@ struct Empty zeroSizeElements(void) {
// FIXME: We probably shouldn't report this access.
return ZeroSizeElements[5];
// expected-warning at -1 {{Out of bound access to memory after the end of 'ZeroSizeElements'}}
- // expected-note at -2 {{Access of 'ZeroSizeElements' at byte offset 0, while it holds only 0 byte}}
+ // expected-note at -2 {{Access of 'ZeroSizeElements' at byte offset 0, while it holds only 0 bytes}}
}
#endif
>From a7bcbd2afd19053e2c190f27713cca1300f269fe Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <donat.nagy at ericsson.com>
Date: Tue, 25 Aug 2026 15:46:54 +0200
Subject: [PATCH 2/4] Fix handling of access of zero-sized elements
Previously the ArrayBound checker mishandled the following code under
non-windows platforms where `sizeof(struct Empty) == 0`:
```
struct Empty {};
struct Empty Array[10];
struct Empty foo(void) { return Array[5]; }
```
The explanation of the false positive was "Access of 'Array' at
byte offset 0, while it holds only 0 bytes" -- that is, the checker
thought that this is access of a past-the-end pointer.
This commit suppresses this false positive by saying that accessing a
zero-sized object starting at the past-the-end pointer is valid.
This is implemented by adding an `AlsoAcceptEquality` flag for
`checkBounds`, which will also be useful when I will write checkers that
vaildate pointer arithmetics (where forming the past-the-end pointer is
completely valid).
---
.../StaticAnalyzer/Checkers/BoundsChecking.h | 23 +++++++++--
.../Checkers/ArrayBoundChecker.cpp | 24 ++++++------
.../Checkers/BoundsChecking.cpp | 18 ++++-----
.../test/Analysis/ArrayBound/verbose-tests.c | 38 +++++++++++++++++--
4 files changed, 75 insertions(+), 28 deletions(-)
diff --git a/clang/include/clang/StaticAnalyzer/Checkers/BoundsChecking.h b/clang/include/clang/StaticAnalyzer/Checkers/BoundsChecking.h
index 2c8469694b661..e90e3767bfd62 100644
--- a/clang/include/clang/StaticAnalyzer/Checkers/BoundsChecking.h
+++ b/clang/include/clang/StaticAnalyzer/Checkers/BoundsChecking.h
@@ -29,6 +29,7 @@ namespace clang::ento::bounds {
struct CheckFlags {
unsigned CheckUnderflow : 1;
unsigned OffsetObviouslyNonnegative : 1;
+ unsigned AlsoAcceptEquality : 1;
};
class CheckResult;
@@ -91,16 +92,30 @@ class CheckResult {
ProgramStateRef InBoundsState = nullptr;
};
-// Evaluate the comparison Value < Threshold with the help of the custom
+enum class Comparison { LT, LE, EQ };
+
+inline BinaryOperator::Opcode asOpcode(Comparison C) {
+ switch (C) {
+ case Comparison::LT:
+ return BO_LT;
+ case Comparison::LE:
+ return BO_LE;
+ case Comparison::EQ:
+ return BO_EQ;
+ }
+ llvm_unreachable("unhandled Comparison kind");
+}
+
+// Evaluate the comparison \p Value < \p Threshold with the help of the custom
// simplification algorithm. Return a pair of states, where the first one
// corresponds to "value below threshold" and the second corresponds to "value
// at or above threshold". Returns {nullptr, nullptr} in the case when the
// evaluation fails.
-// If the optional argument CheckEquality is true, then use BO_EQ instead of
-// the default BO_LT after consistently applying the same simplification steps.
+// If the optional argument \p CmpKind is specified, then that comparison
+// operator is used (instead of the default '<') after the same simplification
std::pair<ProgramStateRef, ProgramStateRef>
compareValueToThreshold(ProgramStateRef State, SValBuilder &SVB, NonLoc Value,
- NonLoc Threshold, bool CheckEquality = false);
+ NonLoc Threshold, Comparison CmpKind = Comparison::LT);
} // namespace clang::ento::bounds
#endif // LLVM_CLANG_STATICANALYZER_CHECKERS_BOUNDSCHECKING_H
diff --git a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp
index e41457ccc2786..3acbf7cd75ab8 100644
--- a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp
@@ -200,6 +200,14 @@ static bool isDeterminedByInterestingSymbol(SVal SV,
return false;
}
+static int64_t getElementSize(const ElementRegion *ER, SValBuilder &SVB) {
+ QualType ElemType = ER->getElementType();
+
+ assert(!ElemType->isIncompleteType() && "ElemType cannot be incomplete");
+
+ return SVB.getContext().getTypeSizeInChars(ElemType).getQuantity();
+}
+
/// For a given \p CurRegion that can be represented as a symbolic expression
/// Arr[Idx] (or perhaps Arr[Idx1][Idx2] etc.), return the parent memory block
/// Arr and the distance of Location from the beginning of Arr (expressed in a
@@ -222,17 +230,8 @@ computeOffset(ProgramStateRef State, SValBuilder &SVB,
if (!Index)
return std::nullopt;
- QualType ElemType = CurRegion->getElementType();
-
- // FIXME: The following early return was presumably added to safeguard the
- // getTypeSizeInChars() call (which doesn't accept an incomplete type), but
- // it seems that `ElemType` cannot be incomplete at this point.
- if (ElemType->isIncompleteType())
- return std::nullopt;
-
// Calculate Delta = Index * sizeof(ElemType).
- NonLoc Size = SVB.makeArrayIndex(
- SVB.getContext().getTypeSizeInChars(ElemType).getQuantity());
+ NonLoc Size = SVB.makeArrayIndex(getElementSize(CurRegion, SVB));
auto Delta = EvalBinOp(BO_Mul, *Index, Size);
if (!Delta)
return std::nullopt;
@@ -452,7 +451,8 @@ void ArrayBoundChecker::handleAccessExpr(const Expr *E,
bounds::CheckFlags Flags = {
/*CheckUnderflow=*/!(isa<SymbolicRegion>(Reg) &&
isa<UnknownSpaceRegion>(Space)),
- /*OffsetObviouslyNonnegative=*/isOffsetObviouslyNonnegative(E, C)};
+ /*OffsetObviouslyNonnegative=*/isOffsetObviouslyNonnegative(E, C),
+ /*AlsoAcceptEquality=*/(getElementSize(AccessedER, SVB) == 0)};
bounds::CheckResult Res = checkBounds(State, SVB, ByteOffset, Extent, Flags);
@@ -472,7 +472,7 @@ void ArrayBoundChecker::handleAccessExpr(const Expr *E,
// forms the past-the-end pointer without actually dereferencing it.
auto [EqualsToThreshold, NotEqualToThreshold] =
bounds::compareValueToThreshold(State, SVB, ByteOffset, *Extent,
- /*CheckEquality=*/true);
+ bounds::Comparison::EQ);
if (EqualsToThreshold && !NotEqualToThreshold) {
C.addTransition(EqualsToThreshold);
return;
diff --git a/clang/lib/StaticAnalyzer/Checkers/BoundsChecking.cpp b/clang/lib/StaticAnalyzer/Checkers/BoundsChecking.cpp
index 9c11e9e2bd69b..55b32447d870a 100644
--- a/clang/lib/StaticAnalyzer/Checkers/BoundsChecking.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/BoundsChecking.cpp
@@ -77,7 +77,7 @@ static bool isUnsigned(SValBuilder &SVB, NonLoc Value) {
std::pair<ProgramStateRef, ProgramStateRef>
bounds::compareValueToThreshold(ProgramStateRef State, SValBuilder &SVB,
NonLoc Value, NonLoc Threshold,
- bool CheckEquality) {
+ Comparison CmpKind) {
if (auto ConcreteThreshold = Threshold.getAs<nonloc::ConcreteInt>()) {
std::tie(Value, Threshold) =
getSimplifiedOffsets(Value, *ConcreteThreshold, SVB);
@@ -91,16 +91,16 @@ bounds::compareValueToThreshold(ProgramStateRef State, SValBuilder &SVB,
// To avoid automatic conversions, we evaluate the "obvious" cases without
// calling `evalBinOpNN`:
if (isNegative(SVB, State, Value) && isUnsigned(SVB, Threshold)) {
- if (CheckEquality) {
- // negative_value == unsigned_threshold is always false
+ if (CmpKind == Comparison::EQ) {
+ // negative == unsigned is always false
return {nullptr, State};
}
- // negative_value < unsigned_threshold is always true
+ // negative < unsigned and negative <= unsigned are always true
return {State, nullptr};
}
if (isUnsigned(SVB, Value) && isNegative(SVB, State, Threshold)) {
- // unsigned_value == negative_threshold and
- // unsigned_value < negative_threshold are both always false
+ // unsigned == negative, unsigned < negative and unsigned <= negative are
+ // all always false
return {nullptr, State};
}
// FIXME: These special cases are sufficient for handling real-world
@@ -114,7 +114,7 @@ bounds::compareValueToThreshold(ProgramStateRef State, SValBuilder &SVB,
// evaluate these "mathematical" comparisons through a separate pathway would
// be a step backwards in this sense.
- const BinaryOperatorKind OpKind = CheckEquality ? BO_EQ : BO_LT;
+ const BinaryOperatorKind OpKind = asOpcode(CmpKind);
auto BelowThreshold =
SVB.evalBinOpNN(State, OpKind, Value, Threshold, SVB.getConditionType())
.getAs<NonLoc>();
@@ -188,9 +188,9 @@ bounds::CheckResult bounds::checkBounds(ProgramStateRef State, SValBuilder &SVB,
// checker will first assume that the offset is non-negative, and then
// (with this additional assumption) it will detect an overflow error.
// In this situation the warning message should mention both possibilities.
-
+ Comparison CK = Flags.AlsoAcceptEquality ? Comparison::LE : Comparison::LT;
auto [WithinUpperBound, ExceedsUpperBound] =
- compareValueToThreshold(State, SVB, Offset, *Extent);
+ compareValueToThreshold(State, SVB, Offset, *Extent, /*CmpKind=*/CK);
if (ExceedsUpperBound) {
// The offset may be invalid (>= Size)...
diff --git a/clang/test/Analysis/ArrayBound/verbose-tests.c b/clang/test/Analysis/ArrayBound/verbose-tests.c
index 96f4d29ae0777..a8aada2f7e8f5 100644
--- a/clang/test/Analysis/ArrayBound/verbose-tests.c
+++ b/clang/test/Analysis/ArrayBound/verbose-tests.c
@@ -475,9 +475,41 @@ struct Empty {};
struct Empty ZeroSizeElements[10];
struct Empty zeroSizeElements(void) {
- // FIXME: We probably shouldn't report this access.
- return ZeroSizeElements[5];
+ // Previously this had produced the false positive warning {{Access of
+ // 'ZeroSizeElements' at byte offset 0, while it holds only 0 bytes}}.
+ return ZeroSizeElements[5]; // no-warning
+}
+
+struct Empty zeroSizeElementsNegativeIndex(void) {
+ // The negative index does not change anything, it still means offset = 0.
+ return ZeroSizeElements[-5]; // no-warning
+}
+
+int zeroSizeContainerIntAccess(void) {
+ return ((int*)ZeroSizeElements)[5];
// expected-warning at -1 {{Out of bound access to memory after the end of 'ZeroSizeElements'}}
- // expected-note at -2 {{Access of 'ZeroSizeElements' at byte offset 0, while it holds only 0 bytes}}
+ // expected-note at -2 {{Access of 'ZeroSizeElements' at index 5, while it holds only 0 'int' elements}}
}
+
+struct Empty zeroSizeAccessOfPastTheEnd(void) {
+ // We currently allow zero-sized access of past-the-end pointers as a side
+ // effect of the logic that handles the testcase 'zeroSizeElements'.
+ return *(struct Empty *)(TenElements + 10); // no-warning
+}
+
+struct Empty zeroSizeAccessFarAway(void) {
+ // However, zero-sized access of other out-of-bounds pointers is reported
+ // (with byte offsets, because the zero-sized element is not suitable for
+ // calculating indices).
+ return *(struct Empty *)(TenElements + 20);
+ // expected-warning at -1 {{Out of bound access to memory after the end of 'TenElements'}}
+ // expected-note at -2 {{Access of 'TenElements' at byte offset 80, while it holds only 40 bytes}}
+}
+
+struct Empty zeroSizeAccessUnderflow(void) {
+ return *(struct Empty *)(TenElements - 10);
+ // expected-warning at -1 {{Out of bound access to memory preceding 'TenElements'}}
+ // expected-note at -2 {{Access of 'TenElements' at negative byte offset -40}}
+}
+
#endif
>From ae5ac116d6e0e177a450ed0322ee899b7bc26378 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <donat.nagy at ericsson.com>
Date: Tue, 25 Aug 2026 17:24:48 +0200
Subject: [PATCH 3/4] [side] Delete an obsolete comment block
---
clang/lib/StaticAnalyzer/Checkers/BoundsChecking.cpp | 5 -----
1 file changed, 5 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/BoundsChecking.cpp b/clang/lib/StaticAnalyzer/Checkers/BoundsChecking.cpp
index 55b32447d870a..416fa1c4c4d3f 100644
--- a/clang/lib/StaticAnalyzer/Checkers/BoundsChecking.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/BoundsChecking.cpp
@@ -183,11 +183,6 @@ bounds::CheckResult bounds::checkBounds(ProgramStateRef State, SValBuilder &SVB,
// CHECK UPPER BOUND
if (Extent) {
- // In a situation where both underflow and overflow are possible (but the
- // index is either tainted or known to be invalid), the logic of this
- // checker will first assume that the offset is non-negative, and then
- // (with this additional assumption) it will detect an overflow error.
- // In this situation the warning message should mention both possibilities.
Comparison CK = Flags.AlsoAcceptEquality ? Comparison::LE : Comparison::LT;
auto [WithinUpperBound, ExceedsUpperBound] =
compareValueToThreshold(State, SVB, Offset, *Extent, /*CmpKind=*/CK);
>From 98b7635097293c436500652e4a687bda6f54fd63 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <donat.nagy at ericsson.com>
Date: Wed, 26 Aug 2026 15:03:07 +0200
Subject: [PATCH 4/4] Improve comments in tests
---
clang/test/Analysis/ArrayBound/verbose-tests.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/clang/test/Analysis/ArrayBound/verbose-tests.c b/clang/test/Analysis/ArrayBound/verbose-tests.c
index a8aada2f7e8f5..29e80947d2f54 100644
--- a/clang/test/Analysis/ArrayBound/verbose-tests.c
+++ b/clang/test/Analysis/ArrayBound/verbose-tests.c
@@ -470,13 +470,15 @@ int *nothingIsCertain(int x, int y) {
// We disable this test under Windows because 'struct Empty {}' has a nozero
// size on that platform. Note that '_WIN32' is also defined on 64-bit systems
// and is apparently the customary way to detect Windows OS.
+// The empty struct also has nonzero size under C++ so these corner cases are
+// only relevant under C.
struct Empty {};
struct Empty ZeroSizeElements[10];
struct Empty zeroSizeElements(void) {
- // Previously this had produced the false positive warning {{Access of
- // 'ZeroSizeElements' at byte offset 0, while it holds only 0 bytes}}.
+ // Here the offset and extent are both 0, which previously caused a false
+ // positive out of bounds report.
return ZeroSizeElements[5]; // no-warning
}
More information about the cfe-commits
mailing list