https://github.com/NagyDonat updated https://github.com/llvm/llvm-project/pull/210774
From 8c23f5f8063c0f49fa3c90f06d6b69f7219f395f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Wed, 3 Jun 2026 13:31:30 +0200 Subject: [PATCH 01/22] Only take the name of the region in StateUpdateReporter constructor It does not need to know about the region object. --- .../StaticAnalyzer/Checkers/ArrayBoundChecker.cpp | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp index 67110f021bc56..6b12fa6717fbf 100644 --- a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp @@ -77,8 +77,7 @@ determineElementSize(const std::optional<QualType> T, const CheckerContext &C) { } class StateUpdateReporter { - const MemSpaceRegion *Space; - const SubRegion *Reg; + const std::string RegName; const NonLoc ByteOffsetVal; const std::optional<QualType> ElementType; const std::optional<int64_t> ElementSize; @@ -86,10 +85,10 @@ class StateUpdateReporter { std::optional<NonLoc> AssumedUpperBound = std::nullopt; public: - StateUpdateReporter(const SubRegion *R, NonLoc ByteOffsVal, const Expr *E, + StateUpdateReporter(std::string RN, NonLoc ByteOffsVal, const Expr *E, CheckerContext &C) - : Space(R->getMemorySpace(C.getState())), Reg(R), - ByteOffsetVal(ByteOffsVal), ElementType(determineElementType(E, C)), + : RegName(RN), ByteOffsetVal(ByteOffsVal), + ElementType(determineElementType(E, C)), ElementSize(determineElementSize(ElementType, C)) {} void recordNonNegativeAssumption() { AssumedNonNegative = true; } @@ -550,7 +549,7 @@ std::string StateUpdateReporter::getMessage(PathSensitiveBugReport &BR) const { << "' elements in "; else Out << "the extent of "; - Out << getRegionName(Space, Reg); + Out << RegName; } return std::string(Out.str()); } @@ -597,7 +596,8 @@ void ArrayBoundChecker::performCheck(const Expr *E, CheckerContext &C) const { // The state updates will be reported as a single note tag, which will be // composed by this helper class. - StateUpdateReporter SUR(Reg, ByteOffset, E, C); + std::string RegName = getRegionName(Reg->getMemorySpace(C.getState()), Reg); + StateUpdateReporter SUR(RegName, ByteOffset, E, C); // CHECK LOWER BOUND const MemSpaceRegion *Space = Reg->getMemorySpace(State); From 2f961f8d3e136dfb8627827dd4aa9030e980ef01 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Wed, 3 Jun 2026 14:52:33 +0200 Subject: [PATCH 02/22] Move note creation out of StateUpdateReporter I'm gradually transforming this class into a generic utility, taking away some responsibilities and then adding other responsibilities. --- .../Checkers/ArrayBoundChecker.cpp | 46 +++++++++---------- 1 file changed, 22 insertions(+), 24 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp index 6b12fa6717fbf..750d79993c5b7 100644 --- a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp @@ -77,7 +77,6 @@ determineElementSize(const std::optional<QualType> T, const CheckerContext &C) { } class StateUpdateReporter { - const std::string RegName; const NonLoc ByteOffsetVal; const std::optional<QualType> ElementType; const std::optional<int64_t> ElementSize; @@ -85,10 +84,8 @@ class StateUpdateReporter { std::optional<NonLoc> AssumedUpperBound = std::nullopt; public: - StateUpdateReporter(std::string RN, NonLoc ByteOffsVal, const Expr *E, - CheckerContext &C) - : RegName(RN), ByteOffsetVal(ByteOffsVal), - ElementType(determineElementType(E, C)), + StateUpdateReporter(NonLoc ByteOffsVal, const Expr *E, CheckerContext &C) + : ByteOffsetVal(ByteOffsVal), ElementType(determineElementType(E, C)), ElementSize(determineElementSize(ElementType, C)) {} void recordNonNegativeAssumption() { AssumedNonNegative = true; } @@ -98,11 +95,11 @@ class StateUpdateReporter { bool assumedNonNegative() { return AssumedNonNegative; } - const NoteTag *createNoteTag(CheckerContext &C) const; + bool hasAssumption() const { return AssumedNonNegative || AssumedUpperBound; } -private: - std::string getMessage(PathSensitiveBugReport &BR) const; + std::string getMessage(PathSensitiveBugReport &BR, StringRef RegName) const; +private: /// Return true if information about the value of `Sym` can put constraints /// on some symbol which is interesting within the bug report `BR`. /// In particular, this returns true when `Sym` is interesting within `BR`; @@ -488,17 +485,8 @@ static Messages getTaintMsgs(const MemSpaceRegion *Space, AlsoMentionUnderflow ? "negative or " : "")}; } -const NoteTag *StateUpdateReporter::createNoteTag(CheckerContext &C) const { - // Don't create a note tag if we didn't assume anything: - if (!AssumedNonNegative && !AssumedUpperBound) - return nullptr; - - return C.getNoteTag([*this](PathSensitiveBugReport &BR) -> std::string { - return getMessage(BR); - }); -} - -std::string StateUpdateReporter::getMessage(PathSensitiveBugReport &BR) const { +std::string StateUpdateReporter::getMessage(PathSensitiveBugReport &BR, + StringRef RegName) const { bool ShouldReportNonNegative = AssumedNonNegative; if (!providesInformationAboutInteresting(ByteOffsetVal, BR)) { if (AssumedUpperBound && @@ -596,8 +584,7 @@ void ArrayBoundChecker::performCheck(const Expr *E, CheckerContext &C) const { // The state updates will be reported as a single note tag, which will be // composed by this helper class. - std::string RegName = getRegionName(Reg->getMemorySpace(C.getState()), Reg); - StateUpdateReporter SUR(RegName, ByteOffset, E, C); + StateUpdateReporter SUR(ByteOffset, E, C); // CHECK LOWER BOUND const MemSpaceRegion *Space = Reg->getMemorySpace(State); @@ -683,8 +670,9 @@ void ArrayBoundChecker::performCheck(const Expr *E, CheckerContext &C) const { // expression that calculates the past-the-end pointer. if (isIdiomaticPastTheEndPtr(E, ExceedsUpperBound, ByteOffset, *KnownSize, C)) { - C.addTransition(ExceedsUpperBound, SUR.createNoteTag(C)); - return; + // The use of 'goto' is a temporary solution, will be eliminated in + // the next steps of the refactoring. + goto NormalTransition; } BadOffsetKind Problem = AlsoMentionUnderflow @@ -726,8 +714,18 @@ void ArrayBoundChecker::performCheck(const Expr *E, CheckerContext &C) const { State = WithinUpperBound; } +NormalTransition: // Add a transition, reporting the state updates that we accumulated. - C.addTransition(State, SUR.createNoteTag(C)); + const NoteTag *Tag = nullptr; + + if (SUR.hasAssumption()) { + std::string RN = getRegionName(Reg->getMemorySpace(C.getState()), Reg); + Tag = C.getNoteTag([SUR, RN](PathSensitiveBugReport &BR) -> std::string { + return SUR.getMessage(BR, RN); + }); + } + + C.addTransition(State, Tag); } void ArrayBoundChecker::markPartsInteresting(PathSensitiveBugReport &BR, From 80fcb6fdfa20720311087b59c51dbd46294b1466 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Wed, 3 Jun 2026 15:31:48 +0200 Subject: [PATCH 03/22] Introduce the helper class SizeUnit This is currently used for the reporting of assumptions, but in follow-up commits I will use it to unify the reporting of assumptions and overflow reports. --- .../Checkers/ArrayBoundChecker.cpp | 81 +++++++++++-------- 1 file changed, 46 insertions(+), 35 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp index 750d79993c5b7..5418fde506bfd 100644 --- a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp @@ -57,36 +57,49 @@ getAsCleanArraySubscriptExpr(const Expr *E, const CheckerContext &C) { return ASE; } -/// If `E` is a "clean" array subscript expression, return the type of the -/// accessed element; otherwise return std::nullopt because that's the best (or -/// least bad) option for the diagnostic generation that relies on this. -static std::optional<QualType> determineElementType(const Expr *E, - const CheckerContext &C) { - const auto *ASE = getAsCleanArraySubscriptExpr(E, C); - if (!ASE) - return std::nullopt; +class SizeUnit { + QualType AsType; + int64_t AsCharUnits; - return ASE->getType(); -} + SizeUnit() : AsType(), AsCharUnits(1) {} -static std::optional<int64_t> -determineElementSize(const std::optional<QualType> T, const CheckerContext &C) { - if (!T) - return std::nullopt; - return C.getASTContext().getTypeSizeInChars(*T).getQuantity(); -} +public: + SizeUnit(QualType T, const ASTContext &ACtx) + : AsType(T), AsCharUnits(ACtx.getTypeSizeInChars(T).getQuantity()) { + assert(!T.isNull()); + } + + static SizeUnit bytes() { return SizeUnit(); } + + bool isBytes() const { return AsType.isNull(); } + + /// If `E` is a "clean" array subscript expression, return the type of the + /// accessed element; otherwise return 'Bytes' because that's the best (or + /// least bad) option for the diagnostic generation that relies on this. + static SizeUnit forExpr(const Expr *E, const CheckerContext &C) { + const auto *ASE = getAsCleanArraySubscriptExpr(E, C); + if (!ASE) + return bytes(); + + return SizeUnit(ASE->getType(), C.getASTContext()); + } + + int64_t asCharUnits() const { return AsCharUnits; } + + std::string asExtentDesc(bool ForceBytes) const { + if (ForceBytes || isBytes()) + return "the extent of"; + return formatv("the number of '{0}' elements in", AsType.getAsString()); + } +}; class StateUpdateReporter { const NonLoc ByteOffsetVal; - const std::optional<QualType> ElementType; - const std::optional<int64_t> ElementSize; bool AssumedNonNegative = false; std::optional<NonLoc> AssumedUpperBound = std::nullopt; public: - StateUpdateReporter(NonLoc ByteOffsVal, const Expr *E, CheckerContext &C) - : ByteOffsetVal(ByteOffsVal), ElementType(determineElementType(E, C)), - ElementSize(determineElementSize(ElementType, C)) {} + StateUpdateReporter(NonLoc ByteOffsVal) : ByteOffsetVal(ByteOffsVal) {} void recordNonNegativeAssumption() { AssumedNonNegative = true; } void recordUpperBoundAssumption(NonLoc UpperBoundVal) { @@ -97,7 +110,8 @@ class StateUpdateReporter { bool hasAssumption() const { return AssumedNonNegative || AssumedUpperBound; } - std::string getMessage(PathSensitiveBugReport &BR, StringRef RegName) const; + std::string getMessage(PathSensitiveBugReport &BR, StringRef RegName, + SizeUnit SU) const; private: /// Return true if information about the value of `Sym` can put constraints @@ -486,7 +500,8 @@ static Messages getTaintMsgs(const MemSpaceRegion *Space, } std::string StateUpdateReporter::getMessage(PathSensitiveBugReport &BR, - StringRef RegName) const { + StringRef RegName, + SizeUnit SU) const { bool ShouldReportNonNegative = AssumedNonNegative; if (!providesInformationAboutInteresting(ByteOffsetVal, BR)) { if (AssumedUpperBound && @@ -505,7 +520,7 @@ std::string StateUpdateReporter::getMessage(PathSensitiveBugReport &BR, std::optional<int64_t> ExtentN = getConcreteValue(AssumedUpperBound); const bool UseIndex = - ElementSize && tryDividePair(OffsetN, ExtentN, *ElementSize); + !SU.isBytes() && tryDividePair(OffsetN, ExtentN, SU.asCharUnits()); SmallString<256> Buf; llvm::raw_svector_ostream Out(Buf); @@ -532,12 +547,7 @@ std::string StateUpdateReporter::getMessage(PathSensitiveBugReport &BR, Out << " less than "; if (ExtentN) Out << *ExtentN << ", "; - if (UseIndex && ElementType) - Out << "the number of '" << ElementType->getAsString() - << "' elements in "; - else - Out << "the extent of "; - Out << RegName; + Out << SU.asExtentDesc(/*ForceBytes=*/!UseIndex) << ' ' << RegName; } return std::string(Out.str()); } @@ -584,7 +594,7 @@ void ArrayBoundChecker::performCheck(const Expr *E, CheckerContext &C) const { // The state updates will be reported as a single note tag, which will be // composed by this helper class. - StateUpdateReporter SUR(ByteOffset, E, C); + StateUpdateReporter SUR(ByteOffset); // CHECK LOWER BOUND const MemSpaceRegion *Space = Reg->getMemorySpace(State); @@ -716,16 +726,17 @@ void ArrayBoundChecker::performCheck(const Expr *E, CheckerContext &C) const { NormalTransition: // Add a transition, reporting the state updates that we accumulated. - const NoteTag *Tag = nullptr; + const NoteTag *T = nullptr; if (SUR.hasAssumption()) { std::string RN = getRegionName(Reg->getMemorySpace(C.getState()), Reg); - Tag = C.getNoteTag([SUR, RN](PathSensitiveBugReport &BR) -> std::string { - return SUR.getMessage(BR, RN); + SizeUnit SU = SizeUnit::forExpr(E, C); + T = C.getNoteTag([SUR, RN, SU](PathSensitiveBugReport &BR) -> std::string { + return SUR.getMessage(BR, RN, SU); }); } - C.addTransition(State, Tag); + C.addTransition(State, T); } void ArrayBoundChecker::markPartsInteresting(PathSensitiveBugReport &BR, From 742e589a9c0f1e1f09a1e3aa985ce8b3cf0af58b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Thu, 4 Jun 2026 11:37:33 +0200 Subject: [PATCH 04/22] Rename 'performCheck' to 'handleAccessExpr' --- .../lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp index 5418fde506bfd..843d67ce5abb3 100644 --- a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp @@ -167,7 +167,7 @@ class ArrayBoundChecker : public Checker<check::PostStmt<ArraySubscriptExpr>, BugType BT{this, "Out-of-bound access"}; BugType TaintBT{this, "Out-of-bound access", categories::TaintedData}; - void performCheck(const Expr *E, CheckerContext &C) const; + void handleAccessExpr(const Expr *E, CheckerContext &C) const; void reportOOB(CheckerContext &C, ProgramStateRef ErrorState, Messages Msgs, NonLoc Offset, std::optional<NonLoc> Extent, @@ -188,15 +188,15 @@ class ArrayBoundChecker : public Checker<check::PostStmt<ArraySubscriptExpr>, public: void checkPostStmt(const ArraySubscriptExpr *E, CheckerContext &C) const { - performCheck(E, C); + handleAccessExpr(E, C); } void checkPostStmt(const UnaryOperator *E, CheckerContext &C) const { if (E->getOpcode() == UO_Deref) - performCheck(E, C); + handleAccessExpr(E, C); } void checkPostStmt(const MemberExpr *E, CheckerContext &C) const { if (E->isArrow()) - performCheck(E->getBase(), C); + handleAccessExpr(E->getBase(), C); } }; @@ -570,7 +570,8 @@ bool StateUpdateReporter::providesInformationAboutInteresting( return false; } -void ArrayBoundChecker::performCheck(const Expr *E, CheckerContext &C) const { +void ArrayBoundChecker::handleAccessExpr(const Expr *E, + CheckerContext &C) const { const SVal Location = C.getSVal(E); // The header ctype.h (from e.g. glibc) implements the isXXXXX() macros as From 08c957bf21e2e553d4afe52c57859cb4ff054897 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Wed, 8 Jul 2026 19:11:38 +0200 Subject: [PATCH 05/22] Rename StateUpdateReporter to CheckResult This is now a small utility class, but will be generalized through several steps to turn into a general "result of a bounds check" type. --- .../Checkers/ArrayBoundChecker.cpp | 25 +++++++++---------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp index 843d67ce5abb3..5779acce9dc8c 100644 --- a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp @@ -93,13 +93,13 @@ class SizeUnit { } }; -class StateUpdateReporter { +class CheckResult { const NonLoc ByteOffsetVal; bool AssumedNonNegative = false; std::optional<NonLoc> AssumedUpperBound = std::nullopt; public: - StateUpdateReporter(NonLoc ByteOffsVal) : ByteOffsetVal(ByteOffsVal) {} + CheckResult(NonLoc ByteOffsVal) : ByteOffsetVal(ByteOffsVal) {} void recordNonNegativeAssumption() { AssumedNonNegative = true; } void recordUpperBoundAssumption(NonLoc UpperBoundVal) { @@ -499,9 +499,8 @@ static Messages getTaintMsgs(const MemSpaceRegion *Space, AlsoMentionUnderflow ? "negative or " : "")}; } -std::string StateUpdateReporter::getMessage(PathSensitiveBugReport &BR, - StringRef RegName, - SizeUnit SU) const { +std::string CheckResult::getMessage(PathSensitiveBugReport &BR, + StringRef RegName, SizeUnit SU) const { bool ShouldReportNonNegative = AssumedNonNegative; if (!providesInformationAboutInteresting(ByteOffsetVal, BR)) { if (AssumedUpperBound && @@ -552,7 +551,7 @@ std::string StateUpdateReporter::getMessage(PathSensitiveBugReport &BR, return std::string(Out.str()); } -bool StateUpdateReporter::providesInformationAboutInteresting( +bool CheckResult::providesInformationAboutInteresting( SymbolRef Sym, PathSensitiveBugReport &BR) { if (!Sym) return false; @@ -595,7 +594,7 @@ void ArrayBoundChecker::handleAccessExpr(const Expr *E, // The state updates will be reported as a single note tag, which will be // composed by this helper class. - StateUpdateReporter SUR(ByteOffset); + CheckResult Res(ByteOffset); // CHECK LOWER BOUND const MemSpaceRegion *Space = Reg->getMemorySpace(State); @@ -649,7 +648,7 @@ void ArrayBoundChecker::handleAccessExpr(const Expr *E, } // ...but it can be valid as well, so the checker will (optimistically) // assume that it's valid and mention this in the note tag. - SUR.recordNonNegativeAssumption(); + Res.recordNonNegativeAssumption(); } } @@ -668,7 +667,7 @@ void ArrayBoundChecker::handleAccessExpr(const Expr *E, // 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. - bool AlsoMentionUnderflow = SUR.assumedNonNegative(); + bool AlsoMentionUnderflow = Res.assumedNonNegative(); auto [WithinUpperBound, ExceedsUpperBound] = compareValueToThreshold(State, ByteOffset, *KnownSize, SVB); @@ -715,7 +714,7 @@ void ArrayBoundChecker::handleAccessExpr(const Expr *E, } // ...and it isn't tainted, so the checker will (optimistically) assume // that the offset is in bounds and mention this in the note tag. - SUR.recordUpperBoundAssumption(*KnownSize); + Res.recordUpperBoundAssumption(*KnownSize); } // Actually update the state. The "if" only fails in the extremely unlikely @@ -729,11 +728,11 @@ void ArrayBoundChecker::handleAccessExpr(const Expr *E, // Add a transition, reporting the state updates that we accumulated. const NoteTag *T = nullptr; - if (SUR.hasAssumption()) { + if (Res.hasAssumption()) { std::string RN = getRegionName(Reg->getMemorySpace(C.getState()), Reg); SizeUnit SU = SizeUnit::forExpr(E, C); - T = C.getNoteTag([SUR, RN, SU](PathSensitiveBugReport &BR) -> std::string { - return SUR.getMessage(BR, RN, SU); + T = C.getNoteTag([Res, RN, SU](PathSensitiveBugReport &BR) -> std::string { + return Res.getMessage(BR, RN, SU); }); } From 29a21c4372d6757a91b58e64a8d4539599f5268f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Thu, 9 Jul 2026 14:04:00 +0200 Subject: [PATCH 06/22] Rename members of CheckResult As of now this type is only used on the branch where valid access is feasible (and we want to produce an "assuming in bounds" note), but I want to reuse this logic in the case when valid access is infeasible (and we are composing an invalid access bug report), so I'm picking new names that would work in both situations. --- .../Checkers/ArrayBoundChecker.cpp | 56 +++++++++++-------- 1 file changed, 34 insertions(+), 22 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp index 5779acce9dc8c..8a37c1bde5b07 100644 --- a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp @@ -94,21 +94,33 @@ class SizeUnit { }; class CheckResult { - const NonLoc ByteOffsetVal; - bool AssumedNonNegative = false; - std::optional<NonLoc> AssumedUpperBound = std::nullopt; + /// Offset of the accessed location, measured from the start of the region. + /// As of now, the offset and the extent are always measured in bytes, but it + /// may be necessary to change this in the future. + const NonLoc Offset; + + /// Updated to true when we detect that underflow is feasible. + bool MayUnderflow = false; + + /// Updated to store the extent of the accessed buffer when we notice that + /// problematic overflow was feasible and therefore we either need to create + /// an overflow bug report, or need to note that we are assuming that there + /// is no overflow. Otherwise (when overflow was infeasible or this is an + /// '&array[size]' expression that idiomatically creates a past-the-end + /// pointer) this data member is std::nullopt. In these cases we do not need + /// to know the extent and its value should be ignored so it is comfortable + /// to avoid storing it. + std::optional<NonLoc> MayOverflowExtent = std::nullopt; public: - CheckResult(NonLoc ByteOffsVal) : ByteOffsetVal(ByteOffsVal) {} + CheckResult(NonLoc Offs) : Offset(Offs) {} - void recordNonNegativeAssumption() { AssumedNonNegative = true; } - void recordUpperBoundAssumption(NonLoc UpperBoundVal) { - AssumedUpperBound = UpperBoundVal; - } + void recordMayUnderflow() { MayUnderflow = true; } + void recordMayOverflowExtent(NonLoc Extent) { MayOverflowExtent = Extent; } - bool assumedNonNegative() { return AssumedNonNegative; } + bool mayUnderflow() { return MayUnderflow; } - bool hasAssumption() const { return AssumedNonNegative || AssumedUpperBound; } + bool mayBeInvalid() const { return MayUnderflow || MayOverflowExtent; } std::string getMessage(PathSensitiveBugReport &BR, StringRef RegName, SizeUnit SU) const; @@ -501,10 +513,10 @@ static Messages getTaintMsgs(const MemSpaceRegion *Space, std::string CheckResult::getMessage(PathSensitiveBugReport &BR, StringRef RegName, SizeUnit SU) const { - bool ShouldReportNonNegative = AssumedNonNegative; - if (!providesInformationAboutInteresting(ByteOffsetVal, BR)) { - if (AssumedUpperBound && - providesInformationAboutInteresting(*AssumedUpperBound, BR)) { + bool ShouldReportNonNegative = MayUnderflow; + if (!providesInformationAboutInteresting(Offset, BR)) { + if (MayOverflowExtent && + providesInformationAboutInteresting(*MayOverflowExtent, BR)) { // Even if the byte offset isn't interesting (e.g. it's a constant value), // the assumption can still be interesting if it provides information // about an interesting symbolic upper bound. @@ -515,8 +527,8 @@ std::string CheckResult::getMessage(PathSensitiveBugReport &BR, } } - std::optional<int64_t> OffsetN = getConcreteValue(ByteOffsetVal); - std::optional<int64_t> ExtentN = getConcreteValue(AssumedUpperBound); + std::optional<int64_t> OffsetN = getConcreteValue(Offset); + std::optional<int64_t> ExtentN = getConcreteValue(MayOverflowExtent); const bool UseIndex = !SU.isBytes() && tryDividePair(OffsetN, ExtentN, SU.asCharUnits()); @@ -528,7 +540,7 @@ std::string CheckResult::getMessage(PathSensitiveBugReport &BR, Out << "index "; if (OffsetN) Out << "'" << OffsetN << "' "; - } else if (AssumedUpperBound) { + } else if (MayOverflowExtent) { Out << "byte offset "; if (OffsetN) Out << "'" << OffsetN << "' "; @@ -540,7 +552,7 @@ std::string CheckResult::getMessage(PathSensitiveBugReport &BR, if (ShouldReportNonNegative) { Out << " non-negative"; } - if (AssumedUpperBound) { + if (MayOverflowExtent) { if (ShouldReportNonNegative) Out << " and"; Out << " less than "; @@ -648,7 +660,7 @@ void ArrayBoundChecker::handleAccessExpr(const Expr *E, } // ...but it can be valid as well, so the checker will (optimistically) // assume that it's valid and mention this in the note tag. - Res.recordNonNegativeAssumption(); + Res.recordMayUnderflow(); } } @@ -667,7 +679,7 @@ void ArrayBoundChecker::handleAccessExpr(const Expr *E, // 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. - bool AlsoMentionUnderflow = Res.assumedNonNegative(); + bool AlsoMentionUnderflow = Res.mayUnderflow(); auto [WithinUpperBound, ExceedsUpperBound] = compareValueToThreshold(State, ByteOffset, *KnownSize, SVB); @@ -714,7 +726,7 @@ void ArrayBoundChecker::handleAccessExpr(const Expr *E, } // ...and it isn't tainted, so the checker will (optimistically) assume // that the offset is in bounds and mention this in the note tag. - Res.recordUpperBoundAssumption(*KnownSize); + Res.recordMayOverflowExtent(*KnownSize); } // Actually update the state. The "if" only fails in the extremely unlikely @@ -728,7 +740,7 @@ void ArrayBoundChecker::handleAccessExpr(const Expr *E, // Add a transition, reporting the state updates that we accumulated. const NoteTag *T = nullptr; - if (Res.hasAssumption()) { + if (Res.mayBeInvalid()) { std::string RN = getRegionName(Reg->getMemorySpace(C.getState()), Reg); SizeUnit SU = SizeUnit::forExpr(E, C); T = C.getNoteTag([Res, RN, SU](PathSensitiveBugReport &BR) -> std::string { From 4430fdcb0e6110470e933c683ed04abbdffc0bbb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Thu, 9 Jul 2026 14:18:45 +0200 Subject: [PATCH 07/22] Simplify providesInformationAboutInteresting --- .../Checkers/ArrayBoundChecker.cpp | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp index 8a37c1bde5b07..4e0ffad76804b 100644 --- a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp @@ -126,11 +126,11 @@ class CheckResult { SizeUnit SU) const; private: - /// Return true if information about the value of `Sym` can put constraints - /// on some symbol which is interesting within the bug report `BR`. - /// In particular, this returns true when `Sym` is interesting within `BR`; - /// but it also returns true if `Sym` is an expression that contains integer - /// constants and a single symbolic operand which is interesting (in `BR`). + /// Return true if information about the value of \p SV can put constraints + /// on some symbol which is interesting within the bug report \p BR + /// In particular, this returns true when \p SV is interesting within \p BR; + /// but it also returns true if \p SV is an expression that contains integer + /// constants and a single symbolic operand which is interesting (in \p BR). /// We need to use this instead of plain `BR.isInteresting()` because if we /// are analyzing code like /// int array[10]; @@ -140,12 +140,8 @@ class CheckResult { /// then the byte offsets are `arg * 4` and `(arg + 10) * 4`, which are not /// sub-expressions of each other (but `getSimplifiedOffsets` is smart enough /// to detect this out of bounds access). - static bool providesInformationAboutInteresting(SymbolRef Sym, - PathSensitiveBugReport &BR); static bool providesInformationAboutInteresting(SVal SV, - PathSensitiveBugReport &BR) { - return providesInformationAboutInteresting(SV.getAsSymbol(), BR); - } + PathSensitiveBugReport &BR); }; struct Messages { @@ -564,7 +560,8 @@ std::string CheckResult::getMessage(PathSensitiveBugReport &BR, } bool CheckResult::providesInformationAboutInteresting( - SymbolRef Sym, PathSensitiveBugReport &BR) { + SVal SV, PathSensitiveBugReport &BR) { + SymbolRef Sym = SV.getAsSymbol(); if (!Sym) return false; for (SymbolRef PartSym : Sym->symbols()) { From 75309f946919acd62e51ee3e0855a36e6bf12121 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Thu, 9 Jul 2026 15:49:17 +0200 Subject: [PATCH 08/22] Move providesInformationAboutInteresting out of class It was a private static helper method of `CheckResult` which was only tangentially related to this class. When this class is eventually moved to a header, I don't want to see the declaration of this there. --- .../Checkers/ArrayBoundChecker.cpp | 72 +++++++++---------- 1 file changed, 34 insertions(+), 38 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp index 4e0ffad76804b..fb6b406c4f662 100644 --- a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp @@ -118,30 +118,12 @@ class CheckResult { void recordMayUnderflow() { MayUnderflow = true; } void recordMayOverflowExtent(NonLoc Extent) { MayOverflowExtent = Extent; } - bool mayUnderflow() { return MayUnderflow; } + bool mayUnderflow() const { return MayUnderflow; } bool mayBeInvalid() const { return MayUnderflow || MayOverflowExtent; } std::string getMessage(PathSensitiveBugReport &BR, StringRef RegName, SizeUnit SU) const; - -private: - /// Return true if information about the value of \p SV can put constraints - /// on some symbol which is interesting within the bug report \p BR - /// In particular, this returns true when \p SV is interesting within \p BR; - /// but it also returns true if \p SV is an expression that contains integer - /// constants and a single symbolic operand which is interesting (in \p BR). - /// We need to use this instead of plain `BR.isInteresting()` because if we - /// are analyzing code like - /// int array[10]; - /// int f(int arg) { - /// return array[arg] && array[arg + 10]; - /// } - /// then the byte offsets are `arg * 4` and `(arg + 10) * 4`, which are not - /// sub-expressions of each other (but `getSimplifiedOffsets` is smart enough - /// to detect this out of bounds access). - static bool providesInformationAboutInteresting(SVal SV, - PathSensitiveBugReport &BR); }; struct Messages { @@ -210,6 +192,39 @@ class ArrayBoundChecker : public Checker<check::PostStmt<ArraySubscriptExpr>, } // anonymous namespace +/// Return true if information about the value of \p SV can put constraints +/// on some symbol which is interesting within the bug report \p BR +/// In particular, this returns true when \p SV is interesting within \p BR; +/// but it also returns true if \p SV is an expression that contains integer +/// constants and a single symbolic operand which is interesting (in \p BR). +/// We need to use this instead of plain `BR.isInteresting()` because if we +/// are analyzing code like +/// int array[10]; +/// int f(int arg) { +/// return array[arg] && array[arg + 10]; +/// } +/// then the byte offsets are `arg * 4` and `(arg + 10) * 4`, which are not +/// sub-expressions of each other (but `getSimplifiedOffsets` is smart enough +/// to detect this out of bounds access). +static bool providesInformationAboutInteresting(SVal SV, + PathSensitiveBugReport &BR) { + SymbolRef Sym = SV.getAsSymbol(); + if (!Sym) + return false; + for (SymbolRef PartSym : Sym->symbols()) { + // The interestingess mark may appear on any layer as we're stripping off + // the SymIntExpr, UnarySymExpr etc. layers... + if (BR.isInteresting(PartSym)) + return true; + // ...but if both sides of the expression are symbolic, then there is no + // practical algorithm to produce separate constraints for the two + // operands (from the single combined result). + if (isa<SymSymExpr>(PartSym)) + return false; + } + return false; +} + /// For a given Location 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 @@ -559,25 +574,6 @@ std::string CheckResult::getMessage(PathSensitiveBugReport &BR, return std::string(Out.str()); } -bool CheckResult::providesInformationAboutInteresting( - SVal SV, PathSensitiveBugReport &BR) { - SymbolRef Sym = SV.getAsSymbol(); - if (!Sym) - return false; - for (SymbolRef PartSym : Sym->symbols()) { - // The interestingess mark may appear on any layer as we're stripping off - // the SymIntExpr, UnarySymExpr etc. layers... - if (BR.isInteresting(PartSym)) - return true; - // ...but if both sides of the expression are symbolic, then there is no - // practical algorithm to produce separate constraints for the two - // operands (from the single combined result). - if (isa<SymSymExpr>(PartSym)) - return false; - } - return false; -} - void ArrayBoundChecker::handleAccessExpr(const Expr *E, CheckerContext &C) const { const SVal Location = C.getSVal(E); From 2de10524cc2a2eba91f9c805373a506ee3e290be Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Mon, 13 Jul 2026 16:57:55 +0200 Subject: [PATCH 09/22] Hoist getRegionName from get{Non,}TaintMsgs The functions `getNonTaintMsgs` and `getTaintMsgs` took the `Region` and its memory `Space` as arguments, but only used these to calculate the user-friendly name of the region. This commit modifies them to only take the region name as a `StringRef` -- this will allow simplifications in the upcoming commits. --- .../Checkers/ArrayBoundChecker.cpp | 26 ++++++++----------- 1 file changed, 11 insertions(+), 15 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp index fb6b406c4f662..0b4a001260ffb 100644 --- a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp @@ -454,12 +454,9 @@ static bool tryDividePair(std::optional<int64_t> &Val1, return true; } -static Messages getNonTaintMsgs(const ASTContext &ACtx, - const MemSpaceRegion *Space, - const SubRegion *Region, NonLoc Offset, - std::optional<NonLoc> Extent, SVal Location, - BadOffsetKind Problem) { - std::string RegName = getRegionName(Space, Region); +static Messages getNonTaintMsgs(const ASTContext &ACtx, StringRef RegName, + NonLoc Offset, std::optional<NonLoc> Extent, + SVal Location, BadOffsetKind Problem) { const auto *EReg = Location.getAsRegion()->getAs<ElementRegion>(); assert(EReg && "this checker only handles element access"); QualType ElemType = EReg->getElementType(); @@ -511,10 +508,8 @@ static Messages getNonTaintMsgs(const ASTContext &ACtx, std::string(Buf)}; } -static Messages getTaintMsgs(const MemSpaceRegion *Space, - const SubRegion *Region, const char *OffsetName, +static Messages getTaintMsgs(StringRef RegName, const char *OffsetName, bool AlsoMentionUnderflow) { - std::string RegName = getRegionName(Space, Region); return {formatv("Potential out of bound access to {0} with tainted {1}", RegName, OffsetName), formatv("Access of {0} with a tainted {1} that may be {2}too large", @@ -645,7 +640,8 @@ void ArrayBoundChecker::handleAccessExpr(const Expr *E, } else { if (!WithinLowerBound) { // ...and it cannot be valid (>= 0), so report an error. - Messages Msgs = getNonTaintMsgs(C.getASTContext(), Space, Reg, + std::string RegName = getRegionName(Space, Reg); + Messages Msgs = getNonTaintMsgs(C.getASTContext(), RegName, ByteOffset, /*Extent=*/std::nullopt, Location, BadOffsetKind::Negative); reportOOB(C, PrecedesLowerBound, Msgs, ByteOffset, std::nullopt); @@ -693,9 +689,9 @@ void ArrayBoundChecker::handleAccessExpr(const Expr *E, BadOffsetKind Problem = AlsoMentionUnderflow ? BadOffsetKind::Indeterminate : BadOffsetKind::Overflowing; - Messages Msgs = - getNonTaintMsgs(C.getASTContext(), Space, Reg, ByteOffset, - *KnownSize, Location, Problem); + std::string RegName = getRegionName(Space, Reg); + Messages Msgs = getNonTaintMsgs(C.getASTContext(), RegName, ByteOffset, + *KnownSize, Location, Problem); reportOOB(C, ExceedsUpperBound, Msgs, ByteOffset, KnownSize); return; } @@ -711,8 +707,8 @@ void ArrayBoundChecker::handleAccessExpr(const Expr *E, if (isTainted(State, ASE->getIdx(), C.getStackFrame())) OffsetName = "index"; - Messages Msgs = - getTaintMsgs(Space, Reg, OffsetName, AlsoMentionUnderflow); + std::string RegName = getRegionName(Space, Reg); + Messages Msgs = getTaintMsgs(RegName, OffsetName, AlsoMentionUnderflow); reportOOB(C, ExceedsUpperBound, Msgs, ByteOffset, KnownSize, /*IsTaintBug=*/true); return; From e663757d5405646fa64a0ad6397ad22510727fa6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Mon, 13 Jul 2026 18:51:31 +0200 Subject: [PATCH 10/22] Refactor CheckResult, prepare for splitting handleAccessExpr This commit extends `CheckResult` with new and currently unused members. This prepares the ground for splitting `handleAccessExpr` into a new function `performCheck` (which will return a `CheckResult`) and a smaller `handleAccessExpr` (which calls `performCheck` and uses the `CheckResult` to report errors). This commit temporarily marks some implementation details of `CheckResult` as public, because I didn't see a clear way to mark the method `handleAccessExpr` as a friend of `CheckResult`. In the follow-up commit these will become private and `performCheck` will be (naturally) a friend of `CheckResult`. --- .../Checkers/ArrayBoundChecker.cpp | 75 ++++++++++++------- 1 file changed, 46 insertions(+), 29 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp index 0b4a001260ffb..b5d365521bbf5 100644 --- a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp @@ -94,36 +94,53 @@ class SizeUnit { }; class CheckResult { - /// Offset of the accessed location, measured from the start of the region. - /// As of now, the offset and the extent are always measured in bytes, but it - /// may be necessary to change this in the future. - const NonLoc Offset; - - /// Updated to true when we detect that underflow is feasible. - bool MayUnderflow = false; - - /// Updated to store the extent of the accessed buffer when we notice that - /// problematic overflow was feasible and therefore we either need to create - /// an overflow bug report, or need to note that we are assuming that there - /// is no overflow. Otherwise (when overflow was infeasible or this is an - /// '&array[size]' expression that idiomatically creates a past-the-end - /// pointer) this data member is std::nullopt. In these cases we do not need - /// to know the extent and its value should be ignored so it is comfortable - /// to avoid storing it. - std::optional<NonLoc> MayOverflowExtent = std::nullopt; - public: - CheckResult(NonLoc Offs) : Offset(Offs) {} - - void recordMayUnderflow() { MayUnderflow = true; } - void recordMayOverflowExtent(NonLoc Extent) { MayOverflowExtent = Extent; } - + /// When true, the bounds check noticed that the value of an unsigned + /// expression is constrained to negative values (because the analyzer + /// skipped the modeling of a cast expression). This execution path must be + /// discarded because it does not represent a real possibility. + /// FIXME: This hack is currently needed to filter out many ugly false + /// positives; but it should be removed when we fix cast modeling. + bool isCorruptedState() const { return IsCorruptedState; } + + /// When true, the checked offset may be in bounds. + bool mayBeValid() const { return static_cast<bool>(ValidState); } + + /// 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. + bool mayOverflow() const { return MayOverflowExtent.has_value(); } + /// When true, the checked offset may be out of bounds. bool mayBeInvalid() const { return MayUnderflow || MayOverflowExtent; } + /// Returns the program state that should be used for continuing the analysis + /// after this bounds check. This returns null if mayBeValid() is false, in + /// that case use the state before the check as the state of the error node. + ProgramStateRef getValidState() const { return ValidState; } + + /// When the access was ambiguous (that is, mayBeValid() && mayBeInvalid()), + /// returns the note "assuming in bounds" note that is relevant for the bug + /// report \p BR. When the access wasn't ambiguous or the the assumption is + /// irrelevant for \p BR, this returns the empty string (which signifies "do + /// not emit a note tag" when returned by a note tag callback). std::string getMessage(PathSensitiveBugReport &BR, StringRef RegName, SizeUnit SU) const; + +private: + // Offset of the accessed location, measured from the start of the region. + // As of now, the offset and the extent are always measured in bytes, but it + // may be necessary to change this in the future. + const NonLoc Offset; + +public: + // FIXME: The following members should be private and they will become + // private soon in a follow-up commit. + explicit CheckResult(NonLoc Offs) : Offset(Offs) {} + + bool IsCorruptedState = false; + bool MayUnderflow = false; + std::optional<NonLoc> MayOverflowExtent = std::nullopt; + ProgramStateRef ValidState = nullptr; }; struct Messages { @@ -592,12 +609,13 @@ void ArrayBoundChecker::handleAccessExpr(const Expr *E, auto [Reg, ByteOffset] = *RawOffset; + const MemSpaceRegion *Space = Reg->getMemorySpace(State); + DefinedOrUnknownSVal Extent = getDynamicExtent(State, Reg, SVB); // The state updates will be reported as a single note tag, which will be // composed by this helper class. CheckResult Res(ByteOffset); // CHECK LOWER BOUND - const MemSpaceRegion *Space = Reg->getMemorySpace(State); if (!(isa<SymbolicRegion>(Reg) && isa<UnknownSpaceRegion>(Space))) { // A symbolic region in unknown space represents an unknown pointer that // may point into the middle of an array, so we don't look for underflows. @@ -649,7 +667,7 @@ void ArrayBoundChecker::handleAccessExpr(const Expr *E, } // ...but it can be valid as well, so the checker will (optimistically) // assume that it's valid and mention this in the note tag. - Res.recordMayUnderflow(); + Res.MayUnderflow = true; } } @@ -661,8 +679,7 @@ void ArrayBoundChecker::handleAccessExpr(const Expr *E, } // CHECK UPPER BOUND - DefinedOrUnknownSVal Size = getDynamicExtent(State, Reg, SVB); - if (auto KnownSize = Size.getAs<NonLoc>()) { + if (auto KnownSize = Extent.getAs<NonLoc>()) { // 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 @@ -715,7 +732,7 @@ void ArrayBoundChecker::handleAccessExpr(const Expr *E, } // ...and it isn't tainted, so the checker will (optimistically) assume // that the offset is in bounds and mention this in the note tag. - Res.recordMayOverflowExtent(*KnownSize); + Res.MayOverflowExtent = *KnownSize; } // Actually update the state. The "if" only fails in the extremely unlikely From c3188960930d90bc4128c545c4a0bf0714978d59 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Wed, 15 Jul 2026 16:30:45 +0200 Subject: [PATCH 11/22] Separate handleAccessExpr and performCheck This splits `handleAccessExpr` into a new function `performCheck` (which will return a `CheckResult`) and a smaller `handleAccessExpr` (which calls `performCheck` and uses the `CheckResult` to report errors). --- .../Checkers/ArrayBoundChecker.cpp | 235 ++++++++++-------- 1 file changed, 132 insertions(+), 103 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp index b5d365521bbf5..f642f762b707a 100644 --- a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp @@ -93,6 +93,20 @@ class SizeUnit { } }; +struct CheckFlags { + unsigned CheckUnderflow : 1; + unsigned OffsetObviouslyNonnegative : 1; + unsigned AcceptPastTheEnd : 1; +}; + +// FIXME: This will be removed in a follow-up change: +enum class BadOffsetKind { Negative, Overflowing, Indeterminate }; + +class CheckResult; + +CheckResult checkBounds(ProgramStateRef State, SValBuilder &SVB, NonLoc Offset, + std::optional<NonLoc> Extent, CheckFlags Flags); + class CheckResult { public: /// When true, the bounds check noticed that the value of an unsigned @@ -104,20 +118,39 @@ 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 mayBeValid() const { return static_cast<bool>(ValidState); } /// 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 MayOverflowExtent.has_value(); } /// When true, the checked offset may be out of bounds. bool mayBeInvalid() const { return MayUnderflow || MayOverflowExtent; } + /// Returns the extent of the accessed region if it is relevant (because the + /// offset may overflow it), otherwise returns std::nullopt. + std::optional<NonLoc> extentIfRelevant() const { return MayOverflowExtent; } + /// Returns the program state that should be used for continuing the analysis /// after this bounds check. This returns null if mayBeValid() is false, in - /// that case use the state before the check as the state of the error node. + /// 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 getValidState() const { return ValidState; } + /// FIXME: This is a temporary hack to postpone the removal of BadOffsetKind. + /// Will be removed in a follow-up change. + BadOffsetKind getBadOffsetKind() const { + assert(mayBeInvalid() && "this is only used when the access is invalid"); + return (MayUnderflow ? (MayOverflowExtent ? BadOffsetKind::Indeterminate + : BadOffsetKind::Negative) + : BadOffsetKind::Overflowing); + } + /// When the access was ambiguous (that is, mayBeValid() && mayBeInvalid()), /// returns the note "assuming in bounds" note that is relevant for the bug /// report \p BR. When the access wasn't ambiguous or the the assumption is @@ -126,15 +159,16 @@ class CheckResult { std::string getMessage(PathSensitiveBugReport &BR, StringRef RegName, SizeUnit SU) const; + friend CheckResult checkBounds(ProgramStateRef State, SValBuilder &SVB, + NonLoc Offset, std::optional<NonLoc> Extent, + CheckFlags Flags); + private: // Offset of the accessed location, measured from the start of the region. // As of now, the offset and the extent are always measured in bytes, but it // may be necessary to change this in the future. const NonLoc Offset; -public: - // FIXME: The following members should be private and they will become - // private soon in a follow-up commit. explicit CheckResult(NonLoc Offs) : Offset(Offs) {} bool IsCorruptedState = false; @@ -147,8 +181,6 @@ struct Messages { std::string Short, Full; }; -enum class BadOffsetKind { Negative, Overflowing, Indeterminate }; - constexpr llvm::StringLiteral Adjectives[] = {"a negative", "an overflowing", "a negative or overflowing"}; static StringRef asAdjective(BadOffsetKind Problem) { @@ -188,9 +220,6 @@ class ArrayBoundChecker : public Checker<check::PostStmt<ArraySubscriptExpr>, static bool isOffsetObviouslyNonnegative(const Expr *E, CheckerContext &C); - static bool isIdiomaticPastTheEndPtr(const Expr *E, ProgramStateRef State, - NonLoc Offset, NonLoc Limit, - CheckerContext &C); static bool isInAddressOf(const Stmt *S, ASTContext &AC); public: @@ -610,27 +639,83 @@ void ArrayBoundChecker::handleAccessExpr(const Expr *E, auto [Reg, ByteOffset] = *RawOffset; const MemSpaceRegion *Space = Reg->getMemorySpace(State); - DefinedOrUnknownSVal Extent = getDynamicExtent(State, Reg, SVB); - // The state updates will be reported as a single note tag, which will be - // composed by this helper class. - CheckResult Res(ByteOffset); + auto Extent = getDynamicExtent(State, Reg, SVB).getAs<NonLoc>(); + + // A symbolic region in unknown space represents an unknown pointer that + // may point into the middle of an array, so we don't look for underflows. + // Both conditions are significant because we want to check underflows in + // symbolic regions on the heap (which may be introduced by checkers like + // MallocChecker that call SValBuilder::getConjuredHeapSymbolVal()) and + // non-symbolic regions (e.g. a field subregion of a symbolic region) in + // unknown space. + + CheckFlags Flags = { + /*CheckUnderflow=*/!(isa<SymbolicRegion>(Reg) && + isa<UnknownSpaceRegion>(Space)), + /*OffsetObviouslyNonnegative=*/isOffsetObviouslyNonnegative(E, C), + /*AcceptPastTheEnd=*/isa<ArraySubscriptExpr>(E) && + isInAddressOf(E, C.getASTContext()), + }; + + CheckResult Res = checkBounds(State, SVB, ByteOffset, Extent, Flags); + + if (Res.isCorruptedState()) { + C.addSink(); + return; + } + + const NoteTag *T = nullptr; + + if (Res.mayBeInvalid()) { + if (!Res.mayBeValid()) { + std::string RegName = getRegionName(Space, Reg); + Messages Msgs = getNonTaintMsgs(C.getASTContext(), RegName, ByteOffset, + Res.extentIfRelevant(), Location, + Res.getBadOffsetKind()); + reportOOB(C, State, Msgs, ByteOffset, Res.extentIfRelevant()); + return; + } + + if (isTainted(State, ByteOffset)) { + // Diagnostic detail: saying "tainted offset" is always correct, but + // the common case is that 'idx' is tainted in 'arr[idx]' and then it's + // nicer to say "tainted index". + const char *OffsetName = "offset"; + if (const auto *ASE = dyn_cast<ArraySubscriptExpr>(E)) + if (isTainted(State, ASE->getIdx(), C.getStackFrame())) + OffsetName = "index"; + + std::string RegName = getRegionName(Space, Reg); + Messages Msgs = getTaintMsgs(RegName, OffsetName, Res.mayUnderflow()); + reportOOB(C, State, Msgs, ByteOffset, Extent, /*IsTaintBug=*/true); + return; + } + + std::string RN = getRegionName(Space, Reg); + SizeUnit SU = SizeUnit::forExpr(E, C); + T = C.getNoteTag([Res, RN, SU](PathSensitiveBugReport &BR) -> std::string { + return Res.getMessage(BR, RN, SU); + }); + } + + C.addTransition(Res.getValidState(), T); +} + +namespace { + +CheckResult checkBounds(ProgramStateRef State, SValBuilder &SVB, NonLoc Offset, + std::optional<NonLoc> Extent, CheckFlags Flags) { + + CheckResult Res(Offset); // CHECK LOWER BOUND - if (!(isa<SymbolicRegion>(Reg) && isa<UnknownSpaceRegion>(Space))) { - // A symbolic region in unknown space represents an unknown pointer that - // may point into the middle of an array, so we don't look for underflows. - // Both conditions are significant because we want to check underflows in - // symbolic regions on the heap (which may be introduced by checkers like - // MallocChecker that call SValBuilder::getConjuredHeapSymbolVal()) and - // non-symbolic regions (e.g. a field subregion of a symbolic region) in - // unknown space. - auto [PrecedesLowerBound, WithinLowerBound] = compareValueToThreshold( - State, ByteOffset, SVB.makeZeroArrayIndex(), SVB); + if (Flags.CheckUnderflow) { + auto [PrecedesLowerBound, WithinLowerBound] = + compareValueToThreshold(State, Offset, SVB.makeZeroArrayIndex(), SVB); if (PrecedesLowerBound) { // The analyzer thinks that the offset may be invalid (negative)... - - if (isOffsetObviouslyNonnegative(E, C)) { + if (Flags.OffsetObviouslyNonnegative) { // ...but the offset is obviously non-negative (clear array subscript // with an unsigned index), so we're in a buggy situation. @@ -649,25 +734,19 @@ void ArrayBoundChecker::handleAccessExpr(const Expr *E, if (!WithinLowerBound) { // The state is completely nonsense -- let's just sink it! - C.addSink(); - return; + Res.IsCorruptedState = true; + return Res; } // Otherwise continue on the 'WithinLowerBound' branch where the // unsigned index _is_ non-negative. Don't mention this assumption as a // note tag, because it would just confuse the users! } else { + Res.MayUnderflow = true; + if (!WithinLowerBound) { // ...and it cannot be valid (>= 0), so report an error. - std::string RegName = getRegionName(Space, Reg); - Messages Msgs = getNonTaintMsgs(C.getASTContext(), RegName, - ByteOffset, /*Extent=*/std::nullopt, - Location, BadOffsetKind::Negative); - reportOOB(C, PrecedesLowerBound, Msgs, ByteOffset, std::nullopt); - return; + return Res; } - // ...but it can be valid as well, so the checker will (optimistically) - // assume that it's valid and mention this in the note tag. - Res.MayUnderflow = true; } } @@ -679,84 +758,46 @@ void ArrayBoundChecker::handleAccessExpr(const Expr *E, } // CHECK UPPER BOUND - if (auto KnownSize = Extent.getAs<NonLoc>()) { + 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. - bool AlsoMentionUnderflow = Res.mayUnderflow(); auto [WithinUpperBound, ExceedsUpperBound] = - compareValueToThreshold(State, ByteOffset, *KnownSize, SVB); + compareValueToThreshold(State, Offset, *Extent, SVB); if (ExceedsUpperBound) { // The offset may be invalid (>= Size)... + Res.MayOverflowExtent = 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 (isIdiomaticPastTheEndPtr(E, ExceedsUpperBound, ByteOffset, - *KnownSize, C)) { - // The use of 'goto' is a temporary solution, will be eliminated in - // the next steps of the refactoring. - goto NormalTransition; + if (Flags.AcceptPastTheEnd) { + auto [EqualsToThreshold, NotEqualToThreshold] = + compareValueToThreshold(State, Offset, *Extent, SVB, + /*CheckEquality=*/true); + if (EqualsToThreshold && !NotEqualToThreshold) { + Res.MayOverflowExtent = std::nullopt; + Res.ValidState = EqualsToThreshold; + } } - - BadOffsetKind Problem = AlsoMentionUnderflow - ? BadOffsetKind::Indeterminate - : BadOffsetKind::Overflowing; - std::string RegName = getRegionName(Space, Reg); - Messages Msgs = getNonTaintMsgs(C.getASTContext(), RegName, ByteOffset, - *KnownSize, Location, Problem); - reportOOB(C, ExceedsUpperBound, Msgs, ByteOffset, KnownSize); - return; - } - // ...and it can be valid as well... - if (isTainted(State, ByteOffset)) { - // ...but it's tainted, so report an error. - - // Diagnostic detail: saying "tainted offset" is always correct, but - // the common case is that 'idx' is tainted in 'arr[idx]' and then it's - // nicer to say "tainted index". - const char *OffsetName = "offset"; - if (const auto *ASE = dyn_cast<ArraySubscriptExpr>(E)) - if (isTainted(State, ASE->getIdx(), C.getStackFrame())) - OffsetName = "index"; - - std::string RegName = getRegionName(Space, Reg); - Messages Msgs = getTaintMsgs(RegName, OffsetName, AlsoMentionUnderflow); - reportOOB(C, ExceedsUpperBound, Msgs, ByteOffset, KnownSize, - /*IsTaintBug=*/true); - return; + return Res; } - // ...and it isn't tainted, so the checker will (optimistically) assume - // that the offset is in bounds and mention this in the note tag. - Res.MayOverflowExtent = *KnownSize; } - - // Actually update the state. The "if" only fails in the extremely unlikely - // case when compareValueToThreshold returns {nullptr, nullptr} because - // evalBinOpNN fails to evaluate the less-than operator. if (WithinUpperBound) State = WithinUpperBound; } -NormalTransition: - // Add a transition, reporting the state updates that we accumulated. - const NoteTag *T = nullptr; - - if (Res.mayBeInvalid()) { - std::string RN = getRegionName(Reg->getMemorySpace(C.getState()), Reg); - SizeUnit SU = SizeUnit::forExpr(E, C); - T = C.getNoteTag([Res, RN, SU](PathSensitiveBugReport &BR) -> std::string { - return Res.getMessage(BR, RN, SU); - }); - } - - C.addTransition(State, T); + Res.ValidState = State; + return Res; } +} // anonymous namespace + void ArrayBoundChecker::markPartsInteresting(PathSensitiveBugReport &BR, ProgramStateRef ErrorState, NonLoc Val, bool MarkTaint) { @@ -851,18 +892,6 @@ bool ArrayBoundChecker::isInAddressOf(const Stmt *S, ASTContext &ACtx) { return UnaryOp && UnaryOp->getOpcode() == UO_AddrOf; } -bool ArrayBoundChecker::isIdiomaticPastTheEndPtr(const Expr *E, - ProgramStateRef State, - NonLoc Offset, NonLoc Limit, - CheckerContext &C) { - if (isa<ArraySubscriptExpr>(E) && isInAddressOf(E, C.getASTContext())) { - auto [EqualsToThreshold, NotEqualToThreshold] = compareValueToThreshold( - State, Offset, Limit, C.getSValBuilder(), /*CheckEquality=*/true); - return EqualsToThreshold && !NotEqualToThreshold; - } - return false; -} - void ento::registerArrayBoundChecker(CheckerManager &mgr) { mgr.registerChecker<ArrayBoundChecker>(); } From bc0db58a7e1492751531b2f761a7b99e1854f2b8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Wed, 15 Jul 2026 17:11:24 +0200 Subject: [PATCH 12/22] Unify calls of getRegionName --- .../StaticAnalyzer/Checkers/ArrayBoundChecker.cpp | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp index f642f762b707a..158a98e3e645e 100644 --- a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp @@ -664,11 +664,11 @@ void ArrayBoundChecker::handleAccessExpr(const Expr *E, return; } - const NoteTag *T = nullptr; + std::string RegName = getRegionName(Space, Reg); + const NoteTag *T = nullptr; if (Res.mayBeInvalid()) { if (!Res.mayBeValid()) { - std::string RegName = getRegionName(Space, Reg); Messages Msgs = getNonTaintMsgs(C.getASTContext(), RegName, ByteOffset, Res.extentIfRelevant(), Location, Res.getBadOffsetKind()); @@ -685,17 +685,16 @@ void ArrayBoundChecker::handleAccessExpr(const Expr *E, if (isTainted(State, ASE->getIdx(), C.getStackFrame())) OffsetName = "index"; - std::string RegName = getRegionName(Space, Reg); Messages Msgs = getTaintMsgs(RegName, OffsetName, Res.mayUnderflow()); reportOOB(C, State, Msgs, ByteOffset, Extent, /*IsTaintBug=*/true); return; } - std::string RN = getRegionName(Space, Reg); SizeUnit SU = SizeUnit::forExpr(E, C); - T = C.getNoteTag([Res, RN, SU](PathSensitiveBugReport &BR) -> std::string { - return Res.getMessage(BR, RN, SU); - }); + T = C.getNoteTag( + [Res, RegName, SU](PathSensitiveBugReport &BR) -> std::string { + return Res.getMessage(BR, RegName, SU); + }); } C.addTransition(Res.getValidState(), T); From b2e3397bd628c907e4cb6693b3f6486824d1bc23 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Mon, 20 Jul 2026 14:34:36 +0200 Subject: [PATCH 13/22] Eliminate enum BadOffsetKind --- .../Checkers/ArrayBoundChecker.cpp | 64 ++++++++----------- 1 file changed, 27 insertions(+), 37 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp index 158a98e3e645e..6482c1f66b95d 100644 --- a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp @@ -99,9 +99,6 @@ struct CheckFlags { unsigned AcceptPastTheEnd : 1; }; -// FIXME: This will be removed in a follow-up change: -enum class BadOffsetKind { Negative, Overflowing, Indeterminate }; - class CheckResult; CheckResult checkBounds(ProgramStateRef State, SValBuilder &SVB, NonLoc Offset, @@ -131,9 +128,15 @@ class CheckResult { /// When true, the checked offset may be out of bounds. bool mayBeInvalid() const { return MayUnderflow || MayOverflowExtent; } + /// Returns the offset of the accessed location from the beginning of the + /// accessd region. + NonLoc getOffset() const { return Offset; } + /// Returns the extent of the accessed region if it is relevant (because the /// offset may overflow it), otherwise returns std::nullopt. - std::optional<NonLoc> extentIfRelevant() const { return MayOverflowExtent; } + std::optional<NonLoc> getExtentIfRelevant() const { + return MayOverflowExtent; + } /// Returns the program state that should be used for continuing the analysis /// after this bounds check. This returns null if mayBeValid() is false, in @@ -142,15 +145,6 @@ class CheckResult { /// 'access' calculates the past-the-end pointer without dereferencing it. ProgramStateRef getValidState() const { return ValidState; } - /// FIXME: This is a temporary hack to postpone the removal of BadOffsetKind. - /// Will be removed in a follow-up change. - BadOffsetKind getBadOffsetKind() const { - assert(mayBeInvalid() && "this is only used when the access is invalid"); - return (MayUnderflow ? (MayOverflowExtent ? BadOffsetKind::Indeterminate - : BadOffsetKind::Negative) - : BadOffsetKind::Overflowing); - } - /// When the access was ambiguous (that is, mayBeValid() && mayBeInvalid()), /// returns the note "assuming in bounds" note that is relevant for the bug /// report \p BR. When the access wasn't ambiguous or the the assumption is @@ -181,18 +175,6 @@ struct Messages { std::string Short, Full; }; -constexpr llvm::StringLiteral Adjectives[] = {"a negative", "an overflowing", - "a negative or overflowing"}; -static StringRef asAdjective(BadOffsetKind Problem) { - return Adjectives[static_cast<int>(Problem)]; -} - -constexpr llvm::StringLiteral Prepositions[] = {"preceding", "after the end of", - "around"}; -static StringRef asPreposition(BadOffsetKind Problem) { - return Prepositions[static_cast<int>(Problem)]; -} - // NOTE: The `ArraySubscriptExpr` and `UnaryOperator` callbacks are `PostStmt` // instead of `PreStmt` because the current implementation passes the whole // expression to `CheckerContext::getSVal()` which only works after the @@ -500,15 +482,24 @@ static bool tryDividePair(std::optional<int64_t> &Val1, return true; } +static const char *getAdjective(const CheckResult &R) { + return (R.mayUnderflow() + ? (R.mayOverflow() ? "a negative or overflowing" : "a negative") + : (R.mayOverflow() ? "an overflowing" : "a valid")); +} + +static const char *getPreposition(const CheckResult &R) { + return (R.mayUnderflow() ? (R.mayOverflow() ? "around" : "preceding") + : (R.mayOverflow() ? "after the end of" : "within")); +} static Messages getNonTaintMsgs(const ASTContext &ACtx, StringRef RegName, - NonLoc Offset, std::optional<NonLoc> Extent, - SVal Location, BadOffsetKind Problem) { + CheckResult Res, SVal Location) { const auto *EReg = Location.getAsRegion()->getAs<ElementRegion>(); assert(EReg && "this checker only handles element access"); QualType ElemType = EReg->getElementType(); - std::optional<int64_t> OffsetN = getConcreteValue(Offset); - std::optional<int64_t> ExtentN = getConcreteValue(Extent); + std::optional<int64_t> OffsetN = getConcreteValue(Res.getOffset()); + std::optional<int64_t> ExtentN = getConcreteValue(Res.getExtentIfRelevant()); int64_t ElemSize = ACtx.getTypeSizeInChars(ElemType).getQuantity(); @@ -528,11 +519,11 @@ static Messages getNonTaintMsgs(const ASTContext &ACtx, StringRef RegName, } Out << RegName << " at "; if (OffsetN) { - if (Problem == BadOffsetKind::Negative) + if (Res.mayUnderflow() && !Res.mayOverflow()) Out << "negative "; Out << OffsetOrIndex << " " << *OffsetN; } else { - Out << asAdjective(Problem) << " " << OffsetOrIndex; + Out << getAdjective(Res) << " " << OffsetOrIndex; } if (ExtentN) { Out << ", while it holds only "; @@ -549,8 +540,8 @@ static Messages getNonTaintMsgs(const ASTContext &ACtx, StringRef RegName, Out << "s"; } - return {formatv("Out of bound access to memory {0} {1}", - asPreposition(Problem), RegName), + return {formatv("Out of bound access to memory {0} {1}", getPreposition(Res), + RegName), std::string(Buf)}; } @@ -669,10 +660,9 @@ void ArrayBoundChecker::handleAccessExpr(const Expr *E, const NoteTag *T = nullptr; if (Res.mayBeInvalid()) { if (!Res.mayBeValid()) { - Messages Msgs = getNonTaintMsgs(C.getASTContext(), RegName, ByteOffset, - Res.extentIfRelevant(), Location, - Res.getBadOffsetKind()); - reportOOB(C, State, Msgs, ByteOffset, Res.extentIfRelevant()); + Messages Msgs = + getNonTaintMsgs(C.getASTContext(), RegName, Res, Location); + reportOOB(C, State, Msgs, ByteOffset, Res.getExtentIfRelevant()); return; } From dd1073b852a5f32f09258e37240a1f11c86414e3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Mon, 20 Jul 2026 14:40:45 +0200 Subject: [PATCH 14/22] Rename Messages to BugDescription; rename functions using it --- .../Checkers/ArrayBoundChecker.cpp | 39 +++++++++++-------- 1 file changed, 23 insertions(+), 16 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp index 6482c1f66b95d..79317c462b1c3 100644 --- a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp @@ -171,8 +171,11 @@ class CheckResult { ProgramStateRef ValidState = nullptr; }; -struct Messages { - std::string Short, Full; +/// Strings that will be passed to the parameters 'desc' and 'fullDesc' of the +/// constructor of 'PathSensitiveBugReport'. +struct BugDescription { + std::string Short; + std::string Full; }; // NOTE: The `ArraySubscriptExpr` and `UnaryOperator` callbacks are `PostStmt` @@ -190,9 +193,9 @@ class ArrayBoundChecker : public Checker<check::PostStmt<ArraySubscriptExpr>, void handleAccessExpr(const Expr *E, CheckerContext &C) const; - void reportOOB(CheckerContext &C, ProgramStateRef ErrorState, Messages Msgs, - NonLoc Offset, std::optional<NonLoc> Extent, - bool IsTaintBug = false) const; + void reportOOB(CheckerContext &C, ProgramStateRef ErrorState, + BugDescription Desc, NonLoc Offset, + std::optional<NonLoc> Extent, bool IsTaintBug = false) const; static void markPartsInteresting(PathSensitiveBugReport &BR, ProgramStateRef ErrorState, NonLoc Val, @@ -492,8 +495,10 @@ static const char *getPreposition(const CheckResult &R) { return (R.mayUnderflow() ? (R.mayOverflow() ? "around" : "preceding") : (R.mayOverflow() ? "after the end of" : "within")); } -static Messages getNonTaintMsgs(const ASTContext &ACtx, StringRef RegName, - CheckResult Res, SVal Location) { + +static BugDescription describeInvalidAccess(const ASTContext &ACtx, + StringRef RegName, CheckResult Res, + SVal Location) { const auto *EReg = Location.getAsRegion()->getAs<ElementRegion>(); assert(EReg && "this checker only handles element access"); QualType ElemType = EReg->getElementType(); @@ -545,8 +550,9 @@ static Messages getNonTaintMsgs(const ASTContext &ACtx, StringRef RegName, std::string(Buf)}; } -static Messages getTaintMsgs(StringRef RegName, const char *OffsetName, - bool AlsoMentionUnderflow) { +static BugDescription describeTaintBug(StringRef RegName, + const char *OffsetName, + bool AlsoMentionUnderflow) { return {formatv("Potential out of bound access to {0} with tainted {1}", RegName, OffsetName), formatv("Access of {0} with a tainted {1} that may be {2}too large", @@ -660,9 +666,9 @@ void ArrayBoundChecker::handleAccessExpr(const Expr *E, const NoteTag *T = nullptr; if (Res.mayBeInvalid()) { if (!Res.mayBeValid()) { - Messages Msgs = - getNonTaintMsgs(C.getASTContext(), RegName, Res, Location); - reportOOB(C, State, Msgs, ByteOffset, Res.getExtentIfRelevant()); + BugDescription Desc = + describeInvalidAccess(C.getASTContext(), RegName, Res, Location); + reportOOB(C, State, Desc, ByteOffset, Res.getExtentIfRelevant()); return; } @@ -675,8 +681,9 @@ void ArrayBoundChecker::handleAccessExpr(const Expr *E, if (isTainted(State, ASE->getIdx(), C.getStackFrame())) OffsetName = "index"; - Messages Msgs = getTaintMsgs(RegName, OffsetName, Res.mayUnderflow()); - reportOOB(C, State, Msgs, ByteOffset, Extent, /*IsTaintBug=*/true); + BugDescription Desc = + describeTaintBug(RegName, OffsetName, Res.mayUnderflow()); + reportOOB(C, State, Desc, ByteOffset, Extent, /*IsTaintBug=*/true); return; } @@ -811,7 +818,7 @@ void ArrayBoundChecker::markPartsInteresting(PathSensitiveBugReport &BR, } void ArrayBoundChecker::reportOOB(CheckerContext &C, ProgramStateRef ErrorState, - Messages Msgs, NonLoc Offset, + BugDescription Desc, NonLoc Offset, std::optional<NonLoc> Extent, bool IsTaintBug /*=false*/) const { @@ -820,7 +827,7 @@ void ArrayBoundChecker::reportOOB(CheckerContext &C, ProgramStateRef ErrorState, return; auto BR = std::make_unique<PathSensitiveBugReport>( - IsTaintBug ? TaintBT : BT, Msgs.Short, Msgs.Full, ErrorNode); + IsTaintBug ? TaintBT : BT, Desc.Short, Desc.Full, ErrorNode); // FIXME: ideally we would just call trackExpressionValue() and that would // "do the right thing": mark the relevant symbols as interesting, track the From 79a02f19506f2398d939eb425bacd9298936c34e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Mon, 20 Jul 2026 16:07:40 +0200 Subject: [PATCH 15/22] Rename CheckResult::getMessage to static getAssumptionNote This function only uses the public interface of CheckResult, so it doesn't need to be a member function. --- .../Checkers/ArrayBoundChecker.cpp | 35 +++++++++---------- 1 file changed, 16 insertions(+), 19 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp index 79317c462b1c3..e12986e4863c5 100644 --- a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp @@ -145,14 +145,6 @@ class CheckResult { /// 'access' calculates the past-the-end pointer without dereferencing it. ProgramStateRef getValidState() const { return ValidState; } - /// When the access was ambiguous (that is, mayBeValid() && mayBeInvalid()), - /// returns the note "assuming in bounds" note that is relevant for the bug - /// report \p BR. When the access wasn't ambiguous or the the assumption is - /// irrelevant for \p BR, this returns the empty string (which signifies "do - /// not emit a note tag" when returned by a note tag callback). - std::string getMessage(PathSensitiveBugReport &BR, StringRef RegName, - SizeUnit SU) const; - friend CheckResult checkBounds(ProgramStateRef State, SValBuilder &SVB, NonLoc Offset, std::optional<NonLoc> Extent, CheckFlags Flags); @@ -560,12 +552,17 @@ static BugDescription describeTaintBug(StringRef RegName, AlsoMentionUnderflow ? "negative or " : "")}; } -std::string CheckResult::getMessage(PathSensitiveBugReport &BR, - StringRef RegName, SizeUnit SU) const { - bool ShouldReportNonNegative = MayUnderflow; - if (!providesInformationAboutInteresting(Offset, BR)) { - if (MayOverflowExtent && - providesInformationAboutInteresting(*MayOverflowExtent, BR)) { +/// When the access was ambiguous (that is, mayBeValid() && mayBeInvalid()), +/// returns the note "assuming in bounds" note that is relevant for the bug +/// report \p BR. When the access wasn't ambiguous or the the assumption is +/// irrelevant for \p BR, this returns the empty string (which signifies "do +/// not emit a note tag" when returned by a note tag callback). +static std::string getAssumptionNote(CheckResult Res, PathSensitiveBugReport &BR, + StringRef RegName, SizeUnit SU) { + bool ShouldReportNonNegative = Res.mayUnderflow(); + if (!providesInformationAboutInteresting(Res.getOffset(), BR)) { + std::optional<NonLoc> E = Res.getExtentIfRelevant(); + if (E && providesInformationAboutInteresting(*E, BR)) { // Even if the byte offset isn't interesting (e.g. it's a constant value), // the assumption can still be interesting if it provides information // about an interesting symbolic upper bound. @@ -576,8 +573,8 @@ std::string CheckResult::getMessage(PathSensitiveBugReport &BR, } } - std::optional<int64_t> OffsetN = getConcreteValue(Offset); - std::optional<int64_t> ExtentN = getConcreteValue(MayOverflowExtent); + std::optional<int64_t> OffsetN = getConcreteValue(Res.getOffset()); + std::optional<int64_t> ExtentN = getConcreteValue(Res.getExtentIfRelevant()); const bool UseIndex = !SU.isBytes() && tryDividePair(OffsetN, ExtentN, SU.asCharUnits()); @@ -589,7 +586,7 @@ std::string CheckResult::getMessage(PathSensitiveBugReport &BR, Out << "index "; if (OffsetN) Out << "'" << OffsetN << "' "; - } else if (MayOverflowExtent) { + } else if (Res.mayOverflow()) { Out << "byte offset "; if (OffsetN) Out << "'" << OffsetN << "' "; @@ -601,7 +598,7 @@ std::string CheckResult::getMessage(PathSensitiveBugReport &BR, if (ShouldReportNonNegative) { Out << " non-negative"; } - if (MayOverflowExtent) { + if (Res.mayOverflow()) { if (ShouldReportNonNegative) Out << " and"; Out << " less than "; @@ -690,7 +687,7 @@ void ArrayBoundChecker::handleAccessExpr(const Expr *E, SizeUnit SU = SizeUnit::forExpr(E, C); T = C.getNoteTag( [Res, RegName, SU](PathSensitiveBugReport &BR) -> std::string { - return Res.getMessage(BR, RegName, SU); + return getAssumptionNote(Res, BR, RegName, SU); }); } From 17fd4ea92a396dbd744d641b452468cacfb57639 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Mon, 20 Jul 2026 18:23:20 +0200 Subject: [PATCH 16/22] Use SizeUnit in describeInvalidAccess --- .../Checkers/ArrayBoundChecker.cpp | 44 ++++++++++++------- 1 file changed, 27 insertions(+), 17 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp index e12986e4863c5..ab91cdd5837e7 100644 --- a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp @@ -73,9 +73,21 @@ class SizeUnit { bool isBytes() const { return AsType.isNull(); } + /// Return the element type that is "natural" for reporting out-of-bounds + /// memory access to 'Location'. + static SizeUnit forSVal(SVal Location, const ASTContext &ACtx) { + if (const auto *R = Location.getAsRegion()->getAs<TypedValueRegion>()) + return SizeUnit(R->getValueType(), ACtx); + return bytes(); + } + /// If `E` is a "clean" array subscript expression, return the type of the /// accessed element; otherwise return 'Bytes' because that's the best (or - /// least bad) option for the diagnostic generation that relies on this. + /// least bad) option for the assumption messages that use this. + /// FIXME: It is unfortunate that this heuristic differs from the heuristic + /// used for reporting assumption; but this difference is currently needed + /// due to the unfortunate phrasing of the assumption messages. + /// Get rid of this when the assumption note is rephrased and improved. static SizeUnit forExpr(const Expr *E, const CheckerContext &C) { const auto *ASE = getAsCleanArraySubscriptExpr(E, C); if (!ASE) @@ -91,6 +103,12 @@ class SizeUnit { return "the extent of"; return formatv("the number of '{0}' elements in", AsType.getAsString()); } + + std::string asElementName(bool ForceBytes) const { + if (ForceBytes || isBytes()) + return "byte"; + return formatv("'{0}' element", AsType.getAsString()); + } }; struct CheckFlags { @@ -488,19 +506,13 @@ static const char *getPreposition(const CheckResult &R) { : (R.mayOverflow() ? "after the end of" : "within")); } -static BugDescription describeInvalidAccess(const ASTContext &ACtx, - StringRef RegName, CheckResult Res, - SVal Location) { - const auto *EReg = Location.getAsRegion()->getAs<ElementRegion>(); - assert(EReg && "this checker only handles element access"); - QualType ElemType = EReg->getElementType(); +static BugDescription describeInvalidAccess(CheckResult Res, StringRef RegName, + SizeUnit SU) { std::optional<int64_t> OffsetN = getConcreteValue(Res.getOffset()); std::optional<int64_t> ExtentN = getConcreteValue(Res.getExtentIfRelevant()); - int64_t ElemSize = ACtx.getTypeSizeInChars(ElemType).getQuantity(); - - bool UseByteOffsets = !tryDividePair(OffsetN, ExtentN, ElemSize); + bool UseByteOffsets = !tryDividePair(OffsetN, ExtentN, SU.asCharUnits()); const char *OffsetOrIndex = UseByteOffsets ? "byte offset" : "index"; SmallString<256> Buf; @@ -512,7 +524,7 @@ static BugDescription describeInvalidAccess(const ASTContext &ACtx, // natural to mention the element type later where the extent is described, // but if the extent is unknown/irrelevant, then the element type can be // inserted into the message at this point. - Out << "'" << ElemType.getAsString() << "' element in "; + Out << SU.asElementName(/*ForceBytes=*/false) << " in "; } Out << RegName << " at "; if (OffsetN) { @@ -528,10 +540,8 @@ static BugDescription describeInvalidAccess(const ASTContext &ACtx, Out << *ExtentN; else Out << "a single"; - if (UseByteOffsets) - Out << " byte"; - else - Out << " '" << ElemType.getAsString() << "' element"; + + Out << ' ' << SU.asElementName(/*ForceBytes=*/UseByteOffsets); if (*ExtentN > 1) Out << "s"; @@ -663,8 +673,8 @@ void ArrayBoundChecker::handleAccessExpr(const Expr *E, const NoteTag *T = nullptr; if (Res.mayBeInvalid()) { if (!Res.mayBeValid()) { - BugDescription Desc = - describeInvalidAccess(C.getASTContext(), RegName, Res, Location); + SizeUnit SU = SizeUnit::forSVal(Location, C.getASTContext()); + BugDescription Desc = describeInvalidAccess(Res, RegName, SU); reportOOB(C, State, Desc, ByteOffset, Res.getExtentIfRelevant()); return; } From e9ea7a29c108870d6893299768664781e5a585a1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Mon, 20 Jul 2026 19:03:32 +0200 Subject: [PATCH 17/22] Eliminate the 'ForceBytes' hack and tryDividePair The helper function 'tryDividePair' captured logic that was appearing twice in the code, but it was awkward and counter-intuitive, so I decided to spell it out in both locations. --- .../Checkers/ArrayBoundChecker.cpp | 66 +++++++++---------- 1 file changed, 33 insertions(+), 33 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp index ab91cdd5837e7..eb84ca88061b0 100644 --- a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp @@ -98,14 +98,18 @@ class SizeUnit { int64_t asCharUnits() const { return AsCharUnits; } - std::string asExtentDesc(bool ForceBytes) const { - if (ForceBytes || isBytes()) + bool canExpress(std::optional<int64_t> Val) { + return !Val || !(*Val % asCharUnits()); + } + + std::string asExtentDesc() const { + if (isBytes()) return "the extent of"; return formatv("the number of '{0}' elements in", AsType.getAsString()); } - std::string asElementName(bool ForceBytes) const { - if (ForceBytes || isBytes()) + std::string asElementName() const { + if (isBytes()) return "byte"; return formatv("'{0}' element", AsType.getAsString()); } @@ -476,25 +480,6 @@ static std::optional<int64_t> getConcreteValue(std::optional<NonLoc> SV) { return SV ? getConcreteValue(*SV) : std::nullopt; } -/// Try to divide `Val1` and `Val2` (in place) by `Divisor` and return true if -/// it can be performed (`Divisor` is nonzero and there is no remainder). The -/// values `Val1` and `Val2` may be nullopt and in that case the corresponding -/// division is considered to be successful. -static bool tryDividePair(std::optional<int64_t> &Val1, - std::optional<int64_t> &Val2, int64_t Divisor) { - if (!Divisor) - return false; - const bool Val1HasRemainder = Val1 && *Val1 % Divisor; - const bool Val2HasRemainder = Val2 && *Val2 % Divisor; - if (Val1HasRemainder || Val2HasRemainder) - return false; - if (Val1) - *Val1 /= Divisor; - if (Val2) - *Val2 /= Divisor; - return true; -} - static const char *getAdjective(const CheckResult &R) { return (R.mayUnderflow() ? (R.mayOverflow() ? "a negative or overflowing" : "a negative") @@ -508,23 +493,31 @@ static const char *getPreposition(const CheckResult &R) { static BugDescription describeInvalidAccess(CheckResult Res, StringRef RegName, SizeUnit SU) { - std::optional<int64_t> OffsetN = getConcreteValue(Res.getOffset()); std::optional<int64_t> ExtentN = getConcreteValue(Res.getExtentIfRelevant()); - bool UseByteOffsets = !tryDividePair(OffsetN, ExtentN, SU.asCharUnits()); - const char *OffsetOrIndex = UseByteOffsets ? "byte offset" : "index"; + if (SU.canExpress(OffsetN) && SU.canExpress(ExtentN)) { + if (OffsetN) + *OffsetN /= SU.asCharUnits(); + if (ExtentN) + *ExtentN /= SU.asCharUnits(); + } else { + // Fall back to reporting the offsets in bytes. + SU = SizeUnit::bytes(); + } + + const char *OffsetOrIndex = SU.isBytes() ? "byte offset" : "index"; SmallString<256> Buf; llvm::raw_svector_ostream Out(Buf); Out << "Access of "; - if (OffsetN && !ExtentN && !UseByteOffsets) { + if (OffsetN && !ExtentN && !SU.isBytes()) { // If the offset is reported as an index, then the report must mention the // element type (because it is not always clear from the code). It's more // natural to mention the element type later where the extent is described, // but if the extent is unknown/irrelevant, then the element type can be // inserted into the message at this point. - Out << SU.asElementName(/*ForceBytes=*/false) << " in "; + Out << SU.asElementName() << " in "; } Out << RegName << " at "; if (OffsetN) { @@ -541,7 +534,7 @@ static BugDescription describeInvalidAccess(CheckResult Res, StringRef RegName, else Out << "a single"; - Out << ' ' << SU.asElementName(/*ForceBytes=*/UseByteOffsets); + Out << ' ' << SU.asElementName(); if (*ExtentN > 1) Out << "s"; @@ -586,13 +579,20 @@ static std::string getAssumptionNote(CheckResult Res, PathSensitiveBugReport &BR std::optional<int64_t> OffsetN = getConcreteValue(Res.getOffset()); std::optional<int64_t> ExtentN = getConcreteValue(Res.getExtentIfRelevant()); - const bool UseIndex = - !SU.isBytes() && tryDividePair(OffsetN, ExtentN, SU.asCharUnits()); + if (SU.canExpress(OffsetN) && SU.canExpress(ExtentN)) { + if (OffsetN) + *OffsetN /= SU.asCharUnits(); + if (ExtentN) + *ExtentN /= SU.asCharUnits(); + } else { + // Fall back to reporting the offsets in bytes. + SU = SizeUnit::bytes(); + } SmallString<256> Buf; llvm::raw_svector_ostream Out(Buf); Out << "Assuming "; - if (UseIndex) { + if (!SU.isBytes()) { Out << "index "; if (OffsetN) Out << "'" << OffsetN << "' "; @@ -614,7 +614,7 @@ static std::string getAssumptionNote(CheckResult Res, PathSensitiveBugReport &BR Out << " less than "; if (ExtentN) Out << *ExtentN << ", "; - Out << SU.asExtentDesc(/*ForceBytes=*/!UseIndex) << ' ' << RegName; + Out << SU.asExtentDesc() << ' ' << RegName; } return std::string(Out.str()); } From 247526de944c8413bb94f09a0f89760a952ec038 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Mon, 20 Jul 2026 19:26:02 +0200 Subject: [PATCH 18/22] Introduce clang::ento::bounds --- .../Checkers/ArrayBoundChecker.cpp | 33 ++++++++++--------- 1 file changed, 18 insertions(+), 15 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp index eb84ca88061b0..308b2c4742d29 100644 --- a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp @@ -115,6 +115,10 @@ class SizeUnit { } }; +} // anonymous namespace + +namespace clang::ento::bounds { + struct CheckFlags { unsigned CheckUnderflow : 1; unsigned OffsetObviouslyNonnegative : 1; @@ -173,8 +177,8 @@ class CheckResult { private: // Offset of the accessed location, measured from the start of the region. - // As of now, the offset and the extent are always measured in bytes, but it - // may be necessary to change this in the future. + // TODO: As of now, the offset and the extent are always measured in bytes, + // but we will probably need to allow other size units in the future. const NonLoc Offset; explicit CheckResult(NonLoc Offs) : Offset(Offs) {} @@ -185,6 +189,9 @@ class CheckResult { ProgramStateRef ValidState = nullptr; }; +} // namespace clang::ento::bounds + +namespace { /// Strings that will be passed to the parameters 'desc' and 'fullDesc' of the /// constructor of 'PathSensitiveBugReport'. struct BugDescription { @@ -480,18 +487,18 @@ static std::optional<int64_t> getConcreteValue(std::optional<NonLoc> SV) { return SV ? getConcreteValue(*SV) : std::nullopt; } -static const char *getAdjective(const CheckResult &R) { +static const char *getAdjective(const bounds::CheckResult &R) { return (R.mayUnderflow() ? (R.mayOverflow() ? "a negative or overflowing" : "a negative") : (R.mayOverflow() ? "an overflowing" : "a valid")); } -static const char *getPreposition(const CheckResult &R) { +static const char *getPreposition(const bounds::CheckResult &R) { return (R.mayUnderflow() ? (R.mayOverflow() ? "around" : "preceding") : (R.mayOverflow() ? "after the end of" : "within")); } -static BugDescription describeInvalidAccess(CheckResult Res, StringRef RegName, +static BugDescription describeInvalidAccess(bounds::CheckResult Res, StringRef RegName, SizeUnit SU) { std::optional<int64_t> OffsetN = getConcreteValue(Res.getOffset()); std::optional<int64_t> ExtentN = getConcreteValue(Res.getExtentIfRelevant()); @@ -560,7 +567,7 @@ static BugDescription describeTaintBug(StringRef RegName, /// report \p BR. When the access wasn't ambiguous or the the assumption is /// irrelevant for \p BR, this returns the empty string (which signifies "do /// not emit a note tag" when returned by a note tag callback). -static std::string getAssumptionNote(CheckResult Res, PathSensitiveBugReport &BR, +static std::string getAssumptionNote(bounds::CheckResult Res, PathSensitiveBugReport &BR, StringRef RegName, SizeUnit SU) { bool ShouldReportNonNegative = Res.mayUnderflow(); if (!providesInformationAboutInteresting(Res.getOffset(), BR)) { @@ -653,7 +660,7 @@ void ArrayBoundChecker::handleAccessExpr(const Expr *E, // non-symbolic regions (e.g. a field subregion of a symbolic region) in // unknown space. - CheckFlags Flags = { + bounds::CheckFlags Flags = { /*CheckUnderflow=*/!(isa<SymbolicRegion>(Reg) && isa<UnknownSpaceRegion>(Space)), /*OffsetObviouslyNonnegative=*/isOffsetObviouslyNonnegative(E, C), @@ -661,7 +668,7 @@ void ArrayBoundChecker::handleAccessExpr(const Expr *E, isInAddressOf(E, C.getASTContext()), }; - CheckResult Res = checkBounds(State, SVB, ByteOffset, Extent, Flags); + bounds::CheckResult Res = checkBounds(State, SVB, ByteOffset, Extent, Flags); if (Res.isCorruptedState()) { C.addSink(); @@ -704,12 +711,10 @@ void ArrayBoundChecker::handleAccessExpr(const Expr *E, C.addTransition(Res.getValidState(), T); } -namespace { - -CheckResult checkBounds(ProgramStateRef State, SValBuilder &SVB, NonLoc Offset, - std::optional<NonLoc> Extent, CheckFlags Flags) { +bounds::CheckResult bounds::checkBounds(ProgramStateRef State, SValBuilder &SVB, NonLoc Offset, + std::optional<NonLoc> Extent, bounds::CheckFlags Flags) { - CheckResult Res(Offset); + bounds::CheckResult Res(Offset); // CHECK LOWER BOUND if (Flags.CheckUnderflow) { @@ -799,8 +804,6 @@ CheckResult checkBounds(ProgramStateRef State, SValBuilder &SVB, NonLoc Offset, return Res; } -} // anonymous namespace - void ArrayBoundChecker::markPartsInteresting(PathSensitiveBugReport &BR, ProgramStateRef ErrorState, NonLoc Val, bool MarkTaint) { From 495d431cf8327568dc5dedd3bbc239ebda95f8b3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Mon, 29 Jun 2026 14:34:20 +0200 Subject: [PATCH 19/22] [side] Fix an obsolete comment in a test --- clang/test/Analysis/ArrayBound/verbose-tests.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/clang/test/Analysis/ArrayBound/verbose-tests.c b/clang/test/Analysis/ArrayBound/verbose-tests.c index c0da93ea48591..772ef3d8868a5 100644 --- a/clang/test/Analysis/ArrayBound/verbose-tests.c +++ b/clang/test/Analysis/ArrayBound/verbose-tests.c @@ -44,8 +44,7 @@ struct TwoInts underflowReportedAsStruct(void) { struct TwoInts underflowOnlyByteOffset(void) { // In this case the negative byte offset is not a multiple of the size of the - // accessed element, so the part "= -... * sizeof(type)" is omitted at the - // end of the message. + // accessed element, so we use a byte offset instead of an index. return *(struct TwoInts*)(TenElements - 3); // expected-warning@-1 {{Out of bound access to memory preceding 'TenElements'}} // expected-note@-2 {{Access of 'TenElements' at negative byte offset -12}} From 5163e02e4f2593f8be4e5aa2ec53f17370b922fb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Mon, 29 Jun 2026 15:22:01 +0200 Subject: [PATCH 20/22] Improve test 'assumingLower', add other tests Ensure that the test 'assumingLower' would not accept a note that looks like "Assuming index is non-negative and less than..." (By default the expected-... markers look for _substring_ matches; which is a footgun. During development this test failed to catch a regression due to this.) Also add some other tests that check related situations (and happen to highlight unrelated limitations). --- .../ArrayBound/assumption-reporting.c | 43 ++++++++++++++++++- 1 file changed, 42 insertions(+), 1 deletion(-) diff --git a/clang/test/Analysis/ArrayBound/assumption-reporting.c b/clang/test/Analysis/ArrayBound/assumption-reporting.c index bffd5d9bc35b5..f14375476f0de 100644 --- a/clang/test/Analysis/ArrayBound/assumption-reporting.c +++ b/clang/test/Analysis/ArrayBound/assumption-reporting.c @@ -54,7 +54,32 @@ int assumingLower(int arg) { if (arg >= 10) return 0; int a = TenElements[arg]; - // expected-note@-1 {{Assuming index is non-negative}} + // expected-note-re@-1 {{Assuming index is non-negative{{$}}}} + int b = TenElements[arg + 10]; + // expected-warning@-1 {{Out of bound access to memory after the end of 'TenElements'}} + // expected-note@-2 {{Access of 'TenElements' at an overflowing index, while it holds only 10 'int' elements}} + return a + b; +} + +int assumingLowerOnlyUseIndex(int arg) { + // This testcase validates that the note tag says that the _index_ is + // non-negative when there is no upper bound assumption -- even in the case + // when the extent (which is totally irrelevant) is not an integer multiple + // of the element size. + + char TwoAndHalfInts[10] = {0}; + // expected-note@+2 {{Assuming 'arg' is < 2}} + // expected-note@+1 {{Taking false branch}} + if (arg >= 2) + return 0; + + int a = ((int*)TwoAndHalfInts)[arg]; + // expected-note@-1 {{Assuming byte offset is non-negative and less than 10, the extent of 'TwoAndHalfInts'}} + // FIXME: This assumption note should say + // {{Assuming index is non-negative{{$}}}} + // but the comparison logic is not smart enough to deduce that if arg < 2, + // then 4*arg < 10. + int b = TenElements[arg + 10]; // expected-warning@-1 {{Out of bound access to memory after the end of 'TenElements'}} // expected-note@-2 {{Access of 'TenElements' at an overflowing index, while it holds only 10 'int' elements}} @@ -94,6 +119,22 @@ int assumingUpperIrrelevant(int arg) { return a + b; } +int assumingLowerIrrelevant(int arg) { + // FIXME: Analogously to `assumingUpperIrrelevant` here the assumption + // "assuming index is non-negative" is irrelevant, but printed. + // + // expected-note@+2 {{Assuming 'arg' is < 10}} + // expected-note@+1 {{Taking false branch}} + if (arg >= 10) + return 0; + int a = TenElements[arg]; + // expected-note-re@-1 {{Assuming index is non-negative{{$}}}} + int b = TenElements[arg - 10]; + // expected-warning@-1 {{Out of bound access to memory preceding 'TenElements'}} + // expected-note@-2 {{Access of 'TenElements' at a negative index}} + return a + b; +} + int assumingUpperUnsigned(unsigned arg) { int a = TenElements[arg]; // expected-note@-1 {{Assuming index is less than 10, the number of 'int' elements in 'TenElements'}} From 1dc5d19643418668111eb80eb9a4966365f404c0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Mon, 29 Jun 2026 13:56:02 +0200 Subject: [PATCH 21/22] Test that extent is not interesting in underflows --- .../Analysis/ArrayBound/assumption-reporting.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/clang/test/Analysis/ArrayBound/assumption-reporting.c b/clang/test/Analysis/ArrayBound/assumption-reporting.c index f14375476f0de..5ba62a1522d67 100644 --- a/clang/test/Analysis/ArrayBound/assumption-reporting.c +++ b/clang/test/Analysis/ArrayBound/assumption-reporting.c @@ -232,8 +232,8 @@ int assumingExtent(int arg) { } int *extentInterestingness(int arg) { - // Verify that in an out-of-bounds access issue the extent is marked as - // interesting (so assumptions about its value are printed). + // Verify that in a buffer overflow issue the extent is marked as interesting + // (so assumptions about its value are printed). int *mem = (int*)malloc(arg); TenElements[arg] = 123; @@ -244,6 +244,18 @@ int *extentInterestingness(int arg) { // expected-note@-2 {{Access of 'int' element in the heap area at index 12}} } +int *extentNonInterestingInUnderflow(int arg) { + // Verify that in a buffer underflow issue the extent is _not_ marked as + // interesting (because it does not influence anything). + int *mem = (int*)malloc(arg); + + TenElements[arg] = 123; // no-note: arg is not interesting + + return &mem[-2]; + // expected-warning@-1 {{Out of bound access to memory preceding the heap area}} + // expected-note@-2 {{Access of 'int' element in the heap area at negative index -2}} +} + int triggeredByAnyReport(int arg) { // Verify that note tags explaining the assumptions made by ArrayBound are // not limited to ArrayBound reports but will appear on any bug report (that From f863c672ff5b2acc808ea8b143160700d900e9ef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Tue, 21 Jul 2026 12:11:21 +0200 Subject: [PATCH 22/22] Satisfy git-clang-format --- .../StaticAnalyzer/Checkers/ArrayBoundChecker.cpp | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp index 308b2c4742d29..c5a5d5c7cf3e6 100644 --- a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp @@ -498,8 +498,8 @@ static const char *getPreposition(const bounds::CheckResult &R) { : (R.mayOverflow() ? "after the end of" : "within")); } -static BugDescription describeInvalidAccess(bounds::CheckResult Res, StringRef RegName, - SizeUnit SU) { +static BugDescription describeInvalidAccess(bounds::CheckResult Res, + StringRef RegName, SizeUnit SU) { std::optional<int64_t> OffsetN = getConcreteValue(Res.getOffset()); std::optional<int64_t> ExtentN = getConcreteValue(Res.getExtentIfRelevant()); @@ -567,8 +567,9 @@ static BugDescription describeTaintBug(StringRef RegName, /// report \p BR. When the access wasn't ambiguous or the the assumption is /// irrelevant for \p BR, this returns the empty string (which signifies "do /// not emit a note tag" when returned by a note tag callback). -static std::string getAssumptionNote(bounds::CheckResult Res, PathSensitiveBugReport &BR, - StringRef RegName, SizeUnit SU) { +static std::string getAssumptionNote(bounds::CheckResult Res, + PathSensitiveBugReport &BR, + StringRef RegName, SizeUnit SU) { bool ShouldReportNonNegative = Res.mayUnderflow(); if (!providesInformationAboutInteresting(Res.getOffset(), BR)) { std::optional<NonLoc> E = Res.getExtentIfRelevant(); @@ -711,8 +712,10 @@ void ArrayBoundChecker::handleAccessExpr(const Expr *E, C.addTransition(Res.getValidState(), T); } -bounds::CheckResult bounds::checkBounds(ProgramStateRef State, SValBuilder &SVB, NonLoc Offset, - std::optional<NonLoc> Extent, bounds::CheckFlags Flags) { +bounds::CheckResult bounds::checkBounds(ProgramStateRef State, SValBuilder &SVB, + NonLoc Offset, + std::optional<NonLoc> Extent, + bounds::CheckFlags Flags) { bounds::CheckResult Res(Offset); _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
