https://github.com/gamesh411 updated https://github.com/llvm/llvm-project/pull/186802
From 73957c5cc93dff86cfed689696bea91367e9fec8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krist=C3=B3f=20Umann?= <[email protected]> Date: Tue, 22 Oct 2024 11:18:37 +0200 Subject: [PATCH 1/6] [analyzer] Untangle subcheckers of CStringChecker MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It turns out, that some checks for cstring functions happened as a side effect of other checks. For example, whether the arguments to memcpy were uninitialized happened during buffer overflow checking. The way this was implemented is that if alpha.unix.cstring.OutOfBounds was disabled, alpha.unix.cstring.UninitializedRead couldn't emit any warnings. It turns out that major modeling steps are early-exited if a certain checker is disabled! This patch moved the early returns to the report emission parts -- modeling still happens, only the bug report construction is omitted. This would mean that if we find a fatal error (like buffer overflow) we _should_ stop analysis even if we don't emit a warning (thats a part of doing modeling), but I decided against implementing that. One hurdle is that CStringChecker is a dependency of MallocChecker, and the current tests rely on the CStringChecker _not_ terminating execution paths prematurely. Considering that the checkers that would do that are in alpha anyways, this doesn't seem to be an urgent step immediately. I added FIXMEs to all tests would have failed if the patch sank the analysis at the fatal cstring function call, but didn't. I also added a new test case for buffers overlapping, but not being quite equal. Original Author: Kristóf Umann <[email protected]> Co-Author: Endre Fülöp <[email protected]> --- .../Checkers/CStringChecker.cpp | 51 +++++++++++------ clang/test/Analysis/bstring.cpp | 55 +++++++++++++++++-- clang/test/Analysis/malloc.c | 11 ++++ 3 files changed, 94 insertions(+), 23 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp index 144411495f5a1..4fccdf8a0678d 100644 --- a/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp @@ -576,8 +576,11 @@ ProgramStateRef CStringChecker::CheckLocation(CheckerContext &C, auto [StInBound, StOutBound] = state->assumeInBoundDual(*Idx, Size); if (StOutBound && !StInBound) { + // FIXME: We detected a fatal error here, we should stop analysis even if we + // chose not to emit a report here. However, as long as our out-of-bounds + // checker is in alpha, lets just pretend nothing happened. if (!OutOfBounds.isEnabled()) - return nullptr; + return state; ErrorMessage Message = createOutOfBoundErrorMsg(CurrentFunctionDescription, Access); @@ -610,10 +613,6 @@ CStringChecker::CheckBufferAccess(CheckerContext &C, ProgramStateRef State, if (!State) return nullptr; - // If out-of-bounds checking is turned off, skip the rest. - if (!OutOfBounds.isEnabled()) - return State; - SVal BufStart = svalBuilder.evalCast(BufVal, PtrTy, Buffer.Expression->getType()); @@ -661,9 +660,6 @@ ProgramStateRef CStringChecker::CheckOverlap(CheckerContext &C, SizeArgExpr Size, AnyArgExpr First, AnyArgExpr Second, CharKind CK) const { - if (!BufferOverlap.isEnabled()) - return state; - // Do a simple check for overlap: if the two arguments are from the same // buffer, see if the end of the first is greater than the start of the second // or vice versa. @@ -702,9 +698,15 @@ ProgramStateRef CStringChecker::CheckOverlap(CheckerContext &C, state->assume(svalBuilder.evalEQ(state, *firstLoc, *secondLoc)); if (stateTrue && !stateFalse) { - // If the values are known to be equal, that's automatically an overlap. - emitOverlapBug(C, stateTrue, First.Expression, Second.Expression); - return nullptr; + if (BufferOverlap.isEnabled()) { + // If the values are known to be equal, that's automatically an overlap. + emitOverlapBug(C, stateTrue, First.Expression, Second.Expression); + return nullptr; + } + // FIXME: We detected a fatal error here, we should stop analysis even if we + // chose not to emit a report here. However, as long as our overlap checker + // is in alpha, lets just pretend nothing happened. + return state; } // assume the two expressions are not equal. @@ -769,8 +771,14 @@ ProgramStateRef CStringChecker::CheckOverlap(CheckerContext &C, if (stateTrue && !stateFalse) { // Overlap! - emitOverlapBug(C, stateTrue, First.Expression, Second.Expression); - return nullptr; + if (BufferOverlap.isEnabled()) { + emitOverlapBug(C, stateTrue, First.Expression, Second.Expression); + return nullptr; + } + // FIXME: We detected a fatal error here, we should stop analysis even if we + // chose not to emit a report here. However, as long as our overlap checker + // is in alpha, lets just pretend nothing happened. + return state; } // assume the two expressions don't overlap. @@ -779,7 +787,10 @@ ProgramStateRef CStringChecker::CheckOverlap(CheckerContext &C, } void CStringChecker::emitOverlapBug(CheckerContext &C, ProgramStateRef state, - const Stmt *First, const Stmt *Second) const { + const Stmt *First, + const Stmt *Second) const { + assert(BufferOverlap.isEnabled() && + "Can't emit from a checker that is not enabled!"); ExplodedNode *N = C.generateErrorNode(state); if (!N) return; @@ -795,6 +806,8 @@ void CStringChecker::emitOverlapBug(CheckerContext &C, ProgramStateRef state, void CStringChecker::emitNullArgBug(CheckerContext &C, ProgramStateRef State, const Stmt *S, StringRef WarningMsg) const { + assert(NullArg.isEnabled() && + "Can't emit from a checker that is not enabled!"); if (ExplodedNode *N = C.generateErrorNode(State)) { auto Report = std::make_unique<PathSensitiveBugReport>(NullArg, WarningMsg, N); @@ -809,6 +822,8 @@ void CStringChecker::emitUninitializedReadBug(CheckerContext &C, ProgramStateRef State, const Expr *E, const MemRegion *R, StringRef Msg) const { + assert(UninitializedRead.isEnabled() && + "Can't emit from a checker that is not enabled!"); if (ExplodedNode *N = C.generateErrorNode(State)) { auto Report = std::make_unique<PathSensitiveBugReport>(UninitializedRead, Msg, N); @@ -824,6 +839,8 @@ void CStringChecker::emitUninitializedReadBug(CheckerContext &C, void CStringChecker::emitOutOfBoundsBug(CheckerContext &C, ProgramStateRef State, const Stmt *S, StringRef WarningMsg) const { + assert(OutOfBounds.isEnabled() && + "Can't emit from a checker that is not enabled!"); if (ExplodedNode *N = C.generateErrorNode(State)) { // FIXME: It would be nice to eventually make this diagnostic more clear, // e.g., by referencing the original declaration or by saying *why* this @@ -838,6 +855,8 @@ void CStringChecker::emitOutOfBoundsBug(CheckerContext &C, void CStringChecker::emitNotCStringBug(CheckerContext &C, ProgramStateRef State, const Stmt *S, StringRef WarningMsg) const { + assert(NotNullTerm.isEnabled() && + "Can't emit from a checker that is not enabled!"); if (ExplodedNode *N = C.generateNonFatalErrorNode(State)) { auto Report = std::make_unique<PathSensitiveBugReport>(NotNullTerm, WarningMsg, N); @@ -851,10 +870,6 @@ ProgramStateRef CStringChecker::checkAdditionOverflow(CheckerContext &C, ProgramStateRef state, NonLoc left, NonLoc right) const { - // If out-of-bounds checking is turned off, skip the rest. - if (!OutOfBounds.isEnabled()) - return state; - // If a previous check has failed, propagate the failure. if (!state) return nullptr; diff --git a/clang/test/Analysis/bstring.cpp b/clang/test/Analysis/bstring.cpp index 9c30bef15d407..385e7043053e2 100644 --- a/clang/test/Analysis/bstring.cpp +++ b/clang/test/Analysis/bstring.cpp @@ -1,8 +1,43 @@ -// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix.cstring,unix.Malloc,alpha.unix.cstring,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s -// RUN: %clang_analyze_cc1 -DUSE_BUILTINS -analyzer-checker=core,unix.cstring,unix.Malloc,alpha.unix.cstring,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s -// RUN: %clang_analyze_cc1 -DVARIANT -analyzer-checker=core,unix.cstring,alpha.unix.cstring,unix.Malloc,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s -// RUN: %clang_analyze_cc1 -DUSE_BUILTINS -DVARIANT -analyzer-checker=core,unix.cstring,alpha.unix.cstring,unix.Malloc,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s -// RUN: %clang_analyze_cc1 -DSUPPRESS_OUT_OF_BOUND -analyzer-checker=core,unix.cstring,unix.Malloc,alpha.unix.cstring.BufferOverlap,unix.cstring.NotNullTerminated,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s +// RUN: %clang_analyze_cc1 -verify %s \ +// RUN: -analyzer-checker=core \ +// RUN: -analyzer-checker=unix.cstring \ +// RUN: -analyzer-checker=unix.Malloc \ +// RUN: -analyzer-checker=alpha.unix.cstring \ +// RUN: -analyzer-checker=debug.ExprInspection \ +// RUN: -analyzer-config eagerly-assume=false + +// RUN: %clang_analyze_cc1 -verify %s -DUSE_BUILTINS \ +// RUN: -analyzer-checker=core \ +// RUN: -analyzer-checker=unix.cstring \ +// RUN: -analyzer-checker=unix.Malloc \ +// RUN: -analyzer-checker=alpha.unix.cstring \ +// RUN: -analyzer-checker=debug.ExprInspection \ +// RUN: -analyzer-config eagerly-assume=false + +// RUN: %clang_analyze_cc1 -verify %s -DVARIANT \ +// RUN: -analyzer-checker=core \ +// RUN: -analyzer-checker=unix.cstring \ +// RUN: -analyzer-checker=alpha.unix.cstring \ +// RUN: -analyzer-checker=unix.Malloc \ +// RUN: -analyzer-checker=debug.ExprInspection \ +// RUN: -analyzer-config eagerly-assume=false + +// RUN: %clang_analyze_cc1 -verify %s -DUSE_BUILTINS -DVARIANT \ +// RUN: -analyzer-checker=core \ +// RUN: -analyzer-checker=unix.cstring \ +// RUN: -analyzer-checker=alpha.unix.cstring \ +// RUN: -analyzer-checker=unix.Malloc \ +// RUN: -analyzer-checker=debug.ExprInspection \ +// RUN: -analyzer-config eagerly-assume=false + +// RUN: %clang_analyze_cc1 -verify %s -DSUPPRESS_OUT_OF_BOUND \ +// RUN: -analyzer-checker=core \ +// RUN: -analyzer-checker=unix.cstring \ +// RUN: -analyzer-checker=unix.Malloc \ +// RUN: -analyzer-checker=alpha.unix.cstring.BufferOverlap \ +// RUN: -analyzer-checker=unix.cstring.NotNullTerminated \ +// RUN: -analyzer-checker=debug.ExprInspection \ +// RUN: -analyzer-config eagerly-assume=false #include "Inputs/system-header-simulator-cxx.h" #include "Inputs/system-header-simulator-for-malloc.h" @@ -103,6 +138,8 @@ void memset1_inheritance() { #ifdef SUPPRESS_OUT_OF_BOUND void memset2_inheritance_field() { Derived d; + // FIXME: The analyzer should stop analysis after memset. The argument to + // sizeof should be Derived::d_mem. memset(&d.d_mem, 0, sizeof(Derived)); clang_analyzer_eval(d.b_mem == 0); // expected-warning{{UNKNOWN}} clang_analyzer_eval(d.d_mem == 0); // expected-warning{{UNKNOWN}} @@ -110,6 +147,8 @@ void memset2_inheritance_field() { void memset3_inheritance_field() { Derived d; + // FIXME: The analyzer should stop analysis after memset. The argument to + // sizeof should be Derived::b_mem. memset(&d.b_mem, 0, sizeof(Derived)); clang_analyzer_eval(d.b_mem == 0); // expected-warning{{TRUE}} clang_analyzer_eval(d.d_mem == 0); // expected-warning{{TRUE}} @@ -176,6 +215,8 @@ class DerivedVirtual : public BaseVirtual { #ifdef SUPPRESS_OUT_OF_BOUND void memset8_virtual_inheritance_field() { DerivedVirtual d; + // FIXME: The analyzer should stop analysis after memset. The argument to + // sizeof should be Derived::b_mem. memset(&d.b_mem, 0, sizeof(Derived)); clang_analyzer_eval(d.b_mem == 0); // expected-warning{{UNKNOWN}} clang_analyzer_eval(d.d_mem == 0); // expected-warning{{UNKNOWN}} @@ -188,6 +229,10 @@ void memset1_new_array() { int *array = new int[10]; memset(array, 0, 10 * sizeof(int)); clang_analyzer_eval(array[2] == 0); // expected-warning{{TRUE}} + // FIXME: The analyzer should stop analysis after memset. Maybe the intent of + // this test was to test for this as a desired behaviour, but it shouldn't be, + // going out-of-bounds with memset is a fatal error, even if we decide not to + // report it. memset(array + 1, 'a', 10 * sizeof(9)); clang_analyzer_eval(array[2] == 0); // expected-warning{{UNKNOWN}} delete[] array; diff --git a/clang/test/Analysis/malloc.c b/clang/test/Analysis/malloc.c index 92b47bc3b5e9a..1492659d8a128 100644 --- a/clang/test/Analysis/malloc.c +++ b/clang/test/Analysis/malloc.c @@ -870,12 +870,23 @@ void doNotInvalidateWhenPassedToSystemCalls(char *s) { strlen(p); strcpy(p, s); strcpy(s, p); + // FIXME: We should stop analysis here, even if we emit no warnings, since + // overlapping buffers for strycpy is a fatal error. strcpy(p, p); memcpy(p, s, 1); memcpy(s, p, 1); memcpy(p, p, 1); } // expected-warning {{leak}} +void doNotInvalidateWhenPassedToSystemCalls2(char *s) { + char *p = malloc(12); + // FIXME: We should stop analysis here, even if we emit no warnings, since + // overlapping buffers for strycpy is a fatal error. + int a[4] = {0}; + memcpy(a+2, a+1, 8); + (void)p; +} // expected-warning {{leak}} + // Treat source buffer contents as escaped. void escapeSourceContents(char *s) { char *p = malloc(12); From 1e57f2ab461f50b76f8adcac1b08379e85f9a726 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Endre=20F=C3=BCl=C3=B6p?= <[email protected]> Date: Mon, 16 Mar 2026 14:59:42 +0100 Subject: [PATCH 2/6] fix formatting --- clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp index 4fccdf8a0678d..932075d339ebc 100644 --- a/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp @@ -867,9 +867,9 @@ void CStringChecker::emitNotCStringBug(CheckerContext &C, ProgramStateRef State, } ProgramStateRef CStringChecker::checkAdditionOverflow(CheckerContext &C, - ProgramStateRef state, - NonLoc left, - NonLoc right) const { + ProgramStateRef state, + NonLoc left, + NonLoc right) const { // If a previous check has failed, propagate the failure. if (!state) return nullptr; From ba3c8cbe5979f0e6549671f6d7c61d6f11a790d4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Endre=20F=C3=BCl=C3=B6p?= <[email protected]> Date: Mon, 16 Mar 2026 15:37:31 +0100 Subject: [PATCH 3/6] fix grammar and redundant comment --- .../Checkers/CStringChecker.cpp | 21 +++++++++---------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp index 932075d339ebc..6d696a22460d1 100644 --- a/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp @@ -475,7 +475,7 @@ ProgramStateRef CStringChecker::checkInit(CheckerContext &C, return nullptr; } - // We won't check whether the entire region is fully initialized -- lets just + // We won't check whether the entire region is fully initialized -- let's just // check that the first and the last element is. So, onto checking the last // element: const QualType IdxTy = SVB.getArrayIndexType(); @@ -576,9 +576,9 @@ ProgramStateRef CStringChecker::CheckLocation(CheckerContext &C, auto [StInBound, StOutBound] = state->assumeInBoundDual(*Idx, Size); if (StOutBound && !StInBound) { - // FIXME: We detected a fatal error here, we should stop analysis even if we - // chose not to emit a report here. However, as long as our out-of-bounds - // checker is in alpha, lets just pretend nothing happened. + // FIXME: We detected a fatal error here, we should stop the analysis even + // if we choose not to emit a report here. However, as long as our + // out-of-bounds checker is in alpha, let's just pretend nothing happened. if (!OutOfBounds.isEnabled()) return state; @@ -703,9 +703,9 @@ ProgramStateRef CStringChecker::CheckOverlap(CheckerContext &C, emitOverlapBug(C, stateTrue, First.Expression, Second.Expression); return nullptr; } - // FIXME: We detected a fatal error here, we should stop analysis even if we - // chose not to emit a report here. However, as long as our overlap checker - // is in alpha, lets just pretend nothing happened. + // FIXME: We detected a fatal error here, we should stop the analysis even + // if we choose not to emit a report here. However, as long as our overlap + // checker is in alpha, let's just pretend nothing happened. return state; } @@ -770,14 +770,13 @@ ProgramStateRef CStringChecker::CheckOverlap(CheckerContext &C, std::tie(stateTrue, stateFalse) = state->assume(*OverlapTest); if (stateTrue && !stateFalse) { - // Overlap! if (BufferOverlap.isEnabled()) { emitOverlapBug(C, stateTrue, First.Expression, Second.Expression); return nullptr; } - // FIXME: We detected a fatal error here, we should stop analysis even if we - // chose not to emit a report here. However, as long as our overlap checker - // is in alpha, lets just pretend nothing happened. + // FIXME: We detected a fatal error here, we should stop the analysis even + // if we choose not to emit a report here. However, as long as our overlap + // checker is in alpha, let's just pretend nothing happened. return state; } From fbd2f41ababaa3d9b46f0479a6f9b3a3a8cdbd7e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Endre=20F=C3=BCl=C3=B6p?= <[email protected]> Date: Tue, 17 Mar 2026 14:12:07 +0100 Subject: [PATCH 4/6] organize run-lines with defines --- clang/test/Analysis/bstring.cpp | 63 ++++++++++++--------------------- 1 file changed, 23 insertions(+), 40 deletions(-) diff --git a/clang/test/Analysis/bstring.cpp b/clang/test/Analysis/bstring.cpp index 385e7043053e2..66226e68efe53 100644 --- a/clang/test/Analysis/bstring.cpp +++ b/clang/test/Analysis/bstring.cpp @@ -1,43 +1,26 @@ -// RUN: %clang_analyze_cc1 -verify %s \ -// RUN: -analyzer-checker=core \ -// RUN: -analyzer-checker=unix.cstring \ -// RUN: -analyzer-checker=unix.Malloc \ -// RUN: -analyzer-checker=alpha.unix.cstring \ -// RUN: -analyzer-checker=debug.ExprInspection \ -// RUN: -analyzer-config eagerly-assume=false - -// RUN: %clang_analyze_cc1 -verify %s -DUSE_BUILTINS \ -// RUN: -analyzer-checker=core \ -// RUN: -analyzer-checker=unix.cstring \ -// RUN: -analyzer-checker=unix.Malloc \ -// RUN: -analyzer-checker=alpha.unix.cstring \ -// RUN: -analyzer-checker=debug.ExprInspection \ -// RUN: -analyzer-config eagerly-assume=false - -// RUN: %clang_analyze_cc1 -verify %s -DVARIANT \ -// RUN: -analyzer-checker=core \ -// RUN: -analyzer-checker=unix.cstring \ -// RUN: -analyzer-checker=alpha.unix.cstring \ -// RUN: -analyzer-checker=unix.Malloc \ -// RUN: -analyzer-checker=debug.ExprInspection \ -// RUN: -analyzer-config eagerly-assume=false - -// RUN: %clang_analyze_cc1 -verify %s -DUSE_BUILTINS -DVARIANT \ -// RUN: -analyzer-checker=core \ -// RUN: -analyzer-checker=unix.cstring \ -// RUN: -analyzer-checker=alpha.unix.cstring \ -// RUN: -analyzer-checker=unix.Malloc \ -// RUN: -analyzer-checker=debug.ExprInspection \ -// RUN: -analyzer-config eagerly-assume=false - -// RUN: %clang_analyze_cc1 -verify %s -DSUPPRESS_OUT_OF_BOUND \ -// RUN: -analyzer-checker=core \ -// RUN: -analyzer-checker=unix.cstring \ -// RUN: -analyzer-checker=unix.Malloc \ -// RUN: -analyzer-checker=alpha.unix.cstring.BufferOverlap \ -// RUN: -analyzer-checker=unix.cstring.NotNullTerminated \ -// RUN: -analyzer-checker=debug.ExprInspection \ -// RUN: -analyzer-config eagerly-assume=false +// DEFINE: %{analyzer} = %clang_analyze_cc1 \ +// DEFINE: -analyzer-checker=core \ +// DEFINE: -analyzer-checker=unix.cstring \ +// DEFINE: -analyzer-checker=unix.Malloc \ +// DEFINE: -analyzer-checker=debug.ExprInspection \ +// DEFINE: -analyzer-config eagerly-assume=false \ +// DEFINE: -verify %s + +// RUN: %{analyzer} \ +// RUN: -analyzer-checker=alpha.unix.cstring + +// RUN: %{analyzer} -DUSE_BUILTINS \ +// RUN: -analyzer-checker=alpha.unix.cstring + +// RUN: %{analyzer} -DVARIANT \ +// RUN: -analyzer-checker=alpha.unix.cstring + +// RUN: %{analyzer} -DUSE_BUILTINS -DVARIANT \ +// RUN: -analyzer-checker=alpha.unix.cstring + +// RUN: %{analyzer} -DSUPPRESS_OUT_OF_BOUND \ +// RUN: -analyzer-checker=alpha.unix.cstring.BufferOverlap \ +// RUN: -analyzer-checker=unix.cstring.NotNullTerminated #include "Inputs/system-header-simulator-cxx.h" #include "Inputs/system-header-simulator-for-malloc.h" From 1a6c25c43de34e4755689b76dc095bd15821af44 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Endre=20F=C3=BCl=C3=B6p?= <[email protected]> Date: Wed, 18 Mar 2026 10:24:46 +0100 Subject: [PATCH 5/6] update ambiguous comments This is my interpretation of the situation, not @dkszelethus's. --- .../Checkers/CStringChecker.cpp | 14 +++++++---- clang/test/Analysis/bstring.cpp | 23 ++++++++++++++----- 2 files changed, 27 insertions(+), 10 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp index 6d696a22460d1..b6d910d7cdfd3 100644 --- a/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp @@ -577,8 +577,11 @@ ProgramStateRef CStringChecker::CheckLocation(CheckerContext &C, auto [StInBound, StOutBound] = state->assumeInBoundDual(*Idx, Size); if (StOutBound && !StInBound) { // FIXME: We detected a fatal error here, we should stop the analysis even - // if we choose not to emit a report here. However, as long as our - // out-of-bounds checker is in alpha, let's just pretend nothing happened. + // if we choose not to emit a report here. Instead, we choose to continue + // the analysis with a slightly broken state, so that other checkers can + // still emit possibly relevant reports. One such checker would be the + // alpha.unix.cstring.OutOfBounds. Sinking the state here could lead to + // loss reports from those checkers. if (!OutOfBounds.isEnabled()) return state; @@ -704,8 +707,11 @@ ProgramStateRef CStringChecker::CheckOverlap(CheckerContext &C, return nullptr; } // FIXME: We detected a fatal error here, we should stop the analysis even - // if we choose not to emit a report here. However, as long as our overlap - // checker is in alpha, let's just pretend nothing happened. + // if we choose not to emit a report here. Instead, we choose to continue + // the analysis with a slightly broken state, so that other checkers can + // still emit possibly relevant reports. One such checker would be the + // alpha.unix.cstring.OutOfBounds. Sinking the state here could lead to + // loss reports from those checkers. return state; } diff --git a/clang/test/Analysis/bstring.cpp b/clang/test/Analysis/bstring.cpp index 66226e68efe53..6c938fcb8d65e 100644 --- a/clang/test/Analysis/bstring.cpp +++ b/clang/test/Analysis/bstring.cpp @@ -121,8 +121,11 @@ void memset1_inheritance() { #ifdef SUPPRESS_OUT_OF_BOUND void memset2_inheritance_field() { Derived d; - // FIXME: The analyzer should stop analysis after memset. The argument to - // sizeof should be Derived::d_mem. + // FIXME: This example wrongly calls `memset` on the derived field, with the + // size parameter that has the size of the whole derived class. The analysis + // should stop at that point as this is UB. + // This test asserts the current behavior of treating the not set part as + // UNKNOWN. memset(&d.d_mem, 0, sizeof(Derived)); clang_analyzer_eval(d.b_mem == 0); // expected-warning{{UNKNOWN}} clang_analyzer_eval(d.d_mem == 0); // expected-warning{{UNKNOWN}} @@ -130,8 +133,12 @@ void memset2_inheritance_field() { void memset3_inheritance_field() { Derived d; - // FIXME: The analyzer should stop analysis after memset. The argument to - // sizeof should be Derived::b_mem. + // FIXME: Here we are setting the field of the base with the size of the + // Derived class. By the letter of the standard this is UB, but practically + // this only touches memory it is supposed to with the above class + // definitions. If we were to be strict the analysis should stop here. + // This test asserts the current behavior of nevertheless treating the + // wrongly set field as correctly set to 0. memset(&d.b_mem, 0, sizeof(Derived)); clang_analyzer_eval(d.b_mem == 0); // expected-warning{{TRUE}} clang_analyzer_eval(d.d_mem == 0); // expected-warning{{TRUE}} @@ -198,8 +205,12 @@ class DerivedVirtual : public BaseVirtual { #ifdef SUPPRESS_OUT_OF_BOUND void memset8_virtual_inheritance_field() { DerivedVirtual d; - // FIXME: The analyzer should stop analysis after memset. The argument to - // sizeof should be Derived::b_mem. + // FIXME: This example wrongly calls `memset` on the derived field, with the + // size parameter that has the size of the whole derived class. The analysis + // should stop at that point as this is UB. The situation is further + // complicated by the fact the base base a virtual function. + // This test asserts the current behavior of treating the not set part as + // UNKNOWN. memset(&d.b_mem, 0, sizeof(Derived)); clang_analyzer_eval(d.b_mem == 0); // expected-warning{{UNKNOWN}} clang_analyzer_eval(d.d_mem == 0); // expected-warning{{UNKNOWN}} From f8252e5d476912b0226d3942e61c8e10468c2c0a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Endre=20F=C3=BCl=C3=B6p?= <[email protected]> Date: Mon, 16 Mar 2026 16:00:38 +0100 Subject: [PATCH 6/6] add test a for UninitializedRead independent of OutOfBounds This test case verifies alpha.unix.cstring.UninitializedRead produces warnings even when alpha.unix.cstring.OutOfBounds is disabled. Previously, CheckBufferAccess would early-return before reaching checkInit() when OutOfBounds was disabled, suppressing UninitializedRead as a side effect. Adding this test case was suggested by @isuckatcs during the previous PR #113312. --- clang/test/Analysis/bstring.cpp | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/clang/test/Analysis/bstring.cpp b/clang/test/Analysis/bstring.cpp index 6c938fcb8d65e..93cfc871296e0 100644 --- a/clang/test/Analysis/bstring.cpp +++ b/clang/test/Analysis/bstring.cpp @@ -22,6 +22,10 @@ // RUN: -analyzer-checker=alpha.unix.cstring.BufferOverlap \ // RUN: -analyzer-checker=unix.cstring.NotNullTerminated +// RUN: %{analyzer} \ +// RUN: -DUNINIT_WITHOUT_OUTOFBOUND \ +// RUN: -analyzer-checker=alpha.unix.cstring.UninitializedRead + #include "Inputs/system-header-simulator-cxx.h" #include "Inputs/system-header-simulator-for-malloc.h" @@ -232,3 +236,12 @@ void memset1_new_array() { delete[] array; } #endif + +#ifdef UNINIT_WITHOUT_OUTOFBOUND +void memmove_uninit_without_outofbound() { + int src[4]; + int dst[4]; + memmove(dst, src, sizeof(src)); // expected-warning{{The first element of the 2nd argument is undefined}} + // expected-note@-1{{Other elements might also be undefined}} +} +#endif _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
