https://github.com/NagyDonat updated https://github.com/llvm/llvm-project/pull/216077
From 1e4bdff18bacff7fa9ece3a0dbf21298a93051c9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Thu, 13 Aug 2026 16:57:44 +0200 Subject: [PATCH 1/6] [analyzer] Implement potiential underflow warnings The `security.ArrayBound` checker is 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. --- .../Checkers/ArrayBoundChecker.cpp | 20 +++++++++---------- .../test/Analysis/ArrayBound/verbose-tests.c | 17 ++++++++++++++++ 2 files changed, 26 insertions(+), 11 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp index 9e4e8e83749ad..256b17154cd44 100644 --- a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp @@ -334,13 +334,15 @@ 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) { 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 +482,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,8 +491,7 @@ void ArrayBoundChecker::handleAccessExpr(const Expr *E, if (isTainted(State, ASE->getIdx(), C.getStackFrame())) OffsetName = "index"; - BugDescription Desc = - describeTaintBug(RegName, OffsetName, Res.mayUnderflow()); + BugDescription Desc = describeTaintBug(Res, RegName, OffsetName); reportOOB(C, State, Desc, ByteOffset, Extent, /*IsTaintBug=*/true); return; } diff --git a/clang/test/Analysis/ArrayBound/verbose-tests.c b/clang/test/Analysis/ArrayBound/verbose-tests.c index c0b1f2a8ae6be..81b5001c60f89 100644 --- a/clang/test/Analysis/ArrayBound/verbose-tests.c +++ b/clang/test/Analysis/ArrayBound/verbose-tests.c @@ -100,6 +100,23 @@ 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); From 90f88089250caa8dfdfb74915817d67062314720 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Mon, 24 Aug 2026 10:45:12 +0200 Subject: [PATCH 2/6] Remove a newline MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Balázs Benics <[email protected]> --- clang/test/Analysis/ArrayBound/verbose-tests.c | 1 - 1 file changed, 1 deletion(-) diff --git a/clang/test/Analysis/ArrayBound/verbose-tests.c b/clang/test/Analysis/ArrayBound/verbose-tests.c index 81b5001c60f89..bbc32e52a9853 100644 --- a/clang/test/Analysis/ArrayBound/verbose-tests.c +++ b/clang/test/Analysis/ArrayBound/verbose-tests.c @@ -116,7 +116,6 @@ void taintedIndexNonlarge(void) { // expected-note@-2 {{Access of 'TenElements' with a tainted index that may be negative}} } - void taintedIndexUnsigned(void) { unsigned index; scanf("%u", &index); From 65221845c27809955b30be3d1f1a78febf992362 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Mon, 24 Aug 2026 10:58:31 +0200 Subject: [PATCH 3/6] Remove dot from end of message This was added accidentally in this PR. The dot is customarily omitted at the end of our warning messages and and path notes. --- clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp index 256b17154cd44..c22f1f4edfaaa 100644 --- a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp @@ -339,7 +339,7 @@ static BugDescription describeTaintBug(bounds::CheckResult Res, StringRef OffsetName) { 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}{3}{4}.", + 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" : "")}; From 997da098f1345132a47476790f841e752cd3f67b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Mon, 24 Aug 2026 11:02:20 +0200 Subject: [PATCH 4/6] Assert preconditions of methods that calculate warning messages --- clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp index c22f1f4edfaaa..ab3c34a070951 100644 --- a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp @@ -281,6 +281,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()); @@ -337,6 +339,7 @@ static BugDescription describeInvalidAccess(bounds::CheckResult Res, 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}{3}{4}", From 1c13e375c0ef0de30a50dd4ce31e5516761dadf0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Mon, 24 Aug 2026 15:00:43 +0200 Subject: [PATCH 5/6] Add testcase taintedIndexCast --- clang/test/Analysis/ArrayBound/verbose-tests.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/clang/test/Analysis/ArrayBound/verbose-tests.c b/clang/test/Analysis/ArrayBound/verbose-tests.c index bbc32e52a9853..6e23aaced978c 100644 --- a/clang/test/Analysis/ArrayBound/verbose-tests.c +++ b/clang/test/Analysis/ArrayBound/verbose-tests.c @@ -160,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'}} From 1d865c792e9b7a4663c7cc1c861aa1d50a1db0aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Mon, 24 Aug 2026 15:58:10 +0200 Subject: [PATCH 6/6] Don't mark extent as interesting for potential underflow --- .../Checkers/ArrayBoundChecker.cpp | 3 +- .../test/Analysis/ArrayBound/verbose-tests.c | 28 +++++++++++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp index ab3c34a070951..61e6c4df8b3af 100644 --- a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp @@ -495,7 +495,8 @@ void ArrayBoundChecker::handleAccessExpr(const Expr *E, OffsetName = "index"; BugDescription Desc = describeTaintBug(Res, RegName, OffsetName); - reportOOB(C, State, Desc, ByteOffset, Extent, /*IsTaintBug=*/true); + 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 6e23aaced978c..f4619fcc14006 100644 --- a/clang/test/Analysis/ArrayBound/verbose-tests.c +++ b/clang/test/Analysis/ArrayBound/verbose-tests.c @@ -391,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
