[clang] [analyzer] Fix false positive in strchr/strrchr with constant args (PR #210154)
via cfe-commits
cfe-commits at lists.llvm.org
Thu Jul 16 12:47:07 PDT 2026
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 <vladislav.aranov at ericsson.com>
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}}
+}
More information about the cfe-commits
mailing list