https://github.com/steakhal created https://github.com/llvm/llvm-project/pull/212107
This reverts commit a34cb573eae65f48f0e51147289e042a86b55d16. This feature caused some issues (#209905), so the best course of action is to postpone it to clang-24 and revert from this branch. The #210154 tried to fix the surfaced issue, and considered to nominate it for backport in #211832, but ultimately we had to revert it from main in #211857 - this confirms that probably the best action is to revert the half-baked feature from the release branch. This means that #203260 won't be fixed in clang-23, and that's fine. From 092bf6d3d54bb34f50d417f4f11e2992d9c183d7 Mon Sep 17 00:00:00 2001 From: Balazs Benics <[email protected]> Date: Sun, 26 Jul 2026 11:15:19 +0100 Subject: [PATCH] Revert "[analyzer] Model strchr/strrchr/memchr/strstr/strpbrk/strchrnul (#207267)" This reverts commit a34cb573eae65f48f0e51147289e042a86b55d16. This feature caused some issues (#209905), so the best course of action is to postpone it to clang-24 and revert from this branch. The #210154 tried to fix the surfaced issue, and considered to nominate it for backport in #211832, but ultimately we had to revert it from main in #211857 - this confirms that probably the best action is to revert the half-baked feature from the release branch. This means that #203260 won't be fixed in clang-23, and that's fine. --- clang/docs/ReleaseNotes.md | 1 - .../Checkers/CStringChecker.cpp | 83 +------- clang/test/Analysis/string-search-modeling.c | 178 ------------------ 3 files changed, 4 insertions(+), 258 deletions(-) delete mode 100644 clang/test/Analysis/string-search-modeling.c diff --git a/clang/docs/ReleaseNotes.md b/clang/docs/ReleaseNotes.md index f4043d6c64a71..2ea954b442678 100644 --- a/clang/docs/ReleaseNotes.md +++ b/clang/docs/ReleaseNotes.md @@ -1270,7 +1270,6 @@ latest release, please see the [Clang Web Site](https://clang.llvm.org) or the - Added a new `check::LifetimeEnd` callback that fires for each `CFGLifetimeEnds` element, which is useful for detecting dangling pointers. (#GH201123) - The `unix.StdCLibraryFunctions` standard-library summaries were optimized for binary size. (#GH202662) - Fixed the alignment of entries printed by `clang -cc1 -analyzer-print-analyzer-options` / `-analyzer-help`. (#GH190570) -- Improved the models of `strchr`/`strrchr`/`memchr`/`strstr`/`strpbrk`/`strchrnul`, enabling `core.StackAddressEscape` to catch dangling pointers returned by these functions. (#GH203260) - Improved the modeling of symbolic ranges in the engine when calculating the largest and smallest possible values for range sets involving the `+`, `-`, and `*` binary operators. (#GH173113) #### Moved checkers diff --git a/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp index 745297dd1f057..32daa7045b12f 100644 --- a/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp @@ -28,7 +28,6 @@ #include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h" #include "llvm/ADT/APSInt.h" #include "llvm/ADT/STLExtras.h" -#include "llvm/ADT/STLForwardCompat.h" #include "llvm/ADT/StringExtras.h" #include "llvm/Support/raw_ostream.h" #include <functional> @@ -83,7 +82,7 @@ class CStringChecker : public CheckerFamily<eval::Call, check::PreStmt<DeclStmt>, check::LiveSymbols, check::DeadSymbols, check::RegionChanges> { - mutable StringRef CurrentFunctionDescription; + mutable const char *CurrentFunctionDescription = nullptr; public: // FIXME: The bug types emitted by this checker family have confused garbage @@ -163,24 +162,6 @@ class CStringChecker {{CDM::CLibrary, {"strncasecmp"}, 3}, &CStringChecker::evalStrncasecmp}, {{CDM::CLibrary, {"strsep"}, 2}, &CStringChecker::evalStrsep}, {{CDM::CLibrary, {"strxfrm"}, 3}, &CStringChecker::evalStrxfrm}, - {{CDM::CLibraryMaybeHardened, {"strchr"}, 2}, - llvm::bind_back(&CStringChecker::evalStrchrCommon, "strchr()", - /*CanReturnNull=*/true)}, - {{CDM::CLibraryMaybeHardened, {"strrchr"}, 2}, - llvm::bind_back(&CStringChecker::evalStrchrCommon, "strrchr()", - /*CanReturnNull=*/true)}, - {{CDM::CLibraryMaybeHardened, {"memchr"}, 3}, - llvm::bind_back(&CStringChecker::evalStrchrCommon, "memchr()", - /*CanReturnNull=*/true)}, - {{CDM::CLibrary, {"strstr"}, 2}, - llvm::bind_back(&CStringChecker::evalStrchrCommon, "strstr()", - /*CanReturnNull=*/true)}, - {{CDM::CLibrary, {"strpbrk"}, 2}, - llvm::bind_back(&CStringChecker::evalStrchrCommon, "strpbrk()", - /*CanReturnNull=*/true)}, - {{CDM::CLibrary, {"strchrnul"}, 2}, - llvm::bind_back(&CStringChecker::evalStrchrCommon, "strchrnul()", - /*CanReturnNull=*/false)}, {{CDM::CLibrary, {"bcopy"}, 3}, &CStringChecker::evalBcopy}, {{CDM::CLibrary, {"bcmp"}, 3}, std::bind(&CStringChecker::evalMemcmp, _1, _2, _3, CK_Regular)}, @@ -244,9 +225,6 @@ class CStringChecker void evalStrsep(CheckerContext &C, const CallEvent &Call) const; - void evalStrchrCommon(CheckerContext &C, const CallEvent &Call, - StringRef FnName, bool CanReturnNull) const; - void evalStdCopy(CheckerContext &C, const CallEvent &Call) const; void evalStdCopyBackward(CheckerContext &C, const CallEvent &Call) const; void evalStdCopyCommon(CheckerContext &C, const CallEvent &Call) const; @@ -402,7 +380,7 @@ ProgramStateRef CStringChecker::checkNonNull(CheckerContext &C, if (NullArg.isEnabled()) { SmallString<80> buf; llvm::raw_svector_ostream OS(buf); - assert(!CurrentFunctionDescription.empty()); + assert(CurrentFunctionDescription); OS << "Null pointer passed as " << (Arg.ArgumentIndex + 1) << llvm::getOrdinalSuffix(Arg.ArgumentIndex + 1) << " argument to " << CurrentFunctionDescription; @@ -1067,7 +1045,7 @@ SVal CStringChecker::getCStringLength(CheckerContext &C, ProgramStateRef &state, if (NotNullTerm.isEnabled()) { SmallString<120> buf; llvm::raw_svector_ostream os(buf); - assert(!CurrentFunctionDescription.empty()); + assert(CurrentFunctionDescription); os << "Argument to " << CurrentFunctionDescription << " is the address of the label '" << Label->getLabel()->getName() << "', which is not a null-terminated string"; @@ -1137,7 +1115,7 @@ SVal CStringChecker::getCStringLength(CheckerContext &C, ProgramStateRef &state, SmallString<120> buf; llvm::raw_svector_ostream os(buf); - assert(!CurrentFunctionDescription.empty()); + assert(CurrentFunctionDescription); os << "Argument to " << CurrentFunctionDescription << " is "; if (SummarizeRegion(os, C.getASTContext(), MR)) @@ -2639,59 +2617,6 @@ void CStringChecker::evalStrsep(CheckerContext &C, C.addTransition(State); } -void CStringChecker::evalStrchrCommon(CheckerContext &C, const CallEvent &Call, - StringRef FnName, - bool CanReturnNull) const { - CurrentFunctionDescription = FnName; - const Expr *CE = Call.getOriginExpr(); - assert(CE); - - // These functions always return a pointer. - if (!CE->getType()->isPointerType()) - return; - - ProgramStateRef State = C.getState(); - const StackFrame *SF = C.getStackFrame(); - SValBuilder &SVB = C.getSValBuilder(); - ASTContext &Ctx = C.getASTContext(); - - // The first argument must be non-null for all functions in this family. - SourceArgExpr Src = {{Call.getArgExpr(0), 0}}; - SVal SrcVal = State->getSVal(Src.Expression, SF); - State = checkNonNull(C, State, Src, SrcVal); - if (!State) - return; - - // NULL (no-match) branch. - if (CanReturnNull) { - 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; - } - - // 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. void CStringChecker::evalStdCopy(CheckerContext &C, const CallEvent &Call) const { diff --git a/clang/test/Analysis/string-search-modeling.c b/clang/test/Analysis/string-search-modeling.c deleted file mode 100644 index a50ec439731a3..0000000000000 --- a/clang/test/Analysis/string-search-modeling.c +++ /dev/null @@ -1,178 +0,0 @@ -// RUN: %clang_analyze_cc1 -verify %s \ -// RUN: -analyzer-checker=core,unix \ -// RUN: -analyzer-checker=debug.ExprInspection \ -// RUN: -analyzer-config eagerly-assume=false - -typedef __SIZE_TYPE__ size_t; -void *malloc(size_t size); -void free(void *p); -void *memcpy(void *dest, const void *src, size_t n); -char *strchr(const char *s, int c); -char *strrchr(const char *s, int c); -char *strstr(const char *haystack, const char *needle); -char *strpbrk(const char *s, const char *accept); -void *memchr(const void *s, int c, size_t n); -char *strchrnul(const char *s, int c); - -void clang_analyzer_eval(int); - -//===----------------------------------------------------------------------===// -// Check for stack address escapes. -//===----------------------------------------------------------------------===// - -char *returns_stack_strchr(void) { - char buf[8] = "abc"; - return strchr(buf, 'b'); - // expected-warning@-1 {{Address of stack memory associated with local variable 'buf' returned to caller}} -} - -char *returns_stack_strrchr(void) { - char buf[8] = "abc"; - return strrchr(buf, 'b'); - // expected-warning@-1 {{Address of stack memory associated with local variable 'buf' returned to caller}} -} - -char *returns_stack_strstr(void) { - char buf[8] = "abc"; - return strstr(buf, "b"); - // expected-warning@-1 {{Address of stack memory associated with local variable 'buf' returned to caller}} -} - -char *returns_stack_strpbrk(void) { - char buf[8] = "abc"; - return strpbrk(buf, "b"); - // expected-warning@-1 {{Address of stack memory associated with local variable 'buf' returned to caller}} -} - -void *returns_stack_memchr(void) { - char buf[8] = "abc"; - return memchr(buf, 'b', sizeof buf); - // expected-warning@-1 {{Address of stack memory associated with local variable 'buf' returned to caller}} -} - -char *returns_stack_strchrnul(void) { - char buf[8] = "abc"; - return strchrnul(buf, 'b'); - // expected-warning@-1 {{Address of stack memory associated with local variable 'buf' returned to caller}} -} - -char *forwards_param(char *p) { - return strchr(p, 'b'); // no-warning -} - -char *returns_local_static(void) { - extern char g[8]; - return strchr(g, 'b'); // no-warning -} - -//===----------------------------------------------------------------------===// -// unix.cstring.NullArg: the source pointer must be non-null. -//===----------------------------------------------------------------------===// - -void null_source_strchr(int c) { - strchr(0, c); - // expected-warning@-1 {{Null pointer passed as 1st argument to strchr()}} -} - -void null_source_strrchr(int c) { - strrchr(0, c); - // expected-warning@-1 {{Null pointer passed as 1st argument to strrchr()}} -} - -void null_source_memchr(int c) { - memchr(0, c, 4); - // expected-warning@-1 {{Null pointer passed as 1st argument to memchr()}} -} - -void null_source_strstr(void) { - strstr(0, "x"); - // expected-warning@-1 {{Null pointer passed as 1st argument to strstr()}} -} - -void null_source_strpbrk(void) { - strpbrk(0, "x"); - // expected-warning@-1 {{Null pointer passed as 1st argument to strpbrk()}} -} - -void null_source_strchrnul(int c) { - strchrnul(0, c); - // expected-warning@-1 {{Null pointer passed as 1st argument to strchrnul()}} -} - -//===----------------------------------------------------------------------===// -// State split: result == NULL on one branch, in the source on the other. -//===----------------------------------------------------------------------===// - -// Both branches are reachable; the verifier matches the two values set-wise. -void state_split(const char *p) { - clang_analyzer_eval(strchr(p, 'b') == 0); // expected-warning {{TRUE}} expected-warning {{FALSE}} - clang_analyzer_eval(strrchr(p, 'b') == 0); // expected-warning {{TRUE}} expected-warning {{FALSE}} - clang_analyzer_eval(strstr(p, "x") == 0); // expected-warning {{TRUE}} expected-warning {{FALSE}} - clang_analyzer_eval(strpbrk(p, "x") == 0); // expected-warning {{TRUE}} expected-warning {{FALSE}} - clang_analyzer_eval(memchr(p, 'b', 4) == 0); // expected-warning {{TRUE}} expected-warning {{FALSE}} -} - -// strchrnul does not split: it never returns NULL at runtime. -void strchrnul_is_nonnull(const char *p) { - clang_analyzer_eval(strchrnul(p, 'b') == 0); // expected-warning {{FALSE}} -} - -// On the "found" branch the result aliases the source region, but the offset -// is opaque, so equality with in-source pointers is UNKNOWN. -void found_branch_offset_is_opaque(const char *p) { - char *q = strchr(p, 'b'); - if (!q) return; // constrain to "found" branch - clang_analyzer_eval(q == p); // expected-warning {{UNKNOWN}} - clang_analyzer_eval(q == p + 1); // expected-warning {{UNKNOWN}} -} - -void resulting_ptr_shares_provenance_with_src(int rng, char *opaque) { - if (rng == 10) { - char *q = strchr("abcd", 'b'); - free(q); // expected-warning {{Argument to 'free()' is the address of a global variable, which is not memory allocated by 'malloc()'}} - return; - } - - if (rng == 20) { - char *q = strchr(opaque, 'b'); - free(q); // ok - return; - } - - if (rng == 30) { - char *q = strchr(opaque, 'b'); - free(q); // Notionally releases 'opaque'. - free(opaque); // expected-warning {{Attempt to release already released memory}} - return; - } -} - -//===----------------------------------------------------------------------===// -// core.NullDereference: -// A returned pointer used without a NULL check is flagged on the NULL branch. -//===----------------------------------------------------------------------===// - -void deref_unchecked(const char *s) { - char *p = strchr(s, 'b'); - *p = 'X'; // expected-warning {{Dereference of null pointer}} -} - -void deref_after_check(const char *s) { - char *p = strchr(s, 'b'); - if (p) { - *p = 'X'; // no-warning - } -} - -//===----------------------------------------------------------------------===// -// Calling these functions does not invalidate unrelated memory. -//===----------------------------------------------------------------------===// - -int global_unmodified; -void no_invalidation_of_globals(const char *p) { - int local_unmodified = 10; - global_unmodified = 20; - (void)strchr(p, 'b'); - clang_analyzer_eval(local_unmodified == 10); // expected-warning {{TRUE}} - clang_analyzer_eval(global_unmodified == 20); // expected-warning {{TRUE}} -} _______________________________________________ llvm-branch-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
