https://github.com/earnol updated https://github.com/llvm/llvm-project/pull/210154
>From 52b25c5f33d268580e70ecfc1383148694dddcb9 Mon Sep 17 00:00:00 2001 From: Vladislav Aranov <[email protected]> Date: Thu, 16 Jul 2026 21:46:43 +0200 Subject: [PATCH] [analyzer] Fix false positive in strchr/strrchr with constant args When both the source string and the search target are compile-time constants, determine the outcome precisely and only emit the feasible branch (found or not-found). This eliminates false positives from core.NullPointerArithm when e.g. strrchr is used in the common FILE_BASENAME macro pattern. Handles strchr, strrchr, memchr, strstr, and strpbrk. Fixes: https://github.com/llvm/llvm-project/issues/209905 --- .../Checkers/CStringChecker.cpp | 127 ++++++++++++--- clang/test/Analysis/string-search-modeling.c | 153 ++++++++++++++++++ 2 files changed, 260 insertions(+), 20 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp index 745297dd1f057..44111ddaf3148 100644 --- a/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp @@ -30,6 +30,7 @@ #include "llvm/ADT/STLExtras.h" #include "llvm/ADT/STLForwardCompat.h" #include "llvm/ADT/StringExtras.h" +#include "llvm/ADT/StringSwitch.h" #include "llvm/Support/raw_ostream.h" #include <functional> #include <optional> @@ -2662,34 +2663,120 @@ void CStringChecker::evalStrchrCommon(CheckerContext &C, const CallEvent &Call, if (!State) return; - // NULL (no-match) branch. - if (CanReturnNull) { + // If both args are constant, only emit the feasible branch (llvm#209905). + enum MatchResult { Unknown, MustMatch, MustNotMatch }; + MatchResult ConstResult = [&]() -> MatchResult { + const MemRegion *SrcRegion = SrcVal.getAsRegion(); + if (!SrcRegion) + return Unknown; + const StringLiteral *SrcLit = + getStringLiteralFromRegion(SrcRegion->StripCasts()); + if (!SrcLit) + return Unknown; + StringRef Haystack = SrcLit->getString(); + + SVal Arg1Val = State->getSVal(Call.getArgExpr(1), SF); + + // Arg1 is a character value — check if it appears in the haystack. + auto matchChar = [&]() -> MatchResult { + const llvm::APSInt *CharInt = SVB.getKnownValue(State, Arg1Val); + if (!CharInt) + return Unknown; + char Ch = static_cast<char>(CharInt->getExtValue()); + // strchr/strrchr(s, '\0') always finds the null terminator. + if (Ch == '\0') + return MustMatch; + return Haystack.contains(Ch) ? MustMatch : MustNotMatch; + }; + + // Like matchChar but restricted to the first N bytes. + auto matchCharBounded = [&]() -> MatchResult { + const llvm::APSInt *CharInt = SVB.getKnownValue(State, Arg1Val); + if (!CharInt) + return Unknown; + if (Call.getNumArgs() < 3) + return Unknown; + const llvm::APSInt *Len = + SVB.getKnownValue(State, State->getSVal(Call.getArgExpr(2), SF)); + if (!Len) + return Unknown; + char Ch = static_cast<char>(CharInt->getExtValue()); + uint64_t N = Len->getZExtValue(); + if (Ch == '\0') { + // The null terminator is at index Haystack.size(); found if N > size. + return N > Haystack.size() ? MustMatch : MustNotMatch; + } + return Haystack.substr(0, N).contains(Ch) ? MustMatch : MustNotMatch; + }; + + // Arg1 is a string — check if it appears as a substring. + auto matchSubstring = [&]() -> MatchResult { + const MemRegion *R = Arg1Val.getAsRegion(); + if (!R) + return Unknown; + const StringLiteral *Lit = getStringLiteralFromRegion(R->StripCasts()); + if (!Lit) + return Unknown; + StringRef Needle = Lit->getString(); + if (Needle.empty()) + return MustMatch; + return Haystack.contains(Needle) ? MustMatch : MustNotMatch; + }; + + // Arg1 is an accept set — check if any character appears in haystack. + auto matchAnyChar = [&]() -> MatchResult { + const MemRegion *R = Arg1Val.getAsRegion(); + if (!R) + return Unknown; + const StringLiteral *Lit = getStringLiteralFromRegion(R->StripCasts()); + if (!Lit) + return Unknown; + StringRef Accept = Lit->getString(); + return Haystack.find_first_of(Accept) != StringRef::npos ? MustMatch + : MustNotMatch; + }; + + return llvm::StringSwitch<std::function<MatchResult()>>(FnName) + .Case("strchr()", matchChar) + .Case("strrchr()", matchChar) + .Case("memchr()", matchCharBounded) + .Case("strstr()", matchSubstring) + .Case("strpbrk()", matchAnyChar) + .Default([&] { return Unknown; })(); + }(); + + // NULL (no-match) branch — skip when the match is guaranteed. + if (CanReturnNull && ConstResult != MustMatch) { ProgramStateRef NullState = State->BindExpr(CE, SF, SVB.makeNullWithType(CE->getType())); C.addTransition(NullState); } - // Found branch: a pointer within the source; needs a Loc for the arithmetic. - std::optional<Loc> SrcLoc = SrcVal.getAs<Loc>(); - if (!SrcLoc) { - SVal Result = SVB.conjureSymbolVal(Call, C.blockCount()); + // Found branch — skip when the match is impossible. + if (ConstResult != MustNotMatch) { + std::optional<Loc> SrcLoc = SrcVal.getAs<Loc>(); + if (!SrcLoc) { + SVal Result = SVB.conjureSymbolVal(Call, C.blockCount()); + State = State->BindExpr(CE, SF, Result); + C.addTransition(State); + return; + } + + // The result is: Src + SymOffset + auto RemainingExtentBytes = getDynamicExtentWithOffset(State, *SrcLoc) + .castAs<DefinedOrUnknownSVal>(); + NonLoc SymOffset = + SVB.conjureSymbolVal(Call, Ctx.getSizeType(), C.blockCount()) + .castAs<NonLoc>(); + State = State->assumeInBound(SymOffset, RemainingExtentBytes, true); + if (!State) + return; + + SVal Result = SVB.evalBinOpLN(State, BO_Add, *SrcLoc, SymOffset, + Src.Expression->getType()); State = State->BindExpr(CE, SF, Result); C.addTransition(State); - return; } - - // The result is: Src + SymOffset - auto RemainingExtentBytes = - getDynamicExtentWithOffset(State, *SrcLoc).castAs<DefinedOrUnknownSVal>(); - NonLoc SymOffset = - SVB.conjureSymbolVal(Call, Ctx.getSizeType(), C.blockCount()) - .castAs<NonLoc>(); - State = State->assumeInBound(SymOffset, RemainingExtentBytes, true); - - SVal Result = SVB.evalBinOpLN(State, BO_Add, *SrcLoc, SymOffset, - Src.Expression->getType()); - State = State->BindExpr(CE, SF, Result); - C.addTransition(State); } // These should probably be moved into a C++ standard library checker. diff --git a/clang/test/Analysis/string-search-modeling.c b/clang/test/Analysis/string-search-modeling.c index a50ec439731a3..b31556b3ee7b2 100644 --- a/clang/test/Analysis/string-search-modeling.c +++ b/clang/test/Analysis/string-search-modeling.c @@ -176,3 +176,156 @@ void no_invalidation_of_globals(const char *p) { clang_analyzer_eval(local_unmodified == 10); // expected-warning {{TRUE}} clang_analyzer_eval(global_unmodified == 20); // expected-warning {{TRUE}} } + +//===----------------------------------------------------------------------===// +// When both arguments are compile-time constants, only the correct branch is +// taken: found when the target exists, null when it does not. +// See: https://github.com/llvm/llvm-project/issues/209905 +//===----------------------------------------------------------------------===// + +// --- strchr / strrchr: target character IS in the literal --- +const char *test_strrchr_const_no_fp(void) { + // This is the original reproducer from #209905. + return strrchr("/foo/bar.c", '/') ? strrchr("/foo/bar.c", '/') + 1 : "/foo/bar.c"; // no-warning +} + +void test_strchr_const_found(void) { + clang_analyzer_eval(strchr("/foo/bar.c", '/') == 0); // expected-warning {{FALSE}} +} + +void test_strrchr_const_found(void) { + clang_analyzer_eval(strrchr("/foo/bar.c", '/') == 0); // expected-warning {{FALSE}} +} + +// --- strchr / strrchr: target character is NOT in the literal --- +void test_strchr_const_not_found(void) { + clang_analyzer_eval(strchr("hello", 'z') == 0); // expected-warning {{TRUE}} +} + +void test_strrchr_const_not_found(void) { + clang_analyzer_eval(strrchr("hello", 'z') == 0); // expected-warning {{TRUE}} +} + +// --- memchr: character within bounds --- +void test_memchr_const_found(void) { + clang_analyzer_eval(memchr("abcdef", 'c', 6) == 0); // expected-warning {{FALSE}} +} + +// --- memchr: character beyond the specified length --- +void test_memchr_const_not_in_range(void) { + clang_analyzer_eval(memchr("abcdef", 'f', 3) == 0); // expected-warning {{TRUE}} +} + +// --- strstr: needle IS a substring --- +void test_strstr_const_found(void) { + clang_analyzer_eval(strstr("hello world", "world") == 0); // expected-warning {{FALSE}} +} + +// --- strstr: needle is NOT a substring --- +void test_strstr_const_not_found(void) { + clang_analyzer_eval(strstr("hello world", "xyz") == 0); // expected-warning {{TRUE}} +} + +// --- strpbrk: accept set has a match --- +void test_strpbrk_const_found(void) { + clang_analyzer_eval(strpbrk("hello", "aeiou") == 0); // expected-warning {{FALSE}} +} + +// --- strpbrk: no character from accept set in source --- +void test_strpbrk_const_not_found(void) { + clang_analyzer_eval(strpbrk("hello", "xyz") == 0); // expected-warning {{TRUE}} +} + +// --- Non-constant source: both branches must still exist --- +void test_strchr_non_const_source(const char *p) { + clang_analyzer_eval(strchr(p, '/') == 0); // expected-warning {{TRUE}} expected-warning {{FALSE}} +} + +// --- Various constant source forms: const, static const, #define, __FILE__ --- +static const char static_const_path[] = "/usr/local/bin/tool"; + +void test_strchr_static_const(void) { + clang_analyzer_eval(strchr(static_const_path, '/') == 0); // expected-warning {{FALSE}} +} + +const char global_const_path[] = "/etc/config"; + +void test_strrchr_global_const(void) { + clang_analyzer_eval(strrchr(global_const_path, '/') == 0); // expected-warning {{FALSE}} +} + +#define FIXED_PATH "/home/user/project/file.c" + +void test_strchr_define(void) { + clang_analyzer_eval(strchr(FIXED_PATH, '/') == 0); // expected-warning {{FALSE}} +} + +#define MY_FILE_BASENAME (strrchr(__FILE__, '/') ? strrchr(__FILE__, '/') + 1 : __FILE__) + +void test_file_basename_macro(void) { + const char *base = MY_FILE_BASENAME; // no-warning + (void)base; +} + +#define PREFIX "module" +#define SUFFIX "_handler" +// Adjacent string literal concatenation (the realistic preprocessor pattern): +#define MODULE_PATH "/opt/" PREFIX "/" SUFFIX ".so" + +void test_strchr_concatenated_define(void) { + clang_analyzer_eval(strchr(MODULE_PATH, '/') == 0); // expected-warning {{FALSE}} +} + +// --- Character argument via #define --- +#define SEPARATOR '/' + +void test_strchr_define_char(void) { + clang_analyzer_eval(strchr("/foo/bar", SEPARATOR) == 0); // expected-warning {{FALSE}} +} + +#define SEARCH_CHAR 'x' + +void test_strchr_define_char_not_found(void) { + clang_analyzer_eval(strchr("/foo/bar", SEARCH_CHAR) == 0); // expected-warning {{TRUE}} +} + +// --- Edge cases: null character '\0' --- +void test_strchr_null_char_always_found(void) { + // strchr(s, '\0') always finds the terminator. + clang_analyzer_eval(strchr("hello", '\0') == 0); // expected-warning {{FALSE}} +} + +void test_strrchr_null_char_always_found(void) { + clang_analyzer_eval(strrchr("hello", '\0') == 0); // expected-warning {{FALSE}} +} + +void test_memchr_null_char_within_bounds(void) { + // "abc" has terminator at index 3; searching 4 bytes includes it. + clang_analyzer_eval(memchr("abc", '\0', 4) == 0); // expected-warning {{FALSE}} +} + +void test_memchr_null_char_out_of_bounds(void) { + // "abc" has terminator at index 3; searching only 3 bytes misses it. + clang_analyzer_eval(memchr("abc", '\0', 3) == 0); // expected-warning {{TRUE}} +} + +// --- Edge cases: empty strings --- +void test_strchr_empty_haystack(void) { + // Empty string only contains '\0'; '/' is not there. + clang_analyzer_eval(strchr("", '/') == 0); // expected-warning {{TRUE}} +} + +void test_strchr_empty_haystack_null_char(void) { + // strchr("", '\0') finds the terminator. + clang_analyzer_eval(strchr("", '\0') == 0); // expected-warning {{FALSE}} +} + +void test_strstr_empty_needle(void) { + // strstr(s, "") always returns s. + clang_analyzer_eval(strstr("hello", "") == 0); // expected-warning {{FALSE}} +} + +void test_strpbrk_empty_accept(void) { + // strpbrk(s, "") never matches. + clang_analyzer_eval(strpbrk("hello", "") == 0); // expected-warning {{TRUE}} +} _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
