https://github.com/gamesh411 updated https://github.com/llvm/llvm-project/pull/198346
From 2347c4df43ac5b8ba529bb0566c9ffb372624a24 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Endre=20F=C3=BCl=C3=B6p?= <[email protected]> Date: Mon, 18 May 2026 15:59:20 +0200 Subject: [PATCH 1/3] [analyzer] Fix false positive in CStringChecker for offset buffer arguments CStringChecker::checkInit() was checking the wrong array elements when the buffer argument pointed into the middle of an array (e.g., memcpy(dst, &arr[i], size)). It was called with BufEnd instead of BufStart, making the ElementRegion index off by (size-1), and the element lookups were relative to array index 0 instead of the actual buffer start offset. Both bugs were introduced in 483557224b8d which added checkInit. --- .../Checkers/CStringChecker.cpp | 14 ++++++---- clang/test/Analysis/bstring_UninitRead.c | 26 +++++++++++++++++++ 2 files changed, 35 insertions(+), 5 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp index e486787bd8d7f..a6b5e53adf87f 100644 --- a/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp @@ -455,10 +455,11 @@ ProgramStateRef CStringChecker::checkInit(CheckerContext &C, ASTContext &Ctx = SVB.getContext(); const QualType ElemTy = Ctx.getBaseElementType(SuperR->getValueType()); - const NonLoc Zero = SVB.makeZeroArrayIndex(); + + const NonLoc StartIdx = ER->getIndex(); std::optional<Loc> FirstElementVal = - State->getLValue(ElemTy, Zero, loc::MemRegionVal(SuperR)).getAs<Loc>(); + State->getLValue(ElemTy, StartIdx, loc::MemRegionVal(SuperR)).getAs<Loc>(); if (!FirstElementVal) return State; @@ -478,8 +479,8 @@ ProgramStateRef CStringChecker::checkInit(CheckerContext &C, // We won't check whether the entire region is fully initialized -- let's just // check that the first and the last element is. So, onto checking the last // element: - const QualType IdxTy = SVB.getArrayIndexType(); + const QualType IdxTy = SVB.getArrayIndexType(); NonLoc ElemSize = SVB.makeIntVal(Ctx.getTypeSizeInChars(ElemTy).getQuantity(), IdxTy) .castAs<NonLoc>(); @@ -514,8 +515,11 @@ ProgramStateRef CStringChecker::checkInit(CheckerContext &C, const NonLoc One = SVB.makeIntVal(1, IdxTy).castAs<NonLoc>(); SVal LastIdx = SVB.evalBinOpNN(State, BO_Sub, *Offset, One, IdxTy); + const SVal AbsLastIdx = + SVB.evalBinOp(State, BO_Add, StartIdx, LastIdx, IdxTy); + SVal LastElementVal = - State->getLValue(ElemTy, LastIdx, loc::MemRegionVal(SuperR)); + State->getLValue(ElemTy, AbsLastIdx, loc::MemRegionVal(SuperR)); if (!isa<Loc>(LastElementVal)) return State; @@ -653,7 +657,7 @@ CStringChecker::CheckBufferAccess(CheckerContext &C, ProgramStateRef State, svalBuilder.evalBinOpLN(State, BO_Add, *BufLoc, LastOffset, PtrTy); State = CheckLocation(C, State, Buffer, BufEnd, Access, CK); if (Access == AccessKind::read) - State = checkInit(C, State, Buffer, BufEnd, *Length); + State = checkInit(C, State, Buffer, BufStart, *Length); // If the buffer isn't large enough, abort. if (!State) diff --git a/clang/test/Analysis/bstring_UninitRead.c b/clang/test/Analysis/bstring_UninitRead.c index 45e38dd316298..2bbb529ac92fa 100644 --- a/clang/test/Analysis/bstring_UninitRead.c +++ b/clang/test/Analysis/bstring_UninitRead.c @@ -111,6 +111,32 @@ void mempcpy_struct_fully_uninit() { mempcpy(&s2, &s1, sizeof(struct st)); } +void memcpy_offset_uninit(char *dst) { + char buf[10]; + buf[0] = 'a'; + // buf[5] is never written. + memcpy(dst, &buf[5], 3); // expected-warning{{The first element of the 2nd argument is undefined}} + // expected-note@-1{{Other elements might also be undefined}} +} + +void memcpy_offset_uninit_boundary(char *dst) { + char buf[10]; + buf[5] = 'a'; + // buf[6] and buf[7] are never written. + memcpy(dst, &buf[5], 3); // expected-warning{{The last accessed element (at index 2) in the 2nd argument is undefined}} + // expected-note@-1{{Other elements might also be undefined}} +} + +// Reduced false positive: memcpy from an offset into a partially initialized +// buffer should not warn if the accessed range is initialized. +void memcpy_offset_no_false_positive(char *dst) { + char buf[10]; + buf[5] = 'a'; + buf[6] = 'b'; + buf[7] = 'c'; + memcpy(dst, &buf[5], 3); // no-warning +} + // Creduced crash. In this case, an symbolicregion is wrapped in an // elementregion for the src argument. void *ga_copy_strings_from_0; From de699f606aa87a42028fbe90e00295617b6b29e2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Endre=20F=C3=BCl=C3=B6p?= <[email protected]> Date: Mon, 18 May 2026 19:31:08 +0200 Subject: [PATCH 2/3] fix formatting --- 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 a6b5e53adf87f..6bfecde97690d 100644 --- a/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp @@ -459,7 +459,8 @@ ProgramStateRef CStringChecker::checkInit(CheckerContext &C, const NonLoc StartIdx = ER->getIndex(); std::optional<Loc> FirstElementVal = - State->getLValue(ElemTy, StartIdx, loc::MemRegionVal(SuperR)).getAs<Loc>(); + State->getLValue(ElemTy, StartIdx, loc::MemRegionVal(SuperR)) + .getAs<Loc>(); if (!FirstElementVal) return State; From 1d85eac27a1da560f7e4d053d782564027cf53c7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Endre=20F=C3=BCl=C3=B6p?= <[email protected]> Date: Tue, 19 May 2026 17:51:08 +0200 Subject: [PATCH 3/3] use buffer pointer directly for init checks this has the added benefit of fixing another fp: the multi-dimensional array one in the test cases --- .../StaticAnalyzer/Checkers/CStringChecker.cpp | 17 +++++------------ clang/test/Analysis/bstring_UninitRead.c | 6 +----- 2 files changed, 6 insertions(+), 17 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp index 6bfecde97690d..3a4e2005724c3 100644 --- a/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp @@ -437,8 +437,8 @@ ProgramStateRef CStringChecker::checkInit(CheckerContext &C, if (!State) return nullptr; - const MemRegion *R = Element.getAsRegion(); - const auto *ER = dyn_cast_or_null<ElementRegion>(R); + SVal BufVal = C.getSVal(Buffer.Expression); + const auto *ER = dyn_cast_or_null<ElementRegion>(BufVal.getAsRegion()); if (!ER) return State; @@ -456,11 +456,8 @@ ProgramStateRef CStringChecker::checkInit(CheckerContext &C, const QualType ElemTy = Ctx.getBaseElementType(SuperR->getValueType()); - const NonLoc StartIdx = ER->getIndex(); - std::optional<Loc> FirstElementVal = - State->getLValue(ElemTy, StartIdx, loc::MemRegionVal(SuperR)) - .getAs<Loc>(); + State->getLValue(ElemTy, SVB.makeZeroArrayIndex(), BufVal).getAs<Loc>(); if (!FirstElementVal) return State; @@ -512,15 +509,11 @@ ProgramStateRef CStringChecker::checkInit(CheckerContext &C, if (!Offset) return State; - // Retrieve the index of the last element. + // Retrieve the index of the last element relative to the buffer pointer. const NonLoc One = SVB.makeIntVal(1, IdxTy).castAs<NonLoc>(); SVal LastIdx = SVB.evalBinOpNN(State, BO_Sub, *Offset, One, IdxTy); - const SVal AbsLastIdx = - SVB.evalBinOp(State, BO_Add, StartIdx, LastIdx, IdxTy); - - SVal LastElementVal = - State->getLValue(ElemTy, AbsLastIdx, loc::MemRegionVal(SuperR)); + SVal LastElementVal = State->getLValue(ElemTy, LastIdx, BufVal); if (!isa<Loc>(LastElementVal)) return State; diff --git a/clang/test/Analysis/bstring_UninitRead.c b/clang/test/Analysis/bstring_UninitRead.c index 2bbb529ac92fa..03dcec0b5d9aa 100644 --- a/clang/test/Analysis/bstring_UninitRead.c +++ b/clang/test/Analysis/bstring_UninitRead.c @@ -50,11 +50,7 @@ void memcpy_array_from_matrix(char *dst) { char buf[2][2]; buf[1][0] = 'i'; buf[1][1] = 'j'; - // FIXME: This is a FP -- we mistakenly retrieve the first element of buf, - // instead of the first element of buf[1]. getLValueElement simply peels off - // another ElementRegion layer, when in this case it really shouldn't. - memcpy(dst, buf[1], 2); // expected-warning{{The first element of the 2nd argument is undefined}} - // expected-note@-1{{Other elements might also be undefined}} + memcpy(dst, buf[1], 2); // no-warning (void)buf; } _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
