[analyzer][CStringChecker] Adjust the invalidation operation on the super region of the destination buffer during string copy

[OikawaKirie]

Summary

Fixing GitHub issue: https://github.com/llvm/llvm-project/issues/55019
Following the previous fix https://reviews.llvm.org/D12571 on issue https://github.com/llvm/llvm-project/issues/23328

The two issues report false memory leaks after calling string-copy APIs with a buffer field in an object as the destination.
The buffer invalidation incorrectly drops the assignment to a heap memory block when no overflow problems happen.
And the pointer of the dropped assignment is declared in the same object of the destination buffer.

The previous fix only considers the memcpy functions whose copy length is available from arguments.
In this issue, the copy length is inferable from the buffer declaration and string literals being copied.
Therefore, I have adjusted the previous fix to reuse the copy length computed before.

Besides, for APIs that never overflow (strsep) or we never know whether they can overflow (std::copy),
new invalidation operations have been introduced to inform CStringChecker::InvalidateBuffer whether or not to
invalidate the super region that encompasses the destination buffer.

[steakhal]

I left my comment at https://github.com/llvm/llvm-project/issues/55019

[steakhal]

Thanks for the elaborated answer on the GH issue. Now it makes sense. And the fix is align with the observations, which is good.
I only have concerns about the code quality of the modifier code. It was already in pretty bad shape, and I cannot say we improve it with this patch.
However, I can also see that it might require a bit engineering to refactor it into something cleaner, so I won't object if you push back.

Thanks for the patch!

clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp

263–280

I don't think this is the cleanest way to achieve this.
To me, it feels like it might make sense to have distinct invalidation functions for each scenario and mentally share the same overload-set.
Right now, the API looks really complicated: using enum values along with default parameters. Not to speak of how many parameters it accepts.
However, I won't object this time given that it was already pretty bad, and unreadable - so in that sense, it's not much worse this way and it was not the point to improve the quality of the code.

1005–1006

The type of the SVal might not be always accurate.
in fact, we try to move away from using that in the future - whenever possible.
Consequently, I'd prefer passing down the type as an additional parameter instead, or just keeping the Expr along with the corresponding SVal.

1139–1140

Again, it's not your fault, but this code just looks insane.
I'm not surprised it had a bug, and likely it still has.

clang/test/Analysis/issue-55019.c

3 ↗

