Author: Donát Nagy Date: 2026-08-29T21:40:40+02:00 New Revision: c5af4f17fbd72d0da0f41b6dc07a296d8a36a3b0
URL: https://github.com/llvm/llvm-project/commit/c5af4f17fbd72d0da0f41b6dc07a296d8a36a3b0 DIFF: https://github.com/llvm/llvm-project/commit/c5af4f17fbd72d0da0f41b6dc07a296d8a36a3b0.diff LOG: [analyzer] Fix handling of zero-sized elements in ArrayBound (#218712) Previously the `security.ArrayBound` checker mishandled the following C code under non-windows platforms where `sizeof(struct Empty) == 0`: ```c struct Empty {}; struct Empty Array[10]; struct Empty foo(void) { return Array[5]; } ``` Here the checker produced a false positive with explanation "Access of 'Array' at byte offset 0, while it holds only 0 byte" -- that is, the checker said that this accesses the past-the-end pointer, which is usually invalid. This commit suppresses this false positive by saying that accessing a zero-sized object starting at the past-the-end pointer is valid. This is implemented by adding an `AlsoAcceptEquality` flag for `checkBounds`. This flag will also be useful for implementing checkers that check pointer arithmetic (where forming the past-the-end pointer is completely valid). This commit also includes a small grammatical fix: the explanation notes now say "0 bytes" or "0 ... elements" instead of "0 byte" / "0 element". Added: Modified: clang/include/clang/StaticAnalyzer/Checkers/BoundsChecking.h clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp clang/lib/StaticAnalyzer/Checkers/BoundsChecking.cpp clang/test/Analysis/ArrayBound/verbose-tests.c Removed: ################################################################################ diff --git a/clang/include/clang/StaticAnalyzer/Checkers/BoundsChecking.h b/clang/include/clang/StaticAnalyzer/Checkers/BoundsChecking.h index 2c8469694b661..3fc7fa4d470f9 100644 --- a/clang/include/clang/StaticAnalyzer/Checkers/BoundsChecking.h +++ b/clang/include/clang/StaticAnalyzer/Checkers/BoundsChecking.h @@ -29,6 +29,7 @@ namespace clang::ento::bounds { struct CheckFlags { unsigned CheckUnderflow : 1; unsigned OffsetObviouslyNonnegative : 1; + unsigned AlsoAcceptEquality : 1; }; class CheckResult; @@ -91,16 +92,29 @@ class CheckResult { ProgramStateRef InBoundsState = nullptr; }; -// Evaluate the comparison Value < Threshold with the help of the custom -// simplification algorithm. Return a pair of states, where the first one -// corresponds to "value below threshold" and the second corresponds to "value -// at or above threshold". Returns {nullptr, nullptr} in the case when the -// evaluation fails. -// If the optional argument CheckEquality is true, then use BO_EQ instead of -// the default BO_LT after consistently applying the same simplification steps. +enum class Comparison { LT, LE, EQ }; + +inline BinaryOperator::Opcode asOpcode(Comparison C) { + switch (C) { + case Comparison::LT: + return BO_LT; + case Comparison::LE: + return BO_LE; + case Comparison::EQ: + return BO_EQ; + } + llvm_unreachable("unhandled Comparison kind"); +} + +// Evaluates the comparison \p Value \p CmpKind \p Threshold; and splits the +// state, returning a pair {ComparisonTrueState, ComparisonFalseState}. +// May return {nullptr, nullptr} when `evalBinOp` fails. +// This uses a custom simplification algorithm that approximates a mathematical +// comparison between the two numbers; ignoring overflow and heuristically +// avoiding the automatic conversions done by the underlying `evalBinOp`. std::pair<ProgramStateRef, ProgramStateRef> compareValueToThreshold(ProgramStateRef State, SValBuilder &SVB, NonLoc Value, - NonLoc Threshold, bool CheckEquality = false); + NonLoc Threshold, Comparison CmpKind); } // namespace clang::ento::bounds #endif // LLVM_CLANG_STATICANALYZER_CHECKERS_BOUNDSCHECKING_H diff --git a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp index c4054ee8cf5c0..3acbf7cd75ab8 100644 --- a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp @@ -200,6 +200,14 @@ static bool isDeterminedByInterestingSymbol(SVal SV, return false; } +static int64_t getElementSize(const ElementRegion *ER, SValBuilder &SVB) { + QualType ElemType = ER->getElementType(); + + assert(!ElemType->isIncompleteType() && "ElemType cannot be incomplete"); + + return SVB.getContext().getTypeSizeInChars(ElemType).getQuantity(); +} + /// For a given \p CurRegion that can be represented as a symbolic expression /// Arr[Idx] (or perhaps Arr[Idx1][Idx2] etc.), return the parent memory block /// Arr and the distance of Location from the beginning of Arr (expressed in a @@ -222,17 +230,8 @@ computeOffset(ProgramStateRef State, SValBuilder &SVB, if (!Index) return std::nullopt; - QualType ElemType = CurRegion->getElementType(); - - // FIXME: The following early return was presumably added to safeguard the - // getTypeSizeInChars() call (which doesn't accept an incomplete type), but - // it seems that `ElemType` cannot be incomplete at this point. - if (ElemType->isIncompleteType()) - return std::nullopt; - // Calculate Delta = Index * sizeof(ElemType). - NonLoc Size = SVB.makeArrayIndex( - SVB.getContext().getTypeSizeInChars(ElemType).getQuantity()); + NonLoc Size = SVB.makeArrayIndex(getElementSize(CurRegion, SVB)); auto Delta = EvalBinOp(BO_Mul, *Index, Size); if (!Delta) return std::nullopt; @@ -324,7 +323,7 @@ static BugDescription describeInvalidAccess(bounds::CheckResult Res, Out << ' ' << SU.asElementName(); - if (*ExtentN > 1) + if (*ExtentN != 1) Out << "s"; } @@ -452,7 +451,8 @@ void ArrayBoundChecker::handleAccessExpr(const Expr *E, bounds::CheckFlags Flags = { /*CheckUnderflow=*/!(isa<SymbolicRegion>(Reg) && isa<UnknownSpaceRegion>(Space)), - /*OffsetObviouslyNonnegative=*/isOffsetObviouslyNonnegative(E, C)}; + /*OffsetObviouslyNonnegative=*/isOffsetObviouslyNonnegative(E, C), + /*AlsoAcceptEquality=*/(getElementSize(AccessedER, SVB) == 0)}; bounds::CheckResult Res = checkBounds(State, SVB, ByteOffset, Extent, Flags); @@ -472,7 +472,7 @@ void ArrayBoundChecker::handleAccessExpr(const Expr *E, // forms the past-the-end pointer without actually dereferencing it. auto [EqualsToThreshold, NotEqualToThreshold] = bounds::compareValueToThreshold(State, SVB, ByteOffset, *Extent, - /*CheckEquality=*/true); + bounds::Comparison::EQ); if (EqualsToThreshold && !NotEqualToThreshold) { C.addTransition(EqualsToThreshold); return; diff --git a/clang/lib/StaticAnalyzer/Checkers/BoundsChecking.cpp b/clang/lib/StaticAnalyzer/Checkers/BoundsChecking.cpp index 9c11e9e2bd69b..0bb06b3e015aa 100644 --- a/clang/lib/StaticAnalyzer/Checkers/BoundsChecking.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/BoundsChecking.cpp @@ -77,7 +77,7 @@ static bool isUnsigned(SValBuilder &SVB, NonLoc Value) { std::pair<ProgramStateRef, ProgramStateRef> bounds::compareValueToThreshold(ProgramStateRef State, SValBuilder &SVB, NonLoc Value, NonLoc Threshold, - bool CheckEquality) { + Comparison CmpKind) { if (auto ConcreteThreshold = Threshold.getAs<nonloc::ConcreteInt>()) { std::tie(Value, Threshold) = getSimplifiedOffsets(Value, *ConcreteThreshold, SVB); @@ -91,16 +91,16 @@ bounds::compareValueToThreshold(ProgramStateRef State, SValBuilder &SVB, // To avoid automatic conversions, we evaluate the "obvious" cases without // calling `evalBinOpNN`: if (isNegative(SVB, State, Value) && isUnsigned(SVB, Threshold)) { - if (CheckEquality) { - // negative_value == unsigned_threshold is always false + if (CmpKind == Comparison::EQ) { + // negative == unsigned is always false return {nullptr, State}; } - // negative_value < unsigned_threshold is always true + // negative < unsigned and negative <= unsigned are always true return {State, nullptr}; } if (isUnsigned(SVB, Value) && isNegative(SVB, State, Threshold)) { - // unsigned_value == negative_threshold and - // unsigned_value < negative_threshold are both always false + // unsigned == negative, unsigned < negative and unsigned <= negative are + // all always false return {nullptr, State}; } // FIXME: These special cases are sufficient for handling real-world @@ -114,7 +114,7 @@ bounds::compareValueToThreshold(ProgramStateRef State, SValBuilder &SVB, // evaluate these "mathematical" comparisons through a separate pathway would // be a step backwards in this sense. - const BinaryOperatorKind OpKind = CheckEquality ? BO_EQ : BO_LT; + const BinaryOperatorKind OpKind = asOpcode(CmpKind); auto BelowThreshold = SVB.evalBinOpNN(State, OpKind, Value, Threshold, SVB.getConditionType()) .getAs<NonLoc>(); @@ -134,8 +134,8 @@ bounds::CheckResult bounds::checkBounds(ProgramStateRef State, SValBuilder &SVB, // CHECK LOWER BOUND if (Flags.CheckUnderflow) { - auto [PrecedesLowerBound, WithinLowerBound] = - compareValueToThreshold(State, SVB, Offset, SVB.makeZeroArrayIndex()); + auto [PrecedesLowerBound, WithinLowerBound] = compareValueToThreshold( + State, SVB, Offset, SVB.makeZeroArrayIndex(), Comparison::LT); if (PrecedesLowerBound) { // The analyzer thinks that the offset may be invalid (negative)... @@ -183,14 +183,9 @@ bounds::CheckResult bounds::checkBounds(ProgramStateRef State, SValBuilder &SVB, // CHECK UPPER BOUND if (Extent) { - // In a situation where both underflow and overflow are possible (but the - // index is either tainted or known to be invalid), the logic of this - // checker will first assume that the offset is non-negative, and then - // (with this additional assumption) it will detect an overflow error. - // In this situation the warning message should mention both possibilities. - + Comparison CK = Flags.AlsoAcceptEquality ? Comparison::LE : Comparison::LT; auto [WithinUpperBound, ExceedsUpperBound] = - compareValueToThreshold(State, SVB, Offset, *Extent); + compareValueToThreshold(State, SVB, Offset, *Extent, /*CmpKind=*/CK); if (ExceedsUpperBound) { // The offset may be invalid (>= Size)... diff --git a/clang/test/Analysis/ArrayBound/verbose-tests.c b/clang/test/Analysis/ArrayBound/verbose-tests.c index f4619fcc14006..29e80947d2f54 100644 --- a/clang/test/Analysis/ArrayBound/verbose-tests.c +++ b/clang/test/Analysis/ArrayBound/verbose-tests.c @@ -470,14 +470,48 @@ int *nothingIsCertain(int x, int y) { // We disable this test under Windows because 'struct Empty {}' has a nozero // size on that platform. Note that '_WIN32' is also defined on 64-bit systems // and is apparently the customary way to detect Windows OS. +// The empty struct also has nonzero size under C++ so these corner cases are +// only relevant under C. struct Empty {}; struct Empty ZeroSizeElements[10]; struct Empty zeroSizeElements(void) { - // FIXME: We probably shouldn't report this access. - return ZeroSizeElements[5]; + // Here the offset and extent are both 0, which previously caused a false + // positive out of bounds report. + return ZeroSizeElements[5]; // no-warning +} + +struct Empty zeroSizeElementsNegativeIndex(void) { + // The negative index does not change anything, it still means offset = 0. + return ZeroSizeElements[-5]; // no-warning +} + +int zeroSizeContainerIntAccess(void) { + return ((int*)ZeroSizeElements)[5]; // expected-warning@-1 {{Out of bound access to memory after the end of 'ZeroSizeElements'}} - // expected-note@-2 {{Access of 'ZeroSizeElements' at byte offset 0, while it holds only 0 byte}} + // expected-note@-2 {{Access of 'ZeroSizeElements' at index 5, while it holds only 0 'int' elements}} } + +struct Empty zeroSizeAccessOfPastTheEnd(void) { + // We currently allow zero-sized access of past-the-end pointers as a side + // effect of the logic that handles the testcase 'zeroSizeElements'. + return *(struct Empty *)(TenElements + 10); // no-warning +} + +struct Empty zeroSizeAccessFarAway(void) { + // However, zero-sized access of other out-of-bounds pointers is reported + // (with byte offsets, because the zero-sized element is not suitable for + // calculating indices). + return *(struct Empty *)(TenElements + 20); + // expected-warning@-1 {{Out of bound access to memory after the end of 'TenElements'}} + // expected-note@-2 {{Access of 'TenElements' at byte offset 80, while it holds only 40 bytes}} +} + +struct Empty zeroSizeAccessUnderflow(void) { + return *(struct Empty *)(TenElements - 10); + // expected-warning@-1 {{Out of bound access to memory preceding 'TenElements'}} + // expected-note@-2 {{Access of 'TenElements' at negative byte offset -40}} +} + #endif _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
