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?= <[email protected]> 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@-1 {{Out of bound access to memory after the end of 'ZeroSizeElements'}} - // expected-note@-2 {{Access of 'ZeroSizeElements' at byte offset 0, while it holds only 0 byte}} + // expected-note@-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?= <[email protected]> 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@-1 {{Out of bound access to memory after the end of 'ZeroSizeElements'}} - // expected-note@-2 {{Access of 'ZeroSizeElements' at byte offset 0, while it holds only 0 bytes}} + // expected-note@-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@-1 {{Out of bound access to memory after the end of 'TenElements'}} + // expected-note@-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@-1 {{Out of bound access to memory preceding 'TenElements'}} + // expected-note@-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?= <[email protected]> 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?= <[email protected]> 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 } _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