(On Diff #529555)

Why do you need the Wno-strict-prototypes? Can we make it work with it?

8 ↗

(On Diff #529555)

I guess this include is only for the size_t, right?
In c++, you could use using size_t = decltype(sizeof(int)); to define it.

Ah, I see that it's for c function declarations. If that's the case, have you considered adding the malloc and free declarations to that header?

26 ↗

(On Diff #529555)

I believe you can use the sizeof(x.arr) instead.

28 ↗

(On Diff #529555)

I would rather use no-leak-warning here to be more specific about what we don't expect.

clang/test/Analysis/issue-55019.cpp

2–5

I think you can just append the C file to this one.
This shouldn't make a difference. And if it does, you can still guard that test code using macro ifndef trickery in addition to distinct -verify=xxx prefixes.

25–27

So, you say that It should be HeapSymRegion instead because x.arr shouldn't be invalidated because the std::copy` won't modify it. Right?

clang/test/Analysis/pr22954.c

560

Ah, the leak should have been reported at the end of the full-expression of memcpy.
If we have this expect line here it could mislead the reader that it has anything to do with the eval call before.

In fact, any statement after memcpy would have this diagnostic.
I have seen cases where we have a next_line() opaque call after such places to anchor the leak diagnostic.
I think we can do the same here.

[OikawaKirie]

Update the implementation of InvalidateBuffer in a multi-entrance-with-callback manner.
Update test cases as suggested.

[OikawaKirie]

clang/test/Analysis/issue-55019.cpp

14–15

Ah, I see that it's for c function declarations. If that's the case, have you considered adding the malloc and free declarations to that header?

What about doing this in another patch and updating all test cases that use malloc and free? Maybe other libc APIs as well?
The test case malloc-three-arg.c declares malloc and in a different signature and also includes this header. Simply doing so in this patch will lead to other conflicts.

clang/test/Analysis/pr22954.c

581

Do I need to update all other leak expectation directions in this file as well? Such as this one here.

[steakhal]

Let me come back to this once again in the upcoming days to make sure everything is good.

clang/test/Analysis/pr22954.c

581

Definitely no.
Its not advised to have different, but similar looking hunks out of which some are the consequence of the semantic change we made and also have others where they are just refactors.

[steakhal]

clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp

286–287

I'd highly suggest making this a template taking the functor that way.
Given that we modify these functions, we should make them comply with the lower camel case naming convention.
And their variables with upper camel case.

1056

Shouldn't we assert that SizeV is some known value or at least smaller than (or equal to) the extent of the buffer?

1077–1079

The lambdas inline in the call look a bit messy. I would prefer declaring it and passing it as an argument separately.
This applies to the rest of the lambdas as well.

clang/test/Analysis/issue-55019.cpp

14–15

Okay.

[OikawaKirie]

BTW, what does the Done checkbox mean in the code comments?

clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp

286–287

I'd highly suggest making this a template taking the functor that way.

Any examples?
Do you mean

template <typename T>
static ProgramStateRef
InvalidateBufferAux(CheckerContext &C, ProgramStateRef state, const Expr *Ex,
                    SVal V, std::function<T> CallBack);

1056

The calls in CStringChecker::evalCopyCommon and CStringChecker::evalStrcpyCommon seem to have chances able to pass an unknown value. But I am not very sure about this.

If the SizeV is sure to be smaller or equal to the extent of the buffer, the IsFirstBufInBound check seems useless.
Maybe I need to dig deeper into the callers.

1077–1079

And their variables with upper camel case.

What about the variable names for the lambdas? Lower camel or upper camel?
I cannot find the rules for named lambdas from the guidance https://llvm.org/docs/CodingStandards.html.

clang/test/Analysis/issue-55019.cpp

81–88

So, you say that It should be HeapSymRegion instead because x.arr shouldn't be invalidated because the std::copy won't modify it. Right?

Yes, but just in this test case.

The size of x.arr is 4 and the copy length is 1, which means the call to std::copy here never overflows.
Therefore, we should not invalidate the super region x and keep x.ptr still pointing to the original HeapSymRegion unchanged.
However, the copy length computed through iterators is not modeled currently in CStringChecker::evalStdCopyCommon, where InvalidateDestinationBufferAlwaysEscapeSuperRegion is called.
When it is modeled in the future, the function should call InvalidateDestinationBufferBySize instead to make the invalidation of the super region determined by the copy length to report potential leaks.

clang/test/Analysis/pr22954.c

560

Its not advised to have different, but similar looking hunks out of which some are the consequence of the semantic change we made and also have others where they are just refactors.

Do you mean I only need to update the verification direction here as you suggested above and leave others unchanged?

[steakhal]

In D152435#4433147, @OikawaKirie wrote:

BTW, what does the Done checkbox mean in the code comments?

"Done" means "Resolved" on GitHub - which is basically that some action was taken or the person who raised the issue withdrew.
In general, no revision/pull request should be merged with having open/unresolved discussions.
Usually, the patch author marks comments as "Done" and on the next update (by pressing the "Submit" button) all those threads will be closed automatically.

clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp

286–287

My problem is that std::function owns the callback, and really likely to allocate for doing so.
And in this context a more lightweight approach would be also good.
What I had in mind was:

template <typename Callback>
static ProgramStateRef
InvalidateBufferAux(CheckerContext &C, ProgramStateRef state, const Expr *Ex,
                    SVal V, Callback F);
...
bool asd = F(a, b);

But actually, llvm::function_ref would be probably even better, so use that instead.
Also, add some comments on what the callback's return type means etc.

1056

Indeed.

1077–1079

I think I asked the same question some time ago.
https://discourse.llvm.org/t/naming-lambdas-in-llvm-coding-standards/62689/3

clang/test/Analysis/issue-55019.cpp

81–88

Makes sense. Thanks.

[OikawaKirie]

1. Function and variable names: functions: lower camel, variables: upper camel, lambdas: upper camel
2. std::function -> llvm::function_ref
3. update test case verification directions by following others in this file.

clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp

1056

Function CStringChecker::CheckBufferAccess is responsible to check whether the size overflows.
However, when Filter.CheckCStringOutOfBounds is set to false, the function will skip the overflow checking.
This makes that we cannot assert SizeV is always inbound, and the check in IsFirstBufInBound seems to be necessary.
(evalMemset -> CheckBufferAccess(skip overflow checking) -> memsetAux -> InvalidateDestinationBufferBySize with function call of memset(x.arr, 'a', 42))

Besides, if the size argument is originally an unknown value expression (e.g. (unsigned char) 1.0), the unknown value can be passed to this function.
(CStringChecker::evalCopyCommon -> invalidateDestinationBufferBySize, with function call of memcpy(x.arr, "hi", (unsigned char) 1.0))

Hence, we cannot make such an assertion here. : (

[OikawaKirie]

Sorry for the formatting error... :(

[steakhal]

clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp

1078

[OikawaKirie]

I will submit it again later to update the code as you suggested.

Besides,

Build Status
Buildable 242737
Build 376801: pre-merge checks libcxx CI FAILED

What does this mean? Was this patch correctly compiled and checked in the previous build? How can I avoid this failure?

Thx

[steakhal]

In D152435#4467672, @OikawaKirie wrote:

I will submit it again later to update the code as you suggested.

Feel free to commit it with that modifications included.

Besides,

Build Status
Buildable 242737
Build 376801: pre-merge checks libcxx CI FAILED

What does this mean? Was this patch correctly compiled and checked in the previous build? How can I avoid this failure?

This is likely some anomaly. In general, you should check those logs to see if it's related to your patch. If not, than you can proceed and check if any of the bots cry after you commit.
If they do, triage what went wrong and decide if you attempt a fix commit or a revert.

clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp

SVal getCStringLength(CheckerContext &C,
                         SVal Buf,
                         bool hypothetical = false) const;
 
   const StringLiteral *getCStringLiteral(CheckerContext &C,
                                          ProgramStateRef &state,
                                          const Expr *expr,
                                          SVal val) const;
 
-  static ProgramStateRef InvalidateBuffer(CheckerContext &C,
-                                          ProgramStateRef state,
-                                          const Expr *Ex, SVal V,
-                                          bool IsSourceBuffer,
-                                          const Expr *Size);
+  /// Invalidate the destination buffer determined by characters copied.
+  static ProgramStateRef
+  invalidateDestinationBufferBySize(CheckerContext &C, ProgramStateRef S,
+                                    const Expr *BufE, SVal BufV, SVal SizeV,
+                                    QualType SizeTy);
+
+  /// Operation never overflows, do not invalidate the super region.
+  static ProgramStateRef invalidateDestinationBufferNeverOverflows(
+      CheckerContext &C, ProgramStateRef S, const Expr *BufE, SVal BufV);
+
+  /// We do not know whether the operation can overflow (e.g. size is unknown),
+  /// invalidate the super region and escape related pointers.
+  static ProgramStateRef invalidateDestinationBufferAlwaysEscapeSuperRegion(
+      CheckerContext &C, ProgramStateRef S, const Expr *BufE, SVal BufV);
+
+  /// Invalidate the source buffer for escaping pointers.
+  static ProgramStateRef invalidateSourceBuffer(CheckerContext &C,
+                                                ProgramStateRef S,
+                                                const Expr *BufE, SVal BufV);
+
+  /// @param InvalidationTraitOperations Determine how to invlidate the
+  /// MemRegion by setting the invalidation traits. Return true to cause pointer
+  /// escape, or false otherwise.
+  static ProgramStateRef invalidateBufferAux(
+      CheckerContext &C, ProgramStateRef State, const Expr *Ex, SVal V,
+      llvm::function_ref<bool(RegionAndSymbolInvalidationTraits &,
+                              const MemRegion *)>
+          InvalidationTraitOperations);
 
   static bool SummarizeRegion(raw_ostream &os, ASTContext &Ctx,
                               const MemRegion *MR);
 
   static bool memsetAux(const Expr *DstBuffer, SVal CharE,
                         const Expr *Size, CheckerContext &C,
                         ProgramStateRef &State);
 
public:
   ProgramStateRef checkAdditionOverflow(CheckerContext &C,
                                             ProgramStateRef state,
                                             NonLoc left,
                                             NonLoc right) const;
 
   // Return true if the destination buffer of the copy function may be in bound.
   // Expects SVal of Size to be positive and unsigned.
   // Expects SVal of FirstBuf to be a FieldRegion.
-  static bool IsFirstBufInBound(CheckerContext &C,
-                                ProgramStateRef state,
-                                const Expr *FirstBuf,
-                                const Expr *Size);
+  static bool isFirstBufInBound(CheckerContext &C, ProgramStateRef State,
+                                SVal BufVal, QualType BufTy, SVal LengthVal,
+                                QualType LengthTy);
 };
 
 } //end anonymous namespace
 
 REGISTER_MAP_WITH_PROGRAMSTATE(CStringLength, const MemRegion *, SVal)
 
 //===----------------------------------------------------------------------===//
 // Individual checks and utility methods.
const StringLiteral *CStringChecker::getCStringLiteral(CheckerContext &C,
   const StringRegion *strRegion= dyn_cast<StringRegion>(bufRegion);
   if (!strRegion)
     return nullptr;
 
   // Return the actual string in the string region.
   return strRegion->getStringLiteral();
 }
 
-bool CStringChecker::IsFirstBufInBound(CheckerContext &C,
-                                       ProgramStateRef state,
-                                       const Expr *FirstBuf,
-                                       const Expr *Size) {
+bool CStringChecker::isFirstBufInBound(CheckerContext &C, ProgramStateRef State,
+                                       SVal BufVal, QualType BufTy,
+                                       SVal LengthVal, QualType LengthTy) {
   // If we do not know that the buffer is long enough we return 'true'.
   // Otherwise the parent region of this field region would also get
   // invalidated, which would lead to warnings based on an unknown state.
 
+  if (LengthVal.isUnknown())
+    return false;
+
   // Originally copied from CheckBufferAccess and CheckLocation.
-  SValBuilder &svalBuilder = C.getSValBuilder();
-  ASTContext &Ctx = svalBuilder.getContext();
-  const LocationContext *LCtx = C.getLocationContext();
+  SValBuilder &SB = C.getSValBuilder();
+  ASTContext &Ctx = C.getASTContext();
 
-  QualType sizeTy = Size->getType();
   QualType PtrTy = Ctx.getPointerType(Ctx.CharTy);
-  SVal BufVal = state->getSVal(FirstBuf, LCtx);
 
-  SVal LengthVal = state->getSVal(Size, LCtx);
   std::optional<NonLoc> Length = LengthVal.getAs<NonLoc>();
   if (!Length)
     return true; // cf top comment.
 
   // Compute the offset of the last element to be accessed: size-1.
-  NonLoc One = svalBuilder.makeIntVal(1, sizeTy).castAs<NonLoc>();
-  SVal Offset = svalBuilder.evalBinOpNN(state, BO_Sub, *Length, One, sizeTy);
+  NonLoc One = SB.makeIntVal(1, LengthTy).castAs<NonLoc>();
+  SVal Offset = SB.evalBinOpNN(State, BO_Sub, *Length, One, LengthTy);
   if (Offset.isUnknown())
     return true; // cf top comment
   NonLoc LastOffset = Offset.castAs<NonLoc>();
 
   // Check that the first buffer is sufficiently long.
-  SVal BufStart = svalBuilder.evalCast(BufVal, PtrTy, FirstBuf->getType());
+  SVal BufStart = SB.evalCast(BufVal, PtrTy, BufTy);
   std::optional<Loc> BufLoc = BufStart.getAs<Loc>();
   if (!BufLoc)
     return true; // cf top comment.
 
-  SVal BufEnd =
-      svalBuilder.evalBinOpLN(state, BO_Add, *BufLoc, LastOffset, PtrTy);
+  SVal BufEnd = SB.evalBinOpLN(State, BO_Add, *BufLoc, LastOffset, PtrTy);
 
   // Check for out of bound array element access.
   const MemRegion *R = BufEnd.getAsRegion();
   if (!R)
     return true; // cf top comment.
 
   const ElementRegion *ER = dyn_cast<ElementRegion>(R);
   if (!ER)
     return true; // cf top comment.
 
   // FIXME: Does this crash when a non-standard definition
   // of a library function is encountered?
   assert(ER->getValueType() == C.getASTContext().CharTy &&
-         "IsFirstBufInBound should only be called with char* ElementRegions");
+         "isFirstBufInBound should only be called with char* ElementRegions");
 
   // Get the size of the array.
   const SubRegion *superReg = cast<SubRegion>(ER->getSuperRegion());
-  DefinedOrUnknownSVal SizeDV = getDynamicExtent(state, superReg, svalBuilder);
+  DefinedOrUnknownSVal SizeDV = getDynamicExtent(State, superReg, SB);
 
   // Get the index of the accessed element.
   DefinedOrUnknownSVal Idx = ER->getIndex().castAs<DefinedOrUnknownSVal>();
 
-  ProgramStateRef StInBound = state->assumeInBound(Idx, SizeDV, true);
+  ProgramStateRef StInBound = State->assumeInBound(Idx, SizeDV, true);
 
   return static_cast<bool>(StInBound);
 }
 
-ProgramStateRef CStringChecker::InvalidateBuffer(CheckerContext &C,
-                                                 ProgramStateRef state,
-                                                 const Expr *E, SVal V,
-                                                 bool IsSourceBuffer,
-                                                 const Expr *Size) {
+ProgramStateRef CStringChecker::invalidateDestinationBufferBySize(
+    CheckerContext &C, ProgramStateRef S, const Expr *BufE, SVal BufV,
+    SVal SizeV, QualType SizeTy) {
+  auto InvalidationTraitOperations =
+      [&C, S, BufTy = BufE->getType(), BufV, SizeV,
+       SizeTy](RegionAndSymbolInvalidationTraits &ITraits, const MemRegion *R) {
+        // If destination buffer is a field region and access is in bound, do
+        // not invalidate its super region.
+        if (MemRegion::FieldRegionKind == R->getKind() &&
+            isFirstBufInBound(C, S, BufV, BufTy, SizeV, SizeTy)) {
+          ITraits.setTrait(
+              R,
+              RegionAndSymbolInvalidationTraits::TK_DoNotInvalidateSuperRegion);
+        }
+        return false;
+      };
+
+  return invalidateBufferAux(C, S, BufE, BufV, InvalidationTraitOperations);
+}
+
+ProgramStateRef
+CStringChecker::invalidateDestinationBufferAlwaysEscapeSuperRegion(
+    CheckerContext &C, ProgramStateRef S, const Expr *BufE, SVal BufV) {
+  auto InvalidationTraitOperations = [](RegionAndSymbolInvalidationTraits &,
+                                        const MemRegion *R) {
+    return isa<FieldRegion>(R);
+  };
+
+  return invalidateBufferAux(C, S, BufE, BufV, InvalidationTraitOperations);
+}
+
+ProgramStateRef CStringChecker::invalidateDestinationBufferNeverOverflows(
+    CheckerContext &C, ProgramStateRef S, const Expr *BufE, SVal BufV) {
+  auto InvalidationTraitOperations =
+      [](RegionAndSymbolInvalidationTraits &ITraits, const MemRegion *R) {
+        if (MemRegion::FieldRegionKind == R->getKind())
+          ITraits.setTrait(
+              R,
+              RegionAndSymbolInvalidationTraits::TK_DoNotInvalidateSuperRegion);
+        return false;
+      };
+
+  return invalidateBufferAux(C, S, BufE, BufV, InvalidationTraitOperations);
+}
+
+ProgramStateRef CStringChecker::invalidateSourceBuffer(CheckerContext &C,
+                                                       ProgramStateRef S,
+                                                       const Expr *BufE,
+                                                       SVal BufV) {
+  auto InvalidationTraitOperations =
+      [](RegionAndSymbolInvalidationTraits &ITraits, const MemRegion *R) {
+        ITraits.setTrait(
+            R->getBaseRegion(),
+            RegionAndSymbolInvalidationTraits::TK_PreserveContents);
+        ITraits.setTrait(R,
+                         RegionAndSymbolInvalidationTraits::TK_SuppressEscape);
+        return true;
+      };
+
+  return invalidateBufferAux(C, S, BufE, BufV, InvalidationTraitOperations);
+}
+
+ProgramStateRef CStringChecker::invalidateBufferAux(
+    CheckerContext &C, ProgramStateRef State, const Expr *E, SVal V,
+    llvm::function_ref<bool(RegionAndSymbolInvalidationTraits &,
+                            const MemRegion *)>
+        InvalidationTraitOperations) {
   std::optional<Loc> L = V.getAs<Loc>();
   if (!L)
-    return state;
+    return State;
 
   // FIXME: This is a simplified version of what's in CFRefCount.cpp -- it makes
   // some assumptions about the value that CFRefCount can't. Even so, it should
   // probably be refactored.
   if (std::optional<loc::MemRegionVal> MR = L->getAs<loc::MemRegionVal>()) {
     const MemRegion *R = MR->getRegion()->StripCasts();
 
     // Are we dealing with an ElementRegion?  If so, we should be invalidating
     // the super-region.
     if (const ElementRegion *ER = dyn_cast<ElementRegion>(R)) {
       R = ER->getSuperRegion();
       // FIXME: What about layers of ElementRegions?
     }
 
     // Invalidate this region.
     const LocationContext *LCtx = C.getPredecessor()->getLocationContext();
-
-    bool CausesPointerEscape = false;
     RegionAndSymbolInvalidationTraits ITraits;
-    // Invalidate and escape only indirect regions accessible through the source
-    // buffer.
-    if (IsSourceBuffer) {
-      ITraits.setTrait(R->getBaseRegion(),
-                       RegionAndSymbolInvalidationTraits::TK_PreserveContents);
-      ITraits.setTrait(R, RegionAndSymbolInvalidationTraits::TK_SuppressEscape);
-      CausesPointerEscape = true;
-    } else {
-      const MemRegion::Kind& K = R->getKind();
-      if (K == MemRegion::FieldRegionKind)
-        if (Size && IsFirstBufInBound(C, state, E, Size)) {
-          // If destination buffer is a field region and access is in bound,
-          // do not invalidate its super region.
-          ITraits.setTrait(
-              R,
-              RegionAndSymbolInvalidationTraits::TK_DoNotInvalidateSuperRegion);
-        }
-    }
+    bool CausesPointerEscape = InvalidationTraitOperations(ITraits, R);
 
-    return state->invalidateRegions(R, E, C.blockCount(), LCtx,
+    return State->invalidateRegions(R, E, C.blockCount(), LCtx,
                                     CausesPointerEscape, nullptr, nullptr,
                                     &ITraits);
   }
 
   // If we have a non-region value by chance, just remove the binding.
   // FIXME: is this necessary or correct? This handles the non-Region
   //  cases.  Is it ever valid to store to these?
-  return state->killBinding(*L);
+  return State->killBinding(*L);
 }
 
 bool CStringChecker::SummarizeRegion(raw_ostream &os, ASTContext &Ctx,
                                      const MemRegion *MR) {
   switch (MR->getKind()) {
   case MemRegion::FunctionCodeRegionKind: {
     if (const auto *FD = cast<FunctionCodeRegion>(MR)->getDecl())
       os << "the address of the function '" << *FD << '\'';
if (StateWholeReg && !StateNotWholeReg && StateNullChar &&
       // FIXME: Since there is no perfect way to bind the non-zero character, we
       // can only deal with zero value here. In the future, we need to deal with
       // the binding of non-zero value in the case of whole region.
       State = State->bindDefaultZero(svalBuilder.makeLoc(BR),
                                      C.getLocationContext());
     } else {
       // If the destination buffer's extent is not equal to the value of
       // third argument, just invalidate buffer.
-      State = InvalidateBuffer(C, State, DstBuffer, MemVal,
-                               /*IsSourceBuffer*/ false, Size);
+      State = invalidateDestinationBufferBySize(C, State, DstBuffer, MemVal,
+                                                SizeVal, Size->getType());
     }
 
     if (StateNullChar && !StateNonNullChar) {
       // If the value of the second argument of 'memset()' is zero, set the
       // string length of destination buffer to 0 directly.
       State = setCStringLength(State, MR,
                                svalBuilder.makeZeroVal(Ctx.getSizeType()));
     } else if (!StateNullChar && StateNonNullChar) {
       SVal NewStrLen = svalBuilder.getMetadataSymbolVal(
           CStringChecker::getTag(), MR, DstBuffer, Ctx.getSizeType(),
           C.getLocationContext(), C.blockCount());
 
       // If the value of second argument is not zero, then the string length
       // is at least the size argument.
       SVal NewStrLenGESize = svalBuilder.evalBinOp(
           State, BO_GE, NewStrLen, SizeVal, svalBuilder.getConditionType());
 
       State = setCStringLength(
           State->assume(NewStrLenGESize.castAs<DefinedOrUnknownSVal>(), true),
           MR, NewStrLen);
     }
   } else {
     // If the offset is not zero and char value is not concrete, we can do
     // nothing but invalidate the buffer.
-    State = InvalidateBuffer(C, State, DstBuffer, MemVal,
-                             /*IsSourceBuffer*/ false, Size);
+    State = invalidateDestinationBufferBySize(C, State, DstBuffer, MemVal,
+                                              SizeVal, Size->getType());
   }
   return true;
 }
 
 //===----------------------------------------------------------------------===//
 // evaluation of individual function calls.
 //===----------------------------------------------------------------------===//
 
if (stateNonZeroSize) {
     }
 
     // Invalidate the destination (regular invalidation without pointer-escaping
     // the address of the top-level region).
     // FIXME: Even if we can't perfectly model the copy, we should see if we
     // can use LazyCompoundVals to copy the source values into the destination.
     // This would probably remove any existing bindings past the end of the
     // copied region, but that's still an improvement over blank invalidation.
-    state =
-        InvalidateBuffer(C, state, Dest.Expression, C.getSVal(Dest.Expression),
-                         /*IsSourceBuffer*/ false, Size.Expression);
+    state = invalidateDestinationBufferBySize(
+        C, state, Dest.Expression, C.getSVal(Dest.Expression), sizeVal,
+        Size.Expression->getType());
 
     // Invalidate the source (const-invalidation without const-pointer-escaping
     // the address of the top-level region).
-    state = InvalidateBuffer(C, state, Source.Expression,
-                             C.getSVal(Source.Expression),
-                             /*IsSourceBuffer*/ true, nullptr);
+    state = invalidateSourceBuffer(C, state, Source.Expression,
+                                   C.getSVal(Source.Expression));
 
     C.addTransition(state);
   }
 }
 
 void CStringChecker::evalMemcpy(CheckerContext &C, const CallExpr *CE,
                                 CharKind CK) const {
   // void *memcpy(void *restrict dst, const void *restrict src, size_t n);
if (std::optional<loc::MemRegionVal> dstRegVal =
 
     // Invalidate the destination (regular invalidation without pointer-escaping
     // the address of the top-level region). This must happen before we set the
     // C string length because invalidation will clear the length.
     // FIXME: Even if we can't perfectly model the copy, we should see if we
     // can use LazyCompoundVals to copy the source values into the destination.
     // This would probably remove any existing bindings past the end of the
     // string, but that's still an improvement over blank invalidation.
-    state = InvalidateBuffer(C, state, Dst.Expression, *dstRegVal,
-                             /*IsSourceBuffer*/ false, nullptr);
+    state = invalidateDestinationBufferBySize(C, state, Dst.Expression,
+                                              *dstRegVal, amountCopied,
+                                              C.getASTContext().getSizeType());
 
     // Invalidate the source (const-invalidation without const-pointer-escaping
     // the address of the top-level region).
-    state = InvalidateBuffer(C, state, srcExpr.Expression, srcVal,
-                             /*IsSourceBuffer*/ true, nullptr);
+    state = invalidateSourceBuffer(C, state, srcExpr.Expression, srcVal);
 
     // Set the C string length of the destination, if we know it.
     if (IsBounded && (appendK == ConcatFnKind::none)) {
       // strncpy is annoying in that it doesn't guarantee to null-terminate
       // the result string. If the original string didn't fit entirely inside
       // the bound (including the null-terminator), we don't know how long the
       // result is.
       if (amountCopied != strLength)
void CStringChecker::evalStrsep(CheckerContext &C, const CallExpr *CE) const {
   SValBuilder &SVB = C.getSValBuilder();
   SVal Result;
   if (std::optional<Loc> SearchStrLoc = SearchStrVal.getAs<Loc>()) {
     // Get the current value of the search string pointer, as a char*.
     Result = State->getSVal(*SearchStrLoc, CharPtrTy);
 
     // Invalidate the search string, representing the change of one delimiter
     // character to NUL.
-    State = InvalidateBuffer(C, State, SearchStrPtr.Expression, Result,
-                             /*IsSourceBuffer*/ false, nullptr);
+    // As the replacement never overflows, do not invalidate its super region.
+    State = invalidateDestinationBufferNeverOverflows(
+        C, State, SearchStrPtr.Expression, Result);
 
     // Overwrite the search string pointer. The new value is either an address
     // further along in the same string, or NULL if there are no more tokens.
     State = State->bindLoc(*SearchStrLoc,
         SVB.conjureSymbolVal(getTag(),
           CE,
           LCtx,
           CharPtrTy,
void CStringChecker::evalStdCopyCommon(CheckerContext &C,
   // template <class _InputIterator, class _OutputIterator>
   // _OutputIterator
   // copy(_InputIterator __first, _InputIterator __last,
   //        _OutputIterator __result)
 
   // Invalidate the destination buffer
   const Expr *Dst = CE->getArg(2);
   SVal DstVal = State->getSVal(Dst, LCtx);
-  State = InvalidateBuffer(C, State, Dst, DstVal, /*IsSource=*/false,
-      /*Size=*/nullptr);
+  // FIXME: As we do not know how many items are copied, we also invalidate the
+  // super region containing the target location.
+  State =
+      invalidateDestinationBufferAlwaysEscapeSuperRegion(C, State, Dst, DstVal);
 
   SValBuilder &SVB = C.getSValBuilder();
 
   SVal ResultVal = SVB.conjureSymbolVal(nullptr, CE, LCtx, C.blockCount());
   State = State->BindExpr(CE, LCtx, ResultVal);
 
   C.addTransition(State);
 }

clang/test/Analysis/Inputs/system-header-simulator.h

 int feof(FILE *stream);
 int ferror(FILE *stream);
 int fileno(FILE *stream);
 
 size_t strlen(const char *);
 
 char *strcpy(char *restrict, const char *restrict);
 char *strncpy(char *dst, const char *src, size_t n);
+char *strsep(char **stringp, const char *delim);
 void *memcpy(void *dst, const void *src, size_t n);
+void *memset(void *s, int c, size_t n);
 
 typedef unsigned long __darwin_pthread_key_t;
 typedef __darwin_pthread_key_t pthread_key_t;
 int pthread_setspecific(pthread_key_t, const void *);
 
 int sqlite3_bind_text_my(int, const char*, int n, void(*)(void*));
 
 typedef void (*freeCallback) (void*);

clang/test/Analysis/issue-55019.cpp

+// Refer issue 55019 for more details.
+// A supplemental test case of pr22954.c for other functions modeled in
+// the CStringChecker.
+
+// RUN: %clang_analyze_cc1 %s -verify \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=unix \
+// RUN:   -analyzer-checker=debug.ExprInspection
+
+#include "Inputs/system-header-simulator.h"
+#include "Inputs/system-header-simulator-cxx.h"
+
+void *malloc(size_t);
+void free(void *);
+
+struct mystruct {
+  void *ptr;
+  char arr[4];
+};
+
+void clang_analyzer_dump(const void *);
+
+// CStringChecker::memsetAux
+void fmemset() {
+  mystruct x;
+  x.ptr = malloc(1);
+  clang_analyzer_dump(x.ptr); // expected-warning {{HeapSymRegion}}
+  memset(x.arr, 0, sizeof(x.arr));
+  clang_analyzer_dump(x.ptr); // expected-warning {{HeapSymRegion}}
+  free(x.ptr);                // no-leak-warning
+}
+
+// CStringChecker::evalCopyCommon
+void fmemcpy() {
+  mystruct x;
+  x.ptr = malloc(1);
+  clang_analyzer_dump(x.ptr); // expected-warning {{HeapSymRegion}}
+  memcpy(x.arr, "hi", 2);
+  clang_analyzer_dump(x.ptr); // expected-warning {{HeapSymRegion}}
+  free(x.ptr);                // no-leak-warning
+}
+
+// CStringChecker::evalStrcpyCommon
+void fstrcpy() {
+  mystruct x;
+  x.ptr = malloc(1);
+  clang_analyzer_dump(x.ptr); // expected-warning {{HeapSymRegion}}
+  strcpy(x.arr, "hi");
+  clang_analyzer_dump(x.ptr); // expected-warning {{HeapSymRegion}}
+  free(x.ptr);                // no-leak-warning
+}
+
+void fstrncpy() {
+  mystruct x;
+  x.ptr = malloc(1);
+  clang_analyzer_dump(x.ptr); // expected-warning {{HeapSymRegion}}
+  strncpy(x.arr, "hi", sizeof(x.arr));
+  clang_analyzer_dump(x.ptr); // expected-warning {{HeapSymRegion}}
+  free(x.ptr);                // no-leak-warning
+}
+
+// CStringChecker::evalStrsep
+void fstrsep() {
+  mystruct x;
+  x.ptr = malloc(1);
+  clang_analyzer_dump(x.ptr); // expected-warning {{HeapSymRegion}}
+  char *p = x.arr;
+  (void)strsep(&p, "x");
+  clang_analyzer_dump(x.ptr); // expected-warning {{HeapSymRegion}}
+  free(x.ptr);                // no-leak-warning
+}
+
+// CStringChecker::evalStdCopyCommon
+void fstdcopy() {
+  mystruct x;
+  x.ptr = new char;
+  clang_analyzer_dump(x.ptr); // expected-warning {{HeapSymRegion}}
+
+  const char *p = "x";
+  std::copy(p, p + 1, x.arr);
+
+  // FIXME: As we currently cannot know whether the copy overflows, the checker
+  // invalidates the entire `x` object. When the copy size through iterators
+  // can be correctly modeled, we can then update the verify direction from
+  // SymRegion to HeapSymRegion as this std::copy call never overflows and
+  // hence the pointer `x.ptr` shall not be invalidated.
+  clang_analyzer_dump(x.ptr);       // expected-warning {{SymRegion}}
+  delete static_cast<char*>(x.ptr); // no-leak-warning
+}

clang/test/Analysis/pr22954.c

struct xx {
   char * s2;
 };
 
 int f263(int n, char * len) {
   struct xx x263 = {0};
   x263.s2 = strdup("hello");
   char input[] = {'a', 'b', 'c', 'd'};
   memcpy(x263.s1, input, *(len + n));
-  clang_analyzer_eval(x263.s1[0] == 0); // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(x263.s1[0] == 0); // expected-warning{{UNKNOWN}}\
+  expected-warning{{Potential leak of memory pointed to by 'x263.s2'}}
   clang_analyzer_eval(x263.s1[1] == 0); // expected-warning{{UNKNOWN}}
   clang_analyzer_eval(x263.s1[2] == 0); // expected-warning{{UNKNOWN}}
   clang_analyzer_eval(x263.s1[3] == 0); // expected-warning{{UNKNOWN}}
   clang_analyzer_eval(x263.s2 == 0); // expected-warning{{UNKNOWN}}
-  return 0; // expected-warning{{Potential leak of memory pointed to by 'x263.s2'}}
+  return 0;
 }
 
 
 // Test casting regions with symbolic offseted sub regions.
 int f27(int i) {
   struct mm m27 = {{1, 2, 3, 4}, 0};
   m27.s4 = strdup("hello");
   m27.s3[i] = 5;
   char input[] = {'a', 'b', 'c', 'd'};
   memcpy(((struct ll*)(&m27))->s1, input, 4);
   clang_analyzer_eval(m27.s3[0] == 1); // expected-warning{{UNKNOWN}}
   clang_analyzer_eval(m27.s3[1] == 1); // expected-warning{{UNKNOWN}}
   clang_analyzer_eval(m27.s3[2] == 1); // expected-warning{{UNKNOWN}}
   clang_analyzer_eval(m27.s3[3] == 1); // expected-warning{{UNKNOWN}}
   clang_analyzer_eval(m27.s3[i] == 1); // expected-warning{{UNKNOWN}}\
   expected-warning{{Potential leak of memory pointed to by 'm27.s4'}}
   return 0;
 }
 
 int f28(int i, int j, int k, int l) {
   struct mm m28[2];
   m28[i].s4 = strdup("hello");
   m28[j].s3[k] = 1;
   struct ll * l28 = (struct ll*)(&m28[1]);