[clang] [analyzer] Implement potential underflow warnings (PR #216077)
Donát Nagy via cfe-commits
cfe-commits at lists.llvm.org
Mon Aug 24 06:59:19 PDT 2026
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?= <donat.nagy at ericsson.com>
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 at -2 {{Access of 'TenElements' with a tainted index that may be too large}}
}
+void taintedIndexNonlarge(void) {
+ int index;
+ scanf("%d", &index);
+ // expected-note at -1 {{Taint originated here}}
+ // expected-note at -2 {{Taint propagated to the 2nd argument}}
+
+ // expected-note at +2 {{Assuming 'index' is < 10}}
+ // expected-note at +1 {{Taking false branch}}
+ if (index >= 10)
+ return;
+
+ TenElements[index] = 5;
+ // expected-warning at -1 {{Potential out of bound access to 'TenElements' with tainted index}}
+ // expected-note at -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?= <donat.nagy at ericsson.com>
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 <benicsbalazs at gmail.com>
---
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 at -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?= <donat.nagy at ericsson.com>
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?= <donat.nagy at ericsson.com>
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?= <donat.nagy at ericsson.com>
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 at -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 at -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?= <donat.nagy at ericsson.com>
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 at +4 {{Assuming 'n' is >= 1}}
+ // expected-note at +3 {{Left side of '||' is false}}
+ // expected-note at +2 {{Assuming 'n' is <= 100}}
+ // expected-note at +1 {{Taking false branch}}
+ if (n < 1 || n > 100)
+ return;
+
+ char *p = (char *)malloc(n);
+ int index;
+ // expected-note at +2 {{Taint originated here}}
+ // expected-note at +1 {{Taint propagated to the 2nd argument}}
+ scanf("%d", &index);
+ // expected-note at +2 {{Assuming 'index' is < 'n'}}
+ // expected-note at +1 {{Taking false branch}}
+ if (index >= n) {
+ free(p);
+ return;
+ }
+ p[index] = 5;
+ // expected-warning at -1 {{Potential out of bound access to the heap area with tainted index}}
+ // expected-note at -2 {{Access of the heap area with a tainted index that may be negative}}
+}
+
void *alloca(size_t size);
int allocaRegion(void) {
More information about the cfe-commits
mailing list