diff --git a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp index 29eb932584027..f82288f1099e8 100644 --- a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp @@ -83,6 +83,8 @@ class StateUpdateReporter { AssumedUpperBound = UpperBoundVal; } + bool assumedNonNegative() { return AssumedNonNegative; } + const NoteTag *createNoteTag(CheckerContext &C) const; private: @@ -402,7 +404,8 @@ static bool tryDividePair(std::optional &Val1, } static Messages getExceedsMsgs(ASTContext &ACtx, const SubRegion *Region, - NonLoc Offset, NonLoc Extent, SVal Location) { + NonLoc Offset, NonLoc Extent, SVal Location, + bool AlsoMentionUnderflow) { std::string RegName = getRegionName(Region); const auto *EReg = Location.getAsRegion()->getAs(); assert(EReg && "this checker only handles element access"); @@ -414,6 +417,7 @@ static Messages getExceedsMsgs(ASTContext &ACtx, const SubRegion *Region, int64_t ElemSize = ACtx.getTypeSizeInChars(ElemType).getQuantity(); bool UseByteOffsets = !tryDividePair(OffsetN, ExtentN, ElemSize); + const char *OffsetOrIndex = UseByteOffsets ? "byte offset" : "index"; SmallString<256> Buf; llvm::raw_svector_ostream Out(Buf); @@ -421,10 +425,12 @@ static Messages getExceedsMsgs(ASTContext &ACtx, const SubRegion *Region, if (!ExtentN && !UseByteOffsets) Out << "'" << ElemType.getAsString() << "' element in "; Out << RegName << " at "; - if (OffsetN) { - Out << (UseByteOffsets ? "byte offset " : "index ") << *OffsetN; + if (AlsoMentionUnderflow) { + Out << "a negative or overflowing " << OffsetOrIndex; + } else if (OffsetN) { + Out << OffsetOrIndex << " " << *OffsetN; } else { - Out << "an overflowing " << (UseByteOffsets ? "byte offset" : "index"); + Out << "an overflowing " << OffsetOrIndex; } if (ExtentN) { Out << ", while it holds only "; @@ -441,17 +447,20 @@ static Messages getExceedsMsgs(ASTContext &ACtx, const SubRegion *Region, Out << "s"; } - return { - formatv("Out of bound access to memory after the end of {0}", RegName), - std::string(Buf)}; + return {formatv("Out of bound access to memory {0} {1}", + AlsoMentionUnderflow ? "around" : "after the end of", + RegName), + std::string(Buf)}; } -static Messages getTaintMsgs(const SubRegion *Region, const char *OffsetName) { +static Messages getTaintMsgs(const SubRegion *Region, const char *OffsetName, + bool AlsoMentionUnderflow) { std::string RegName = getRegionName(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 too large", - RegName, OffsetName)}; + formatv("Access of {0} with a tainted {1} that may be {2}too large", + RegName, OffsetName, + AlsoMentionUnderflow ? "negative or " : "")}; } const NoteTag *StateUpdateReporter::createNoteTag(CheckerContext &C) const { @@ -600,6 +609,13 @@ void ArrayBoundCheckerV2::performCheck(const Expr *E, CheckerContext &C) const { // CHECK UPPER BOUND DefinedOrUnknownSVal Size = getDynamicExtent(State, Reg, SVB); if (auto KnownSize = Size.getAs()) { + // In a situation where both overflow 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); @@ -615,8 +631,9 @@ void ArrayBoundCheckerV2::performCheck(const Expr *E, CheckerContext &C) const { return; } - Messages Msgs = getExceedsMsgs(C.getASTContext(), Reg, ByteOffset, - *KnownSize, Location); + Messages Msgs = + getExceedsMsgs(C.getASTContext(), Reg, ByteOffset, *KnownSize, + Location, AlsoMentionUnderflow); reportOOB(C, ExceedsUpperBound, Msgs, ByteOffset, KnownSize); return; } @@ -632,7 +649,7 @@ void ArrayBoundCheckerV2::performCheck(const Expr *E, CheckerContext &C) const { if (isTainted(State, ASE->getIdx(), C.getLocationContext())) OffsetName = "index"; - Messages Msgs = getTaintMsgs(Reg, OffsetName); + Messages Msgs = getTaintMsgs(Reg, OffsetName, AlsoMentionUnderflow); reportOOB(C, ExceedsUpperBound, Msgs, ByteOffset, KnownSize, /*IsTaintBug=*/true); return; diff --git a/clang/test/Analysis/out-of-bounds-diagnostics.c b/clang/test/Analysis/out-of-bounds-diagnostics.c index 0c3c67c6a546a..92f983d8b1561 100644 --- a/clang/test/Analysis/out-of-bounds-diagnostics.c +++ b/clang/test/Analysis/out-of-bounds-diagnostics.c @@ -24,6 +24,33 @@ void taintedIndex(void) { scanf("%d", &index); // expected-note@-1 {{Taint originated here}} // expected-note@-2 {{Taint propagated to the 2nd argument}} + TenElements[index] = 5; + // expected-warning@-1 {{Potential out of bound access to 'TenElements' with tainted index}} + // expected-note@-2 {{Access of 'TenElements' with a tainted index that may be negative or too large}} +} + +void taintedIndexNonneg(void) { + int index; + scanf("%d", &index); + // expected-note@-1 {{Taint originated here}} + // expected-note@-2 {{Taint propagated to the 2nd argument}} + + // expected-note@+2 {{Assuming 'index' is >= 0}} + // expected-note@+1 {{Taking false branch}} + if (index < 0) + return; + + TenElements[index] = 5; + // expected-warning@-1 {{Potential out of bound access to 'TenElements' with tainted index}} + // expected-note@-2 {{Access of 'TenElements' with a tainted index that may be too large}} +} + +void taintedIndexUnsigned(void) { + unsigned index; + scanf("%u", &index); + // expected-note@-1 {{Taint originated here}} + // expected-note@-2 {{Taint propagated to the 2nd argument}} + TenElements[index] = 5; // expected-warning@-1 {{Potential out of bound access to 'TenElements' with tainted index}} // expected-note@-2 {{Access of 'TenElements' with a tainted index that may be too large}} @@ -59,7 +86,7 @@ void taintedOffset(void) { int *p = TenElements + index; p[0] = 5; // expected-warning@-1 {{Potential out of bound access to 'TenElements' with tainted offset}} - // expected-note@-2 {{Access of 'TenElements' with a tainted offset that may be too large}} + // expected-note@-2 {{Access of 'TenElements' with a tainted offset that may be negative or too large}} } void arrayOverflow(void) { @@ -109,6 +136,33 @@ int *potentialAfterTheEndPtr(int idx) { // &TenElements[idx]. } +int overflowOrUnderflow(int arg) { + // expected-note@+2 {{Assuming 'arg' is < 0}} + // expected-note@+1 {{Taking false branch}} + if (arg >= 0) + return 0; + + return TenElements[arg - 1]; + // expected-warning@-1 {{Out of bound access to memory around 'TenElements'}} + // expected-note@-2 {{Access of 'TenElements' at a negative or overflowing index, while it holds only 10 'int' elements}} +} + +char TwoElements[2] = {11, 22}; +char overflowOrUnderflowConcrete(int arg) { + // expected-note@#cond {{Assuming 'arg' is < 3}} + // expected-note@#cond {{Left side of '||' is false}} + // expected-note@#cond {{Assuming 'arg' is not equal to 0}} + // expected-note@#cond {{Left side of '||' is false}} + // expected-note@#cond {{Assuming 'arg' is not equal to 1}} + // expected-note@#cond {{Taking false branch}} + if (arg >= 3 || arg == 0 || arg == 1) // #cond + return 0; + + return TwoElements[arg]; + // expected-warning@-1 {{Out of bound access to memory around 'TwoElements'}} + // expected-note@-2 {{Access of 'TwoElements' at a negative or overflowing index, while it holds only 2 'char' elements}} +} + int scalar; int scalarOverflow(void) { return (&scalar)[1]; diff --git a/clang/test/Analysis/taint-diagnostic-visitor.c b/clang/test/Analysis/taint-diagnostic-visitor.c index 020e9579ac535..2ba7d9938fc3d 100644 --- a/clang/test/Analysis/taint-diagnostic-visitor.c +++ b/clang/test/Analysis/taint-diagnostic-visitor.c @@ -30,7 +30,7 @@ int taintDiagnosticOutOfBound(void) { scanf("%d", &index); // expected-note {{Taint originated here}} // expected-note@-1 {{Taint propagated to the 2nd argument}} return Array[index]; // expected-warning {{Potential out of bound access to 'Array' with tainted index}} - // expected-note@-1 {{Access of 'Array' with a tainted index that may be too large}} + // expected-note@-1 {{Access of 'Array' with a tainted index}} } int taintDiagnosticDivZero(int operand) {