llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-static-analyzer-1 Author: Rashmi Mudduluru (t-rasmud) <details> <summary>Changes</summary> 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. --- Full diff: https://github.com/llvm/llvm-project/pull/161723.diff 2 Files Affected: - (modified) clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp (+10-42) - (added) clang/test/Analysis/ArrayBound/cplusplus-tainted-index.cpp (+52) ``````````diff diff --git a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp index 6ad5acd4e76f2..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) @@ -686,37 +675,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 +726,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; 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..0912f5057f7ed --- /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-config unroll-loops=true -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'}} + } +} `````````` </details> https://github.com/llvm/llvm-project/pull/161723 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
