https://github.com/earnol updated https://github.com/llvm/llvm-project/pull/210154
>From 501238415430557876e9a690ee9373a1f00aab4e 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 | 155 +++++++++-- clang/test/Analysis/string-search-modeling.c | 242 ++++++++++++++++++ 2 files changed, 376 insertions(+), 21 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp index 745297dd1f057..7ccbf4767a643 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,146 @@ 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). + // When a match is found, compute the exact offset for precise pointer + // arithmetic downstream. + // Result encoding: nullopt = unknown, npos = not found, other = offset. + std::optional<size_t> ConstOffset = [&]() -> std::optional<size_t> { + const MemRegion *SrcRegion = SrcVal.getAsRegion(); + if (!SrcRegion) + return std::nullopt; + const StringLiteral *SrcLit = + getStringLiteralFromRegion(SrcRegion->StripCasts()); + if (!SrcLit) + return std::nullopt; + StringRef Haystack = SrcLit->getString(); + + SVal Arg1Val = State->getSVal(Call.getArgExpr(1), SF); + + // strchr/strrchr: find first or last occurrence of character. + auto matchChar = [&](std::function<size_t(StringRef, char)> Find) + -> std::optional<size_t> { + const llvm::APSInt *CharInt = SVB.getKnownValue(State, Arg1Val); + if (!CharInt) + return std::nullopt; + char Ch = static_cast<char>(CharInt->getExtValue()); + // The C string ends at the first null (embedded or at Haystack.size()). + size_t NulPos = Haystack.find('\0'); + size_t TermPos = (NulPos != StringRef::npos) ? NulPos : Haystack.size(); + if (Ch == '\0') + return TermPos; + return Find(Haystack.substr(0, TermPos), Ch); + }; + + // memchr: find character within the first N bytes. + auto matchMemchr = [&]() -> std::optional<size_t> { + const llvm::APSInt *CharInt = SVB.getKnownValue(State, Arg1Val); + if (!CharInt) + return std::nullopt; + if (Call.getNumArgs() < 3) + return std::nullopt; + const llvm::APSInt *Len = + SVB.getKnownValue(State, State->getSVal(Call.getArgExpr(2), SF)); + if (!Len) + return std::nullopt; + char Ch = static_cast<char>(CharInt->getExtValue()); + uint64_t N = Len->getZExtValue(); + // Include the implicit null terminator in the searchable region. + SmallString<64> Buf(Haystack); + Buf.push_back('\0'); + return StringRef(Buf.data(), Buf.size()).substr(0, N).find(Ch); + }; + + // Helper: extract string literal from the second argument. + auto getArg1String = [&]() -> std::optional<StringRef> { + const MemRegion *R = Arg1Val.getAsRegion(); + if (!R) + return std::nullopt; + const StringLiteral *Lit = getStringLiteralFromRegion(R->StripCasts()); + if (!Lit) + return std::nullopt; + return Lit->getString(); + }; + + // strstr: find first occurrence of substring. + auto matchSubstring = [&]() -> std::optional<size_t> { + auto Needle = getArg1String(); + if (!Needle) + return std::nullopt; + if (Needle->empty()) + return size_t{0}; + return Haystack.find(*Needle); + }; + + // strpbrk: find first character from accept set. + auto matchAnyChar = [&]() -> std::optional<size_t> { + auto Accept = getArg1String(); + if (!Accept) + return std::nullopt; + return Haystack.find_first_of(*Accept); + }; + + return llvm::StringSwitch<std::function<std::optional<size_t>()>>(FnName) + .Case("strchr()", + [&] { + return matchChar([](StringRef S, char C) { return S.find(C); }); + }) + .Case("strrchr()", + [&] { + return matchChar( + [](StringRef S, char C) { return S.rfind(C); }); + }) + .Case("memchr()", matchMemchr) + .Case("strstr()", matchSubstring) + .Case("strpbrk()", matchAnyChar) + .Default([&] { return std::nullopt; })(); + }(); + + bool MustMatch = ConstOffset && *ConstOffset != StringRef::npos; + bool MustNotMatch = ConstOffset && *ConstOffset == StringRef::npos; + + // NULL (no-match) branch — skip when the match is guaranteed. + if (CanReturnNull && !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()); - State = State->BindExpr(CE, SF, Result); - C.addTransition(State); - return; - } + // Found branch — skip when the match is impossible. + if (!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 we know the exact offset, use a concrete value. + if (MustMatch) { + NonLoc ConcreteOffset = + SVB.makeIntVal(*ConstOffset, Ctx.getSizeType()).castAs<NonLoc>(); + SVal Result = SVB.evalBinOpLN(State, BO_Add, *SrcLoc, ConcreteOffset, + Src.Expression->getType()); + State = State->BindExpr(CE, SF, Result); + C.addTransition(State); + } else { + // Unknown match: use a symbolic offset constrained to be in bounds. + 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); + 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..17fbf06bfcc02 100644 --- a/clang/test/Analysis/string-search-modeling.c +++ b/clang/test/Analysis/string-search-modeling.c @@ -176,3 +176,245 @@ 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}} +} + +//===----------------------------------------------------------------------===// +// Verify exact pointer offsets when both arguments are compile-time constants. +// The enhanced modeling returns Src + concrete_offset rather than a symbolic +// offset, enabling precise pointer arithmetic downstream. +//===----------------------------------------------------------------------===// + +// --- strchr: returns pointer to first occurrence --- +void test_strchr_exact_offset(void) { + const char *s = "/foo/bar.c"; + // '/' first appears at index 0. + clang_analyzer_eval(strchr(s, '/') == s); // expected-warning {{TRUE}} +} + +// --- strrchr: returns pointer to last occurrence --- +void test_strrchr_exact_offset(void) { + const char *s = "/foo/bar.c"; + // '/' last appears at index 4. + clang_analyzer_eval(strrchr(s, '/') == s + 4); // expected-warning {{TRUE}} +} + +// --- strrchr + 1: the FILE_BASENAME pattern --- +void test_strrchr_plus_one(void) { + const char *s = "/foo/bar.c"; + const char *base = strrchr(s, '/') + 1; + // Should point to 'b' at index 5. + clang_analyzer_eval(base == s + 5); // expected-warning {{TRUE}} +} + +// --- strchr with null terminator: points to end of string --- +void test_strchr_null_terminator_offset(void) { + const char *s = "hello"; + // strchr(s, '\0') returns pointer to the null terminator at index 5. + clang_analyzer_eval(strchr(s, '\0') == s + 5); // expected-warning {{TRUE}} +} + +// --- strstr: returns pointer to first substring match --- +void test_strstr_exact_offset(void) { + const char *s = "hello world"; + // "world" starts at index 6. + clang_analyzer_eval(strstr(s, "world") == s + 6); // expected-warning {{TRUE}} +} + +// --- strstr with empty needle: returns the source pointer --- +void test_strstr_empty_needle_offset(void) { + const char *s = "hello"; + clang_analyzer_eval(strstr(s, "") == s); // expected-warning {{TRUE}} +} + +// --- strpbrk: returns pointer to first matching character --- +void test_strpbrk_exact_offset(void) { + const char *s = "hello"; + // First vowel 'e' is at index 1. + clang_analyzer_eval(strpbrk(s, "aeiou") == s + 1); // expected-warning {{TRUE}} +} + +// --- memchr: returns pointer to character within bounds --- +void test_memchr_exact_offset(void) { + const char *s = "abcdef"; + // 'c' is at index 2. + clang_analyzer_eval(memchr(s, 'c', 6) == s + 2); // expected-warning {{TRUE}} +} + +// --- memchr with embedded null characters --- +void test_memchr_embedded_null(void) { + // String literal "ab\0cd" has a null at index 2, then 'c' at 3, 'd' at 4, + // and the implicit terminator at index 5. + const char *s = "ab\0cd"; + // memchr searching 5 bytes finds the first '\0' at index 2. + clang_analyzer_eval(memchr(s, '\0', 5) == s + 2); // expected-warning {{TRUE}} +} + +void test_memchr_second_segment_after_null(void) { + const char *s = "ab\0cd"; + // 'c' is at index 3; searching 5 bytes should find it. + clang_analyzer_eval(memchr(s, 'c', 5) == s + 3); // expected-warning {{TRUE}} +} + +void test_memchr_char_before_null_boundary(void) { + const char *s = "ab\0cd"; + // 'b' is at index 1; searching only 2 bytes still finds it. + clang_analyzer_eval(memchr(s, 'b', 2) == s + 1); // expected-warning {{TRUE}} +} + +void test_memchr_char_hidden_by_short_len(void) { + const char *s = "ab\0cd"; + // 'c' is at index 3 but searching only 3 bytes (indices 0-2) misses it. + clang_analyzer_eval(memchr(s, 'c', 3) == 0); // expected-warning {{TRUE}} +} _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
