https://github.com/alejandro-alvarez-sonarsource updated https://github.com/llvm/llvm-project/pull/156507
From 736d0dcf2e31f402607656bf100004f8c8dd6539 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alejandro=20=C3=81lvarez=20Ayll=C3=B3n?= <alejandro.alva...@sonarsource.com> Date: Fri, 29 Aug 2025 09:51:04 +0200 Subject: [PATCH 1/8] [clang][analyzer] Model `strxfrm` Signature: ```c size_t strxfrm(char *dest, const char *src, size_t n); ``` The modeling covers: * `src` can never be null * `dest` can be null only if n is 0, and then the return value is some unspecified positive integer * `src` and `dest` must not overlap * `dest` must have at least `n` bytes of capacity * The return value can either be: - `< n`, and the contents of the buffer pointed by `dest` is invalidated - `>= n`, and the contents of the buffer pointed by `dest` is marked as undefined CPP-6854 --- .../Checkers/CStringChecker.cpp | 100 ++++++++++++++++++ clang/test/Analysis/string.c | 57 ++++++++++ 2 files changed, 157 insertions(+) diff --git a/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp index cfc6d34a75ca2..296a803bd04c2 100644 --- a/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp @@ -101,6 +101,7 @@ class CStringChecker static void *getTag() { static int tag; return &tag; } + bool evalCall(const CallEvent &Call, CheckerContext &C) const; void checkPreStmt(const DeclStmt *DS, CheckerContext &C) const; void checkLiveSymbols(ProgramStateRef state, SymbolReaper &SR) const; @@ -163,6 +164,7 @@ class CStringChecker {{CDM::CLibrary, {"strcasecmp"}, 2}, &CStringChecker::evalStrcasecmp}, {{CDM::CLibrary, {"strncasecmp"}, 3}, &CStringChecker::evalStrncasecmp}, {{CDM::CLibrary, {"strsep"}, 2}, &CStringChecker::evalStrsep}, + {{CDM::CLibrary, {"strxfrm"}, 3}, &CStringChecker::evalStrxfrm}, {{CDM::CLibrary, {"bcopy"}, 3}, &CStringChecker::evalBcopy}, {{CDM::CLibrary, {"bcmp"}, 3}, std::bind(&CStringChecker::evalMemcmp, _1, _2, _3, CK_Regular)}, @@ -211,6 +213,8 @@ class CStringChecker bool ReturnEnd, bool IsBounded, ConcatFnKind appendK, bool returnPtr = true) const; + void evalStrxfrm(CheckerContext &C, const CallEvent &Call) const; + void evalStrcat(CheckerContext &C, const CallEvent &Call) const; void evalStrncat(CheckerContext &C, const CallEvent &Call) const; void evalStrlcat(CheckerContext &C, const CallEvent &Call) const; @@ -2243,6 +2247,102 @@ void CStringChecker::evalStrcpyCommon(CheckerContext &C, const CallEvent &Call, C.addTransition(state); } +void CStringChecker::evalStrxfrm(CheckerContext &C, + const CallEvent &Call) const { + // size_t strxfrm(char *dest, const char *src, size_t n); + CurrentFunctionDescription = "locale transformation function"; + + ProgramStateRef state = C.getState(); + const LocationContext *LCtx = C.getLocationContext(); + SValBuilder &SVB = C.getSValBuilder(); + + // Get arguments + DestinationArgExpr Dest = {{Call.getArgExpr(0), 0}}; + SourceArgExpr Source = {{Call.getArgExpr(1), 1}}; + SizeArgExpr Size = {{Call.getArgExpr(2), 2}}; + + // `src` can never be null + SVal SrcVal = state->getSVal(Source.Expression, LCtx); + state = checkNonNull(C, state, Source, SrcVal); + if (!state) + return; + + // Check overlaps + state = CheckOverlap(C, state, Size, Dest, Source, CK_Regular); + if (!state) + return; + + // The function returns an implementation-defined length needed for + // transformation + SVal retVal = SVB.conjureSymbolVal(Call, C.blockCount()); + + state = state->BindExpr(Call.getOriginExpr(), LCtx, retVal); + + // Check if size is zero + SVal sizeVal = state->getSVal(Size.Expression, LCtx); + QualType sizeTy = Size.Expression->getType(); + + auto [stateZeroSize, StateSizeNonZero] = + assumeZero(C, state, sizeVal, sizeTy); + + // If `n` is 0, we just return the implementation defined length + if (stateZeroSize && !StateSizeNonZero) { + C.addTransition(stateZeroSize); + return; + } + + if (!StateSizeNonZero) + return; + + // If `n` is not 0, `dest` can not be null. + SVal destVal = state->getSVal(Dest.Expression, LCtx); + StateSizeNonZero = checkNonNull(C, StateSizeNonZero, Dest, destVal); + if (!StateSizeNonZero) + return; + + // Check that we can write to the destination buffer + StateSizeNonZero = CheckBufferAccess(C, StateSizeNonZero, Dest, Size, + AccessKind::write, CK_Regular); + if (!StateSizeNonZero) + return; + + // Success: return value < `n` + // Failure: return value >= `n` + auto comparisonVal = SVB.evalBinOp(StateSizeNonZero, BO_LT, retVal, sizeVal, + SVB.getConditionType()) + .getAs<DefinedOrUnknownSVal>(); + + if (comparisonVal) { + auto [StateSuccess, StateFailure] = + StateSizeNonZero->assume(*comparisonVal); + + if (StateSuccess) { + // In this case, the transformation invalidated the buffer. + StateSuccess = invalidateDestinationBufferBySize( + C, StateSuccess, Dest.Expression, Call.getCFGElementRef(), destVal, + sizeVal, Size.Expression->getType()); + + C.addTransition(StateSuccess); + } + + if (StateFailure) { + // In this case, dest buffer content is undefined + if (std::optional<Loc> destLoc = destVal.getAs<Loc>()) { + StateFailure = StateFailure->bindLoc(*destLoc, UndefinedVal{}, LCtx); + } + + C.addTransition(StateFailure); + } + } else { + // Fallback: invalidate the buffer. + StateSizeNonZero = invalidateDestinationBufferBySize( + C, StateSizeNonZero, Dest.Expression, Call.getCFGElementRef(), destVal, + sizeVal, Size.Expression->getType()); + + C.addTransition(StateSizeNonZero); + } +} + void CStringChecker::evalStrcmp(CheckerContext &C, const CallEvent &Call) const { //int strcmp(const char *s1, const char *s2); diff --git a/clang/test/Analysis/string.c b/clang/test/Analysis/string.c index e017aff3b4a1d..427b99bfdf295 100644 --- a/clang/test/Analysis/string.c +++ b/clang/test/Analysis/string.c @@ -1789,3 +1789,60 @@ void CWE124_Buffer_Underwrite__malloc_char_memcpy() { free(dataBuffer); } #endif + +//===----------------------------------------------------------------------=== +// strxfrm() +// It is not a built-in. +//===----------------------------------------------------------------------=== + +size_t strxfrm(char *dest, const char *src, size_t n); + +void strxfrm_null_dest(const char *src) { + strxfrm(NULL, src, 0); // no warning + strxfrm(NULL, src, 10); // expected-warning {{Null pointer passed as 1st argument}} +} + +void strxfrm_null_source(char *dest) { + strxfrm(dest, NULL, 0); // expected-warning {{Null pointer passed as 2nd argument}} +} + +#ifndef SUPPRESS_OUT_OF_BOUND +void strxfrm_overflow(const char *src) { + char dest[10]; + strxfrm(dest, src, 55); // expected-warning {{Locale transformation function overflows the destination buffer}} +} +#endif + +void strxfrm_source_smaller() { + char dest[10]; + char source[5]; + strxfrm(dest, source, 10); +} + +void strxfrm_overlap(char *dest) { + strxfrm(dest, dest, 10); // expected-warning {{Arguments must not be overlapping buffers}} +} + +void strxfrm_regular(const char *src) { + size_t n = strxfrm(NULL, src, 0); + char *dest = (char*)malloc(n + 1); + strxfrm(dest, src, n); + free(dest); +} + +int strxfrm_dest_undef(const char *src) { + char dest[10] = {0}; + size_t n = strxfrm(dest, src, sizeof(dest)); + + int c = 0; + if (n >= sizeof(dest)) { + for (int i = 0; i < sizeof(dest); ++i) { + c += dest[i]; // expected-warning {{Assigned value is uninitialized}} + } + } else { + for (int i = 0; i < sizeof(dest); ++i) { + c += dest[i]; // no-warning + } + } + return c; +} From eea0b6b58d1a3673b7220d0f32310322282d0aa8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alejandro=20=C3=81lvarez=20Ayll=C3=B3n?= <alejandro.alva...@sonarsource.com> Date: Wed, 3 Sep 2025 08:28:56 +0200 Subject: [PATCH 2/8] Format --- clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp index 296a803bd04c2..9a4457ec764bc 100644 --- a/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp @@ -101,7 +101,6 @@ class CStringChecker static void *getTag() { static int tag; return &tag; } - bool evalCall(const CallEvent &Call, CheckerContext &C) const; void checkPreStmt(const DeclStmt *DS, CheckerContext &C) const; void checkLiveSymbols(ProgramStateRef state, SymbolReaper &SR) const; @@ -2336,8 +2335,8 @@ void CStringChecker::evalStrxfrm(CheckerContext &C, } else { // Fallback: invalidate the buffer. StateSizeNonZero = invalidateDestinationBufferBySize( - C, StateSizeNonZero, Dest.Expression, Call.getCFGElementRef(), destVal, - sizeVal, Size.Expression->getType()); + C, StateSizeNonZero, Dest.Expression, Call.getCFGElementRef(), destVal, + sizeVal, Size.Expression->getType()); C.addTransition(StateSizeNonZero); } From 55ca6703ea51839541d45ec58c634b55abdd690b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alejandro=20=C3=81lvarez=20Ayll=C3=B3n?= <alejandro.alva...@sonarsource.com> Date: Wed, 3 Sep 2025 08:33:18 +0200 Subject: [PATCH 3/8] Style --- .../Checkers/CStringChecker.cpp | 50 +++++++++---------- 1 file changed, 25 insertions(+), 25 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp index 9a4457ec764bc..6b305e408354d 100644 --- a/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp @@ -2251,7 +2251,7 @@ void CStringChecker::evalStrxfrm(CheckerContext &C, // size_t strxfrm(char *dest, const char *src, size_t n); CurrentFunctionDescription = "locale transformation function"; - ProgramStateRef state = C.getState(); + ProgramStateRef State = C.getState(); const LocationContext *LCtx = C.getLocationContext(); SValBuilder &SVB = C.getSValBuilder(); @@ -2261,32 +2261,32 @@ void CStringChecker::evalStrxfrm(CheckerContext &C, SizeArgExpr Size = {{Call.getArgExpr(2), 2}}; // `src` can never be null - SVal SrcVal = state->getSVal(Source.Expression, LCtx); - state = checkNonNull(C, state, Source, SrcVal); - if (!state) + SVal SrcVal = State->getSVal(Source.Expression, LCtx); + State = checkNonNull(C, State, Source, SrcVal); + if (!State) return; // Check overlaps - state = CheckOverlap(C, state, Size, Dest, Source, CK_Regular); - if (!state) + State = CheckOverlap(C, State, Size, Dest, Source, CK_Regular); + if (!State) return; // The function returns an implementation-defined length needed for // transformation - SVal retVal = SVB.conjureSymbolVal(Call, C.blockCount()); + SVal RetVal = SVB.conjureSymbolVal(Call, C.blockCount()); - state = state->BindExpr(Call.getOriginExpr(), LCtx, retVal); + State = State->BindExpr(Call.getOriginExpr(), LCtx, RetVal); // Check if size is zero - SVal sizeVal = state->getSVal(Size.Expression, LCtx); - QualType sizeTy = Size.Expression->getType(); + SVal SizeVal = State->getSVal(Size.Expression, LCtx); + QualType SizeTy = Size.Expression->getType(); - auto [stateZeroSize, StateSizeNonZero] = - assumeZero(C, state, sizeVal, sizeTy); + auto [StateZeroSize, StateSizeNonZero] = + assumeZero(C, State, SizeVal, SizeTy); // If `n` is 0, we just return the implementation defined length - if (stateZeroSize && !StateSizeNonZero) { - C.addTransition(stateZeroSize); + if (StateZeroSize && !StateSizeNonZero) { + C.addTransition(StateZeroSize); return; } @@ -2294,8 +2294,8 @@ void CStringChecker::evalStrxfrm(CheckerContext &C, return; // If `n` is not 0, `dest` can not be null. - SVal destVal = state->getSVal(Dest.Expression, LCtx); - StateSizeNonZero = checkNonNull(C, StateSizeNonZero, Dest, destVal); + SVal DestVal = State->getSVal(Dest.Expression, LCtx); + StateSizeNonZero = checkNonNull(C, StateSizeNonZero, Dest, DestVal); if (!StateSizeNonZero) return; @@ -2307,27 +2307,27 @@ void CStringChecker::evalStrxfrm(CheckerContext &C, // Success: return value < `n` // Failure: return value >= `n` - auto comparisonVal = SVB.evalBinOp(StateSizeNonZero, BO_LT, retVal, sizeVal, + auto ComparisonVal = SVB.evalBinOp(StateSizeNonZero, BO_LT, RetVal, SizeVal, SVB.getConditionType()) .getAs<DefinedOrUnknownSVal>(); - if (comparisonVal) { + if (ComparisonVal) { auto [StateSuccess, StateFailure] = - StateSizeNonZero->assume(*comparisonVal); + StateSizeNonZero->assume(*ComparisonVal); if (StateSuccess) { // In this case, the transformation invalidated the buffer. StateSuccess = invalidateDestinationBufferBySize( - C, StateSuccess, Dest.Expression, Call.getCFGElementRef(), destVal, - sizeVal, Size.Expression->getType()); + C, StateSuccess, Dest.Expression, Call.getCFGElementRef(), DestVal, + SizeVal, Size.Expression->getType()); C.addTransition(StateSuccess); } if (StateFailure) { // In this case, dest buffer content is undefined - if (std::optional<Loc> destLoc = destVal.getAs<Loc>()) { - StateFailure = StateFailure->bindLoc(*destLoc, UndefinedVal{}, LCtx); + if (std::optional<Loc> DestLoc = DestVal.getAs<Loc>()) { + StateFailure = StateFailure->bindLoc(*DestLoc, UndefinedVal{}, LCtx); } C.addTransition(StateFailure); @@ -2335,8 +2335,8 @@ void CStringChecker::evalStrxfrm(CheckerContext &C, } else { // Fallback: invalidate the buffer. StateSizeNonZero = invalidateDestinationBufferBySize( - C, StateSizeNonZero, Dest.Expression, Call.getCFGElementRef(), destVal, - sizeVal, Size.Expression->getType()); + C, StateSizeNonZero, Dest.Expression, Call.getCFGElementRef(), DestVal, + SizeVal, Size.Expression->getType()); C.addTransition(StateSizeNonZero); } From 5b7f66952e766e2a8cb0d61cc91847cc4a0b3000 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alejandro=20=C3=81lvarez=20Ayll=C3=B3n?= <alejandro.alva...@sonarsource.com> Date: Thu, 4 Sep 2025 13:34:09 +0200 Subject: [PATCH 4/8] Bind close to transition --- .../Checkers/CStringChecker.cpp | 21 ++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp index 6b305e408354d..45be0ea90069a 100644 --- a/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp @@ -2266,7 +2266,7 @@ void CStringChecker::evalStrxfrm(CheckerContext &C, if (!State) return; - // Check overlaps + // Buffer must not overlap State = CheckOverlap(C, State, Size, Dest, Source, CK_Regular); if (!State) return; @@ -2275,8 +2275,6 @@ void CStringChecker::evalStrxfrm(CheckerContext &C, // transformation SVal RetVal = SVB.conjureSymbolVal(Call, C.blockCount()); - State = State->BindExpr(Call.getOriginExpr(), LCtx, RetVal); - // Check if size is zero SVal SizeVal = State->getSVal(Size.Expression, LCtx); QualType SizeTy = Size.Expression->getType(); @@ -2284,17 +2282,22 @@ void CStringChecker::evalStrxfrm(CheckerContext &C, auto [StateZeroSize, StateSizeNonZero] = assumeZero(C, State, SizeVal, SizeTy); + // We can't assume anything about size, just bind the return value and be done + if (!StateZeroSize && !StateSizeNonZero) { + State = State->BindExpr(Call.getOriginExpr(), LCtx, RetVal); + C.addTransition(State); + return; + } + // If `n` is 0, we just return the implementation defined length if (StateZeroSize && !StateSizeNonZero) { + StateZeroSize = StateZeroSize->BindExpr(Call.getOriginExpr(), LCtx, RetVal); C.addTransition(StateZeroSize); return; } - if (!StateSizeNonZero) - return; - // If `n` is not 0, `dest` can not be null. - SVal DestVal = State->getSVal(Dest.Expression, LCtx); + SVal DestVal = StateSizeNonZero->getSVal(Dest.Expression, LCtx); StateSizeNonZero = checkNonNull(C, StateSizeNonZero, Dest, DestVal); if (!StateSizeNonZero) return; @@ -2321,6 +2324,7 @@ void CStringChecker::evalStrxfrm(CheckerContext &C, C, StateSuccess, Dest.Expression, Call.getCFGElementRef(), DestVal, SizeVal, Size.Expression->getType()); + StateSuccess = StateSuccess->BindExpr(Call.getOriginExpr(), LCtx, RetVal); C.addTransition(StateSuccess); } @@ -2330,6 +2334,7 @@ void CStringChecker::evalStrxfrm(CheckerContext &C, StateFailure = StateFailure->bindLoc(*DestLoc, UndefinedVal{}, LCtx); } + StateFailure = StateFailure->BindExpr(Call.getOriginExpr(), LCtx, RetVal); C.addTransition(StateFailure); } } else { @@ -2338,6 +2343,8 @@ void CStringChecker::evalStrxfrm(CheckerContext &C, C, StateSizeNonZero, Dest.Expression, Call.getCFGElementRef(), DestVal, SizeVal, Size.Expression->getType()); + StateSizeNonZero = + StateSizeNonZero->BindExpr(Call.getOriginExpr(), LCtx, RetVal); C.addTransition(StateSizeNonZero); } } From d2614454358da173a44d8d4d1ee5ce17f0897605 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alejandro=20=C3=81lvarez=20Ayll=C3=B3n?= <alejandro.alva...@sonarsource.com> Date: Thu, 4 Sep 2025 14:24:34 +0200 Subject: [PATCH 5/8] Set the full buffer as undefined --- .../Checkers/CStringChecker.cpp | 10 +-- clang/test/Analysis/string.c | 63 ++++++++++++++++--- 2 files changed, 60 insertions(+), 13 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp index 45be0ea90069a..3cbd10ead8cd3 100644 --- a/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp @@ -2313,13 +2313,12 @@ void CStringChecker::evalStrxfrm(CheckerContext &C, auto ComparisonVal = SVB.evalBinOp(StateSizeNonZero, BO_LT, RetVal, SizeVal, SVB.getConditionType()) .getAs<DefinedOrUnknownSVal>(); - if (ComparisonVal) { auto [StateSuccess, StateFailure] = StateSizeNonZero->assume(*ComparisonVal); if (StateSuccess) { - // In this case, the transformation invalidated the buffer. + // The transformation invalidated the buffer. StateSuccess = invalidateDestinationBufferBySize( C, StateSuccess, Dest.Expression, Call.getCFGElementRef(), DestVal, SizeVal, Size.Expression->getType()); @@ -2329,9 +2328,10 @@ void CStringChecker::evalStrxfrm(CheckerContext &C, } if (StateFailure) { - // In this case, dest buffer content is undefined - if (std::optional<Loc> DestLoc = DestVal.getAs<Loc>()) { - StateFailure = StateFailure->bindLoc(*DestLoc, UndefinedVal{}, LCtx); + // `dest` buffer content is undefined + if (auto DestLoc = DestVal.getAs<loc::MemRegionVal>()) { + StateFailure = StateFailure->killBinding(*DestLoc); + StateFailure = StateFailure->bindDefaultInitial(*DestLoc, UndefinedVal{}, LCtx); } StateFailure = StateFailure->BindExpr(Call.getOriginExpr(), LCtx, RetVal); diff --git a/clang/test/Analysis/string.c b/clang/test/Analysis/string.c index 427b99bfdf295..cdd36275568e3 100644 --- a/clang/test/Analysis/string.c +++ b/clang/test/Analysis/string.c @@ -1830,19 +1830,66 @@ void strxfrm_regular(const char *src) { free(dest); } -int strxfrm_dest_undef(const char *src) { - char dest[10] = {0}; +void clang_analyzer_warnIfReached(); + +int strxfrm_dest_undef(const char *src, int oracle) { + char dest[5] = {0}; + clang_analyzer_eval(dest[0] == 0); // expected-warning {{TRUE}} + size_t n = strxfrm(dest, src, sizeof(dest)); + if (oracle >= sizeof(dest) || oracle < 0) { + return 0; + } + int c = 0; if (n >= sizeof(dest)) { - for (int i = 0; i < sizeof(dest); ++i) { - c += dest[i]; // expected-warning {{Assigned value is uninitialized}} - } + // Since accessing uninitialized sinks the execution, use this trick to check all positions + switch (oracle) { + case 0: + c = dest[0]; // expected-warning {{Assigned value is uninitialized}} + break; + case 1: + c = dest[1]; // expected-warning {{Assigned value is uninitialized}} + break; + case 2: + c = dest[2]; // expected-warning {{Assigned value is uninitialized}} + break; + case 3: + c = dest[3]; // expected-warning {{Assigned value is uninitialized}} + break; + case 4: + c = dest[4]; // expected-warning {{Assigned value is uninitialized}} + break; + default: + clang_analyzer_warnIfReached(); + } + } else { + clang_analyzer_eval(n >= 0); // expected-warning {{TRUE}} + clang_analyzer_eval(dest[0] == 0); // expected-warning {{UNKNOWN}} + clang_analyzer_eval(dest[1] == 0); // expected-warning {{UNKNOWN}} + clang_analyzer_eval(dest[2] == 0); // expected-warning {{UNKNOWN}} + clang_analyzer_eval(dest[3] == 0); // expected-warning {{UNKNOWN}} + clang_analyzer_eval(dest[4] == 0); // expected-warning {{UNKNOWN}} + } + return c; +} + +int strxfrm_unknown_dest_undef(const char *src, int oracle) { + size_t n = strlen(src); + + char *dest = (char*)malloc(n * 2); + + size_t n2 = strxfrm(dest, src, n); + + int c = 0; + + if (n2 < n) { + c += dest[50]; } else { - for (int i = 0; i < sizeof(dest); ++i) { - c += dest[i]; // no-warning - } + c += dest[50]; // expected-warning {{Assigned value is uninitialized}} } + + free(dest); return c; } From aed3b69e73ebe8a1092a683d39bd19842256967b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alejandro=20=C3=81lvarez=20Ayll=C3=B3n?= <alejandro.alva...@sonarsource.com> Date: Fri, 5 Sep 2025 14:03:07 +0200 Subject: [PATCH 6/8] Ground bind and return --- .../Checkers/CStringChecker.cpp | 36 +++++++++---------- 1 file changed, 16 insertions(+), 20 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp index 3cbd10ead8cd3..f555ae9a13138 100644 --- a/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp @@ -2275,6 +2275,13 @@ void CStringChecker::evalStrxfrm(CheckerContext &C, // transformation SVal RetVal = SVB.conjureSymbolVal(Call, C.blockCount()); + auto BindReturnAndTransition = [&RetVal, &Call, LCtx, &C](ProgramStateRef State) { + if (State) { + State = State->BindExpr(Call.getOriginExpr(), LCtx, RetVal); + C.addTransition(State); + } + }; + // Check if size is zero SVal SizeVal = State->getSVal(Size.Expression, LCtx); QualType SizeTy = Size.Expression->getType(); @@ -2283,18 +2290,12 @@ void CStringChecker::evalStrxfrm(CheckerContext &C, assumeZero(C, State, SizeVal, SizeTy); // We can't assume anything about size, just bind the return value and be done - if (!StateZeroSize && !StateSizeNonZero) { - State = State->BindExpr(Call.getOriginExpr(), LCtx, RetVal); - C.addTransition(State); - return; - } + if (!StateZeroSize && !StateSizeNonZero) + return BindReturnAndTransition(State); // If `n` is 0, we just return the implementation defined length - if (StateZeroSize && !StateSizeNonZero) { - StateZeroSize = StateZeroSize->BindExpr(Call.getOriginExpr(), LCtx, RetVal); - C.addTransition(StateZeroSize); - return; - } + if (StateZeroSize && !StateSizeNonZero) + return BindReturnAndTransition(StateZeroSize); // If `n` is not 0, `dest` can not be null. SVal DestVal = StateSizeNonZero->getSVal(Dest.Expression, LCtx); @@ -2322,30 +2323,25 @@ void CStringChecker::evalStrxfrm(CheckerContext &C, StateSuccess = invalidateDestinationBufferBySize( C, StateSuccess, Dest.Expression, Call.getCFGElementRef(), DestVal, SizeVal, Size.Expression->getType()); - - StateSuccess = StateSuccess->BindExpr(Call.getOriginExpr(), LCtx, RetVal); - C.addTransition(StateSuccess); + BindReturnAndTransition(StateSuccess); } if (StateFailure) { // `dest` buffer content is undefined if (auto DestLoc = DestVal.getAs<loc::MemRegionVal>()) { StateFailure = StateFailure->killBinding(*DestLoc); - StateFailure = StateFailure->bindDefaultInitial(*DestLoc, UndefinedVal{}, LCtx); + StateFailure = + StateFailure->bindDefaultInitial(*DestLoc, UndefinedVal{}, LCtx); } - StateFailure = StateFailure->BindExpr(Call.getOriginExpr(), LCtx, RetVal); - C.addTransition(StateFailure); + BindReturnAndTransition(StateFailure); } } else { // Fallback: invalidate the buffer. StateSizeNonZero = invalidateDestinationBufferBySize( C, StateSizeNonZero, Dest.Expression, Call.getCFGElementRef(), DestVal, SizeVal, Size.Expression->getType()); - - StateSizeNonZero = - StateSizeNonZero->BindExpr(Call.getOriginExpr(), LCtx, RetVal); - C.addTransition(StateSizeNonZero); + BindReturnAndTransition(StateSizeNonZero); } } From 58bc8a593d06640852951dbe0e1b7a27b862ed8a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alejandro=20=C3=81lvarez=20Ayll=C3=B3n?= <alejandro.alva...@sonarsource.com> Date: Fri, 5 Sep 2025 14:03:28 +0200 Subject: [PATCH 7/8] Format --- clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp index f555ae9a13138..513e63d88ef7a 100644 --- a/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp @@ -2275,7 +2275,8 @@ void CStringChecker::evalStrxfrm(CheckerContext &C, // transformation SVal RetVal = SVB.conjureSymbolVal(Call, C.blockCount()); - auto BindReturnAndTransition = [&RetVal, &Call, LCtx, &C](ProgramStateRef State) { + auto BindReturnAndTransition = [&RetVal, &Call, LCtx, + &C](ProgramStateRef State) { if (State) { State = State->BindExpr(Call.getOriginExpr(), LCtx, RetVal); C.addTransition(State); From b58bef2a2259c1ce16142aa9ed8acd2f7767d21a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alejandro=20=C3=81lvarez=20Ayll=C3=B3n?= <alejandro.alva...@sonarsource.com> Date: Fri, 5 Sep 2025 14:56:14 +0200 Subject: [PATCH 8/8] Be explicit about fallthrough --- .../Checkers/CStringChecker.cpp | 48 +++++++++---------- 1 file changed, 24 insertions(+), 24 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp index 513e63d88ef7a..36f316df0c3ff 100644 --- a/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp @@ -2315,34 +2315,34 @@ void CStringChecker::evalStrxfrm(CheckerContext &C, auto ComparisonVal = SVB.evalBinOp(StateSizeNonZero, BO_LT, RetVal, SizeVal, SVB.getConditionType()) .getAs<DefinedOrUnknownSVal>(); - if (ComparisonVal) { - auto [StateSuccess, StateFailure] = - StateSizeNonZero->assume(*ComparisonVal); - - if (StateSuccess) { - // The transformation invalidated the buffer. - StateSuccess = invalidateDestinationBufferBySize( - C, StateSuccess, Dest.Expression, Call.getCFGElementRef(), DestVal, - SizeVal, Size.Expression->getType()); - BindReturnAndTransition(StateSuccess); - } - - if (StateFailure) { - // `dest` buffer content is undefined - if (auto DestLoc = DestVal.getAs<loc::MemRegionVal>()) { - StateFailure = StateFailure->killBinding(*DestLoc); - StateFailure = - StateFailure->bindDefaultInitial(*DestLoc, UndefinedVal{}, LCtx); - } - - BindReturnAndTransition(StateFailure); - } - } else { + if (!ComparisonVal) { // Fallback: invalidate the buffer. StateSizeNonZero = invalidateDestinationBufferBySize( C, StateSizeNonZero, Dest.Expression, Call.getCFGElementRef(), DestVal, SizeVal, Size.Expression->getType()); - BindReturnAndTransition(StateSizeNonZero); + return BindReturnAndTransition(StateSizeNonZero); + } + + auto [StateSuccess, StateFailure] = StateSizeNonZero->assume(*ComparisonVal); + + if (StateSuccess) { + // The transformation invalidated the buffer. + StateSuccess = invalidateDestinationBufferBySize( + C, StateSuccess, Dest.Expression, Call.getCFGElementRef(), DestVal, + SizeVal, Size.Expression->getType()); + BindReturnAndTransition(StateSuccess); + // Fallthrough: We also want to add a transition to the failure state below. + } + + if (StateFailure) { + // `dest` buffer content is undefined + if (auto DestLoc = DestVal.getAs<loc::MemRegionVal>()) { + StateFailure = StateFailure->killBinding(*DestLoc); + StateFailure = + StateFailure->bindDefaultInitial(*DestLoc, UndefinedVal{}, LCtx); + } + + BindReturnAndTransition(StateFailure); } } _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits