================
@@ -595,139 +148,74 @@ void ArrayBoundChecker::performCheck(const Expr *E,
CheckerContext &C) const {
auto [Reg, ByteOffset] = *RawOffset;
- // 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);
-
- // 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.
- // 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 (PrecedesLowerBound) {
- // The analyzer thinks that the offset may be invalid (negative)...
-
- if (isOffsetObviouslyNonnegative(E, C)) {
- // ...but the offset is obviously non-negative (clear array subscript
- // with an unsigned index), so we're in a buggy situation.
-
- // TODO: Currently the analyzer ignores many casts (e.g. signed ->
- // unsigned casts), so it can easily reach states where it will load a
- // signed (and negative) value from an unsigned variable. This sanity
- // check is a duct tape "solution" that silences most of the ugly false
- // positives that are caused by this buggy behavior. Note that this is
- // not a complete solution: this cannot silence reports where pointer
- // arithmetic complicates the picture and cannot ensure modeling of the
- // "unsigned index is positive with highest bit set" cases which are
- // "usurped" by the nonsense "unsigned index is negative" case.
- // For more information about this topic, see the umbrella ticket
- // https://github.com/llvm/llvm-project/issues/39492
- // TODO: Remove this hack once 'SymbolCast's are modeled properly.
-
- if (!WithinLowerBound) {
- // The state is completely nonsense -- let's just sink it!
- C.addSink();
- return;
- }
- // 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 {
- if (!WithinLowerBound) {
- // ...and it cannot be valid (>= 0), so report an error.
- Messages Msgs = getNonTaintMsgs(C.getASTContext(), Space, Reg,
- ByteOffset, /*Extent=*/std::nullopt,
- Location, BadOffsetKind::Negative);
- reportOOB(C, PrecedesLowerBound, Msgs, ByteOffset, std::nullopt);
- return;
- }
- // ...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();
- }
- }
+ 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()),
+ };
- // 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 (WithinLowerBound)
- State = WithinLowerBound;
- }
+ BoundsCheckResult Res = checkBounds(State, SVB, ByteOffset, Extent, Flags);
+
+ std::string RN = getRegionName(Reg->getMemorySpace(C.getState()), Reg);
- // CHECK UPPER BOUND
- DefinedOrUnknownSVal Size = getDynamicExtent(State, Reg, SVB);
- if (auto KnownSize = Size.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
- // (with this additional assumption) it will detect an overflow error.
- // In this situation the warning message should mention both possibilities.
- bool AlsoMentionUnderflow = SUR.assumedNonNegative();
-
- auto [WithinUpperBound, ExceedsUpperBound] =
- compareValueToThreshold(State, ByteOffset, *KnownSize, SVB);
-
- if (ExceedsUpperBound) {
- // The offset may be invalid (>= Size)...
- 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)) {
- C.addTransition(ExceedsUpperBound, SUR.createNoteTag(C));
- return;
- }
-
- BadOffsetKind Problem = AlsoMentionUnderflow
- ? BadOffsetKind::Indeterminate
- : BadOffsetKind::Overflowing;
- Messages Msgs =
- getNonTaintMsgs(C.getASTContext(), Space, Reg, 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";
-
- Messages Msgs =
- getTaintMsgs(Space, Reg, OffsetName, AlsoMentionUnderflow);
- reportOOB(C, ExceedsUpperBound, Msgs, ByteOffset, KnownSize,
- /*IsTaintBug=*/true);
- return;
- }
- // ...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);
+ switch (Res.getKind()) {
+ case BoundsCheckResult::Kind::Paradox:
+ // The current state is paradoxical (due to bad modeling of casts we
+ // assumed that an unsigned value is negative), so we should sink the
+ // execution path.
+ C.addSink();
+ return;
+
+ case BoundsCheckResult::Kind::Valid: {
+ const NoteTag *Tag = nullptr;
+ if (Res.hasAssumption()) {
+ SizeUnit SU = SizeUnit::forExpr(E, C);
+ Tag = C.getNoteTag(
+ [Res, RN, SU](PathSensitiveBugReport &BR) -> std::string {
+ return Res.getMessage(BR, RN, SU);
+ });
}
- // 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;
+ C.addTransition(Res.getState(), Tag);
+ return;
+ }
+ case BoundsCheckResult::Kind::TaintBug: {
+ // 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 (taint::isTainted(State, ASE->getIdx(), C.getStackFrame()))
+ OffsetName = "index";
+
+ Messages Msgs =
+ getTaintMsgs(RN, OffsetName,
+ /*AlsoMentionUnderflow=*/Res.assumedNonNegative());
+ reportOOB(C, Res.getState(), Msgs, ByteOffset, Extent,
+ /*IsTaintBug=*/true);
+ return;
+ }
+ default: {
----------------
NagyDonat wrote:
Done in
https://github.com/llvm/llvm-project/pull/202372/commits/07426529998d6313ca881befc62ce06f3df03add
https://github.com/llvm/llvm-project/pull/202372
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
