https://github.com/NagyDonat created https://github.com/llvm/llvm-project/pull/214540
The checker `security.ArrayBound` had a special case for not reporting the `&array[size]` expressions where `size` is equal to the the element count of `array`. (Note that `array[size]` is reported as out-of-bounds access, but `&array[size]` does not actually access the past-the end location and it is a somewhat idiomatic way of expressing the past-the-end pointer.) The primary goal of this change is that it simplifies the contract of `bounds::checkBounds` which previously had a flag to act as if forming this past-the-end pointer was valid in-bounds access. With the new implementation, `checkBounds()` diagnoses the `&array[size]` expression as out-of-bounds access, then `ArrayBoundChecker.cpp` postprocesses this result and suppresses the report when it detects that it was an idiomatic `&array[size]` expression. This also removes the ugly but practically irrelevant corner case: the old code for recognizing an idiomatic past-the-end pointer expression also accepted it when the index (`size`) was constrained as "either negative, or the element count of the array" (becasue the "assume lower bound" step happens before checking the upper bound). (The new code only accepts `&array[size]` as an idiomatic past-the end pointer if `size` is constrained to be equal to the element count.) From 148535d6617f71b5034d97cd9b2d6860aa1541a3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Thu, 6 Aug 2026 19:26:03 +0200 Subject: [PATCH] [analyzer] Improve handling of &array[size] in ArrayBound The checker `security.ArrayBound` had a special case for not reporting the `&array[size]` expressions where `size` is equal to the the element count of `array`. (Note that `array[size]` is reported as out-of-bounds access, but `&array[size]` does not actually access the past-the end location and it is a somewhat idiomatic way of expressing the past-the-end pointer.) The primary goal of this change is that it simplifies the contract of `bounds::checkBounds` which previously had a flag to act as if forming this past-the-end pointer was valid in-bounds access. With the new implementation, `checkBounds()` diagnoses the `&array[size]` expression as out-of-bounds access, then `ArrayBoundChecker.cpp` postprocesses this result and suppresses the report when it detects that it was an idiomatic `&array[size]` expression. This also removes the ugly but practically irrelevant corner case: the old code for recognizing an idiomatic past-the-end pointer expression also accepted it when the index (`size`) was constrained as "either negative, or the element count of the array" (becasue the "assume lower bound" step happens before checking the upper bound). (The new code only accepts `&array[size]` as an idiomatic past-the end pointer if `size` is constrained to be equal to the element count.) --- .../StaticAnalyzer/Checkers/BoundsChecking.h | 21 +++++++------ .../Checkers/ArrayBoundChecker.cpp | 22 +++++++++---- .../Checkers/BoundsChecking.cpp | 31 +++++-------------- 3 files changed, 34 insertions(+), 40 deletions(-) diff --git a/clang/include/clang/StaticAnalyzer/Checkers/BoundsChecking.h b/clang/include/clang/StaticAnalyzer/Checkers/BoundsChecking.h index 3e9a639e36cef..2c8469694b661 100644 --- a/clang/include/clang/StaticAnalyzer/Checkers/BoundsChecking.h +++ b/clang/include/clang/StaticAnalyzer/Checkers/BoundsChecking.h @@ -29,15 +29,12 @@ namespace clang::ento::bounds { struct CheckFlags { unsigned CheckUnderflow : 1; unsigned OffsetObviouslyNonnegative : 1; - unsigned AcceptPastTheEnd : 1; }; class CheckResult; /// Checks the validity of accessing a memory region with extent \p Extent at -/// offset \p Offset. The \p Flags influence the semantics of the check, in -/// particular if `AcceptPastTheEnd` is true, then Offset == Extent is also -/// accepted as valid. +/// offset \p Offset. The \p Flags influence the semantics of the check. CheckResult checkBounds(ProgramStateRef State, SValBuilder &SVB, NonLoc Offset, std::optional<NonLoc> Extent, CheckFlags Flags); @@ -52,15 +49,11 @@ class CheckResult { bool isCorruptedState() const { return IsCorruptedState; } /// When true, the checked offset may be in bounds. - /// As an exceptional case, this is also true for idiomatic expressions that - /// define a past-the-end pointer (and do not dereference it). bool mayBeInBounds() const { return static_cast<bool>(InBoundsState); } /// When true, the checked offset may be negative. bool mayUnderflow() const { return MayUnderflow; } /// When true, the checked offset may be >= the extent of the region. - /// As an exceptional case, this is also false for idiomatic expressions that - /// define a past-the-end pointer (and do not dereference it). bool mayOverflow() const { return ExtentIfMayOverflow.has_value(); } /// When true, the checked offset may be out of bounds. bool mayBeInvalid() const { return MayUnderflow || ExtentIfMayOverflow; } @@ -78,8 +71,6 @@ class CheckResult { /// Returns the program state that should be used for continuing the analysis /// after this bounds check. This returns null if mayBeInBounds() is false, in /// that case the state before the check should be used in the error node. - /// Note that we also have a valid state in the exception case when the - /// 'access' calculates the past-the-end pointer without dereferencing it. ProgramStateRef getInBoundsState() const { return InBoundsState; } friend CheckResult checkBounds(ProgramStateRef State, SValBuilder &SVB, @@ -100,6 +91,16 @@ class CheckResult { ProgramStateRef InBoundsState = nullptr; }; +// Evaluate the comparison Value < 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. +std::pair<ProgramStateRef, ProgramStateRef> +compareValueToThreshold(ProgramStateRef State, SValBuilder &SVB, NonLoc Value, + NonLoc Threshold, bool CheckEquality = false); } // 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 897b3b22c5bb8..2afa9e7122480 100644 --- a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp @@ -411,6 +411,7 @@ static std::string getAssumptionNote(bounds::CheckResult Res, void ArrayBoundChecker::handleAccessExpr(const Expr *E, CheckerContext &C) const { + ASTContext &ACtx = C.getASTContext(); const SVal Location = C.getSVal(E); // The header ctype.h (from e.g. glibc) implements the isXXXXX() macros as @@ -418,7 +419,7 @@ void ArrayBoundChecker::handleAccessExpr(const Expr *E, // and incomplete analysis of these leads to false positives. As even // accurate reports would be confusing for the users, just disable reports // from these macros: - if (isFromCtypeMacro(E, C.getASTContext())) + if (isFromCtypeMacro(E, ACtx)) return; ProgramStateRef State = C.getState(); @@ -446,10 +447,7 @@ void ArrayBoundChecker::handleAccessExpr(const Expr *E, bounds::CheckFlags Flags = { /*CheckUnderflow=*/!(isa<SymbolicRegion>(Reg) && isa<UnknownSpaceRegion>(Space)), - /*OffsetObviouslyNonnegative=*/isOffsetObviouslyNonnegative(E, C), - /*AcceptPastTheEnd=*/isa<ArraySubscriptExpr>(E) && - isInAddressOf(E, C.getASTContext()), - }; + /*OffsetObviouslyNonnegative=*/isOffsetObviouslyNonnegative(E, C)}; bounds::CheckResult Res = checkBounds(State, SVB, ByteOffset, Extent, Flags); @@ -464,7 +462,19 @@ void ArrayBoundChecker::handleAccessExpr(const Expr *E, const NoteTag *T = nullptr; if (Res.mayBeInvalid()) { if (!Res.mayBeInBounds()) { - SizeUnit SU = SizeUnit::forSVal(Location, C.getASTContext()); + if (isa<ArraySubscriptExpr>(E) && isInAddressOf(E, ACtx) && Extent) { + // Recognize and accept the idiomatic `&array[size]` expression that + // forms the past-the-end pointer without actually dereferencing it. + auto [EqualsToThreshold, NotEqualToThreshold] = + bounds::compareValueToThreshold(State, SVB, ByteOffset, *Extent, + /*CheckEquality=*/true); + if (EqualsToThreshold && !NotEqualToThreshold) { + C.addTransition(EqualsToThreshold, T); + return; + } + } + + SizeUnit SU = SizeUnit::forSVal(Location, ACtx); BugDescription Desc = describeInvalidAccess(Res, RegName, SU); reportOOB(C, State, Desc, ByteOffset, Res.getExtentIfMayOverflow()); return; diff --git a/clang/lib/StaticAnalyzer/Checkers/BoundsChecking.cpp b/clang/lib/StaticAnalyzer/Checkers/BoundsChecking.cpp index 52086254fff3a..e368931d9df51 100644 --- a/clang/lib/StaticAnalyzer/Checkers/BoundsChecking.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/BoundsChecking.cpp @@ -76,16 +76,10 @@ static bool isUnsigned(SValBuilder &SVB, NonLoc Value) { return T->isUnsignedIntegerType(); } -// Evaluate the comparison Value < Threshold with the help of the custom -// simplification algorithm defined for this checker. 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. -static std::pair<ProgramStateRef, ProgramStateRef> -compareValueToThreshold(ProgramStateRef State, NonLoc Value, NonLoc Threshold, - SValBuilder &SVB, bool CheckEquality = false) { +std::pair<ProgramStateRef, ProgramStateRef> +bounds::compareValueToThreshold(ProgramStateRef State, SValBuilder &SVB, + NonLoc Value, NonLoc Threshold, + bool CheckEquality) { if (auto ConcreteThreshold = Threshold.getAs<nonloc::ConcreteInt>()) { std::tie(Value, Threshold) = getSimplifiedOffsets(Value, *ConcreteThreshold, SVB); @@ -143,7 +137,7 @@ bounds::CheckResult bounds::checkBounds(ProgramStateRef State, SValBuilder &SVB, // CHECK LOWER BOUND if (Flags.CheckUnderflow) { auto [PrecedesLowerBound, WithinLowerBound] = - compareValueToThreshold(State, Offset, SVB.makeZeroArrayIndex(), SVB); + compareValueToThreshold(State, SVB, Offset, SVB.makeZeroArrayIndex()); if (PrecedesLowerBound) { // The analyzer thinks that the offset may be invalid (negative)... @@ -198,25 +192,14 @@ bounds::CheckResult bounds::checkBounds(ProgramStateRef State, SValBuilder &SVB, // In this situation the warning message should mention both possibilities. auto [WithinUpperBound, ExceedsUpperBound] = - compareValueToThreshold(State, Offset, *Extent, SVB); + compareValueToThreshold(State, SVB, Offset, *Extent); if (ExceedsUpperBound) { // The offset may be invalid (>= Size)... Res.ExtentIfMayOverflow = Extent; if (!WithinUpperBound) { - // ...and it cannot be within bounds, so report an error, unless we can - // definitely determine that this is an idiomatic `&array[size]` - // expression that calculates the past-the-end pointer. - if (Flags.AcceptPastTheEnd) { - auto [EqualsToThreshold, NotEqualToThreshold] = - compareValueToThreshold(State, Offset, *Extent, SVB, - /*CheckEquality=*/true); - if (EqualsToThreshold && !NotEqualToThreshold) { - Res.ExtentIfMayOverflow = std::nullopt; - Res.InBoundsState = EqualsToThreshold; - } - } + // ...and it cannot be within bounds. return Res; } } _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
