https://github.com/t-rasmud created https://github.com/llvm/llvm-project/pull/161723
This PR removes the check for whether an array offset is tainted before reporting an OOB in the `security.ArrayBound` checker. The `isTainted` check was not working as intended and leading to false negatives in the case of conditional checks and loops. >From 3f5c9a50529520c35e7b661832b7b9717e766e95 Mon Sep 17 00:00:00 2001 From: Rashmi Mudduluru <[email protected]> Date: Tue, 30 Sep 2025 15:46:19 -0700 Subject: [PATCH 1/3] Fix false negatives when OOB occurs inside a conditional check --- .../Checkers/ArrayBoundChecker.cpp | 41 +++++-------------- 1 file changed, 10 insertions(+), 31 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp index 6ad5acd4e76f2..fd22c9fac4c17 100644 --- a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp @@ -686,37 +686,16 @@ void ArrayBoundChecker::performCheck(const Expr *E, CheckerContext &C) const { 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.getLocationContext())) - 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); + + 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; } // Actually update the state. The "if" only fails in the extremely unlikely @@ -758,7 +737,7 @@ void ArrayBoundChecker::reportOOB(CheckerContext &C, ProgramStateRef ErrorState, std::optional<NonLoc> Extent, bool IsTaintBug /*=false*/) const { - ExplodedNode *ErrorNode = C.generateErrorNode(ErrorState); + ExplodedNode *ErrorNode = C.generateNonFatalErrorNode(ErrorState); if (!ErrorNode) return; >From e80e0990e3a4e8279542c1f26d542e7e099470d4 Mon Sep 17 00:00:00 2001 From: Rashmi Mudduluru <[email protected]> Date: Wed, 1 Oct 2025 10:05:52 -0700 Subject: [PATCH 2/3] test case --- .../Checkers/ArrayBoundChecker.cpp | 11 ---- .../ArrayBound/cplusplus-tainted-index.cpp | 52 +++++++++++++++++++ 2 files changed, 52 insertions(+), 11 deletions(-) create mode 100644 clang/test/Analysis/ArrayBound/cplusplus-tainted-index.cpp diff --git a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp index fd22c9fac4c17..05b55da4c0312 100644 --- a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp @@ -478,17 +478,6 @@ static Messages getNonTaintMsgs(const ASTContext &ACtx, std::string(Buf)}; } -static Messages getTaintMsgs(const MemSpaceRegion *Space, - const SubRegion *Region, const char *OffsetName, - bool AlsoMentionUnderflow) { - std::string RegName = getRegionName(Space, 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 {2}too large", - RegName, OffsetName, - AlsoMentionUnderflow ? "negative or " : "")}; -} - const NoteTag *StateUpdateReporter::createNoteTag(CheckerContext &C) const { // Don't create a note tag if we didn't assume anything: if (!AssumedNonNegative && !AssumedUpperBound) diff --git a/clang/test/Analysis/ArrayBound/cplusplus-tainted-index.cpp b/clang/test/Analysis/ArrayBound/cplusplus-tainted-index.cpp new file mode 100644 index 0000000000000..4df398eadf611 --- /dev/null +++ b/clang/test/Analysis/ArrayBound/cplusplus-tainted-index.cpp @@ -0,0 +1,52 @@ +// RUN: %clang_analyze_cc1 -std=c++11 -Wno-array-bounds -analyzer-checker=unix,core,security.ArrayBound -verify %s + +// Test the interactions of `security.ArrayBound` with C++ features. + +void test_tainted_index_local() { + int arr[10]; + unsigned index = 10; + arr[index] = 7; + // expected-warning@-1{{Out of bound access to memory after the end of 'arr'}} +} + +void test_tainted_index_local_range() { + int arr[10]; + for (unsigned index = 0; index < 11; index++) + arr[index] = index; + // expected-warning@-1{{Out of bound access to memory after the end of 'arr'}} +} + +void test_tainted_index1(unsigned index) { + int arr[10]; + if (index < 12) + arr[index] = index; + // expected-warning@-1{{Out of bound access to memory after the end of 'arr'}} + if (index == 12) + arr[index] = index; + // expected-warning@-1{{Out of bound access to memory after the end of 'arr'}} +} + +void test_tainted_index2(unsigned index) { + int arr[10]; + if (index < 12) + arr[index] = index; + // expected-warning@-1{{Out of bound access to memory after the end of 'arr'}} +} + +unsigned strlen(const char *s); +void strcpy(char *dst, char *src); +void test_ex(int argc, char *argv[]) { + char proc_name[16]; + unsigned offset = strlen(argv[0]); + if (offset == 16) { + strcpy(proc_name, argv[0]); + proc_name[offset] = 'x'; + // expected-warning@-1{{Out of bound access to memory after the end of 'proc_name'}} + } + if (offset <= 16) { + strcpy(proc_name, argv[0]); + proc_name[offset] = argv[0][16]; + // expected-warning@-1{{Out of bound access to memory after the end of the region}} + // expected-warning@-2{{Out of bound access to memory after the end of 'proc_name'}} + } +} >From 147d27e1bb461f656a1dc682208982d70ffd3c04 Mon Sep 17 00:00:00 2001 From: Rashmi Mudduluru <[email protected]> Date: Thu, 2 Oct 2025 12:15:59 -0700 Subject: [PATCH 3/3] unroll loops option to catch OOB in test case --- clang/test/Analysis/ArrayBound/cplusplus-tainted-index.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clang/test/Analysis/ArrayBound/cplusplus-tainted-index.cpp b/clang/test/Analysis/ArrayBound/cplusplus-tainted-index.cpp index 4df398eadf611..0912f5057f7ed 100644 --- a/clang/test/Analysis/ArrayBound/cplusplus-tainted-index.cpp +++ b/clang/test/Analysis/ArrayBound/cplusplus-tainted-index.cpp @@ -1,4 +1,4 @@ -// RUN: %clang_analyze_cc1 -std=c++11 -Wno-array-bounds -analyzer-checker=unix,core,security.ArrayBound -verify %s +// RUN: %clang_analyze_cc1 -std=c++11 -Wno-array-bounds -analyzer-config unroll-loops=true -analyzer-checker=unix,core,security.ArrayBound -verify %s // Test the interactions of `security.ArrayBound` with C++ features. _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
