Author: Donát Nagy Date: 2026-08-25T11:59:18+02:00 New Revision: a6577187e29b3af3bae0140e24c53ec605bcd1d3
URL: https://github.com/llvm/llvm-project/commit/a6577187e29b3af3bae0140e24c53ec605bcd1d3 DIFF: https://github.com/llvm/llvm-project/commit/a6577187e29b3af3bae0140e24c53ec605bcd1d3.diff LOG: [analyzer] Implement potential underflow warnings (#216077) The `security.ArrayBound` checker is able to report potential out-of-bounds access when it detects that the accessed offset is tainted (potentially attacker-controlled). However, until now this only reported cases where _overflow_ was possible with the tainted offset. (This is probably an accidental oversight -- in the old implementation it was easy to forget adding a second check that would report the "potential underflow with tainted offset" case.) This commit corrects this oversight and ensures that potential underflow with a tainted offset is also correctly reported. Co-authored-by: Balázs Benics <[email protected]> Added: Modified: clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp clang/test/Analysis/ArrayBound/verbose-tests.c Removed: ################################################################################ diff --git a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp index 5201eeddca16f..c4054ee8cf5c0 100644 --- a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp @@ -278,6 +278,8 @@ static StringRef getPreposition(const bounds::CheckResult &R) { static BugDescription describeInvalidAccess(bounds::CheckResult Res, StringRef RegName, SizeUnit SU) { + assert(Res.mayBeInvalid()); + std::optional<int64_t> OffsetN = getConcreteValue(Res.getOffset()); std::optional<int64_t> ExtentN = getConcreteValue(Res.getExtentIfMayOverflow()); @@ -331,13 +333,16 @@ static BugDescription describeInvalidAccess(bounds::CheckResult Res, std::string(Buf)}; } -static BugDescription describeTaintBug(StringRef RegName, StringRef OffsetName, - bool AlsoMentionUnderflow) { +static BugDescription describeTaintBug(bounds::CheckResult Res, + StringRef RegName, + StringRef OffsetName) { + assert(Res.mayBeInvalid()); 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", - RegName, OffsetName, - AlsoMentionUnderflow ? "negative or " : "")}; + formatv("Access of {0} with a tainted {1} that may be{2}{3}{4}", + RegName, OffsetName, Res.mayUnderflow() ? " negative" : "", + (Res.mayUnderflow() && Res.mayOverflow()) ? " or" : "", + Res.mayOverflow() ? " too large" : "")}; } /// When the access was ambiguous (that is, mayBeInBounds() && mayBeInvalid()), @@ -480,10 +485,7 @@ void ArrayBoundChecker::handleAccessExpr(const Expr *E, return; } - // FIXME: Remove `Res.mayOverflow()` and provide diagnostics for the case - // when the tainted access operation cannot overflow but can underflow. - // (This is an NFC commit, so I cannot include this improvement.) - if (Res.mayOverflow() && isTainted(State, ByteOffset)) { + 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". @@ -492,9 +494,9 @@ void ArrayBoundChecker::handleAccessExpr(const Expr *E, if (isTainted(State, ASE->getIdx(), C.getStackFrame())) OffsetName = "index"; - BugDescription Desc = - describeTaintBug(RegName, OffsetName, Res.mayUnderflow()); - reportOOB(C, State, Desc, ByteOffset, Extent, /*IsTaintBug=*/true); + BugDescription Desc = describeTaintBug(Res, RegName, OffsetName); + reportOOB(C, State, Desc, ByteOffset, Res.getExtentIfMayOverflow(), + /*IsTaintBug=*/true); return; } diff --git a/clang/test/Analysis/ArrayBound/verbose-tests.c b/clang/test/Analysis/ArrayBound/verbose-tests.c index c0b1f2a8ae6be..f4619fcc14006 100644 --- a/clang/test/Analysis/ArrayBound/verbose-tests.c +++ b/clang/test/Analysis/ArrayBound/verbose-tests.c @@ -100,6 +100,22 @@ void taintedIndexNonneg(void) { // expected-note@-2 {{Access of 'TenElements' with a tainted index that may be too large}} } +void taintedIndexNonlarge(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 < 10}} + // expected-note@+1 {{Taking false branch}} + if (index >= 10) + 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 negative}} +} + void taintedIndexUnsigned(void) { unsigned index; scanf("%u", &index); @@ -144,6 +160,18 @@ void taintedOffset(void) { // expected-note@-2 {{Access of 'TenElements' with a tainted offset that may be negative or too large}} } +void taintedIndexCast(void) { + // '(unsigned)index < 10' guarantees that index is non-negative and less than + // 10, because the cast converts negative values to large positive values. + int index; + scanf("%d", &index); + if ((unsigned)index < 10) + TenElements[index] = 5; // no-warning + unsigned uidx = (unsigned)index; + if (uidx < 10) + TenElements[index] = 5; // no-warning +} + void arrayOverflow(void) { TenElements[12] = 5; // expected-warning@-1 {{Out of bound access to memory after the end of 'TenElements'}} @@ -363,6 +391,34 @@ int *mallocRegionDeref(void) { return mem; } +void taintedExtentNotInteresting(void) { + // This is a potential underflow report, so the extent is not interesting + // (and e.g. we should not print notes about its taintedness). + int n; + scanf("%d", &n); + // expected-note@+4 {{Assuming 'n' is >= 1}} + // expected-note@+3 {{Left side of '||' is false}} + // expected-note@+2 {{Assuming 'n' is <= 100}} + // expected-note@+1 {{Taking false branch}} + if (n < 1 || n > 100) + return; + + char *p = (char *)malloc(n); + int index; + // expected-note@+2 {{Taint originated here}} + // expected-note@+1 {{Taint propagated to the 2nd argument}} + scanf("%d", &index); + // expected-note@+2 {{Assuming 'index' is < 'n'}} + // expected-note@+1 {{Taking false branch}} + if (index >= n) { + free(p); + return; + } + p[index] = 5; + // expected-warning@-1 {{Potential out of bound access to the heap area with tainted index}} + // expected-note@-2 {{Access of the heap area with a tainted index that may be negative}} +} + void *alloca(size_t size); int allocaRegion(void) { _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
