diff --git a/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp --- a/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp @@ -260,11 +260,24 @@ const Expr *expr, SVal val) const; + enum class InvalidationKind { + // Invalidate the source buffer for escaping pointers. + IK_SrcInvalidation, + + // Invalidate the destination buffer determined by characters copied. + IK_DstInvalidationBySize, + + // Operation never overflows, do not invalidate the super region. + IK_DstNeverOverflows, + + // We do not know whether the operation can overflow (e.g. size is unknown), + // invalidate the super region and escape related pointers. + IK_DstAlwaysEscapeSuperRegion, + }; static ProgramStateRef InvalidateBuffer(CheckerContext &C, - ProgramStateRef state, - const Expr *Ex, SVal V, - bool IsSourceBuffer, - const Expr *Size); + ProgramStateRef state, const Expr *Ex, + SVal V, InvalidationKind Kind, + SVal Size = UnknownVal()); static bool SummarizeRegion(raw_ostream &os, ASTContext &Ctx, const MemRegion *MR); @@ -310,10 +323,8 @@ // 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, + const Expr *FirstBuf, SVal Size); }; } //end anonymous namespace @@ -967,10 +978,8 @@ return strRegion->getStringLiteral(); } -bool CStringChecker::IsFirstBufInBound(CheckerContext &C, - ProgramStateRef state, - const Expr *FirstBuf, - const Expr *Size) { +bool CStringChecker::IsFirstBufInBound(CheckerContext &C, ProgramStateRef state, + const Expr *FirstBuf, SVal Size) { // 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. @@ -980,12 +989,11 @@ ASTContext &Ctx = svalBuilder.getContext(); const LocationContext *LCtx = C.getLocationContext(); - QualType sizeTy = Size->getType(); + QualType sizeTy = Size.getType(C.getASTContext()); QualType PtrTy = Ctx.getPointerType(Ctx.CharTy); SVal BufVal = state->getSVal(FirstBuf, LCtx); - SVal LengthVal = state->getSVal(Size, LCtx); - std::optional Length = LengthVal.getAs(); + std::optional Length = Size.getAs(); if (!Length) return true; // cf top comment. @@ -1034,8 +1042,8 @@ ProgramStateRef CStringChecker::InvalidateBuffer(CheckerContext &C, ProgramStateRef state, const Expr *E, SVal V, - bool IsSourceBuffer, - const Expr *Size) { + InvalidationKind Kind, + SVal Size) { std::optional L = V.getAs(); if (!L) return state; @@ -1060,21 +1068,23 @@ RegionAndSymbolInvalidationTraits ITraits; // Invalidate and escape only indirect regions accessible through the source // buffer. - if (IsSourceBuffer) { + if (InvalidationKind::IK_SrcInvalidation == Kind) { 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); - } + } else if (MemRegion::FieldRegionKind == R->getKind()) { + if (InvalidationKind::IK_DstAlwaysEscapeSuperRegion == Kind) { + CausesPointerEscape = true; + } else if (InvalidationKind::IK_DstNeverOverflows == Kind || + (InvalidationKind::IK_DstInvalidationBySize == Kind && + !Size.isUnknown() && IsFirstBufInBound(C, state, E, Size))) { + // If destination buffer is a field region and access is (always) in + // bound, do not invalidate its super region. + ITraits.setTrait( + R, + RegionAndSymbolInvalidationTraits::TK_DoNotInvalidateSuperRegion); + } } return state->invalidateRegions(R, E, C.blockCount(), LCtx, @@ -1182,8 +1192,9 @@ } 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 = + InvalidateBuffer(C, State, DstBuffer, MemVal, + InvalidationKind::IK_DstInvalidationBySize, SizeVal); } if (StateNullChar && !StateNonNullChar) { @@ -1208,8 +1219,9 @@ } 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 = + InvalidateBuffer(C, State, DstBuffer, MemVal, + InvalidationKind::IK_DstInvalidationBySize, SizeVal); } return true; } @@ -1307,13 +1319,13 @@ // 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); + InvalidationKind::IK_DstInvalidationBySize, sizeVal); // 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); + InvalidationKind::IK_SrcInvalidation); C.addTransition(state); } @@ -1986,12 +1998,13 @@ // 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); + InvalidationKind::IK_DstInvalidationBySize, + amountCopied); // 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); + InvalidationKind::IK_SrcInvalidation); // Set the C string length of the destination, if we know it. if (IsBounded && (appendK == ConcatFnKind::none)) { @@ -2206,8 +2219,9 @@ // Invalidate the search string, representing the change of one delimiter // character to NUL. + // As the replacement never overflows, do not invalidate its super region. State = InvalidateBuffer(C, State, SearchStrPtr.Expression, Result, - /*IsSourceBuffer*/ false, nullptr); + InvalidationKind::IK_DstNeverOverflows); // 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. @@ -2256,8 +2270,10 @@ // 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 = InvalidateBuffer(C, State, Dst, DstVal, + InvalidationKind::IK_DstAlwaysEscapeSuperRegion); SValBuilder &SVB = C.getSValBuilder(); diff --git a/clang/test/Analysis/Inputs/system-header-simulator.h b/clang/test/Analysis/Inputs/system-header-simulator.h --- a/clang/test/Analysis/Inputs/system-header-simulator.h +++ b/clang/test/Analysis/Inputs/system-header-simulator.h @@ -63,7 +63,9 @@ 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; diff --git a/clang/test/Analysis/issue-55019.c b/clang/test/Analysis/issue-55019.c new file mode 100644 --- /dev/null +++ b/clang/test/Analysis/issue-55019.c @@ -0,0 +1,69 @@ +// Refer issue 55019 for more details. + +// RUN: %clang_analyze_cc1 %s -verify -Wno-strict-prototypes \ +// RUN: -analyzer-checker=core \ +// RUN: -analyzer-checker=unix \ +// RUN: -analyzer-checker=debug.ExprInspection + +#include "Inputs/system-header-simulator.h" +void *malloc(size_t); +void free(void *); + +#define SIZE 4 + +struct mystruct { + void *ptr; + char arr[SIZE]; +}; + +void clang_analyzer_dump(const void *); + +// CStringChecker::memsetAux +void fmemset() { + struct mystruct x; + x.ptr = malloc(1); + clang_analyzer_dump(x.ptr); // expected-warning {{HeapSymRegion}} + memset(x.arr, 0, SIZE); + clang_analyzer_dump(x.ptr); // expected-warning {{HeapSymRegion}} + free(x.ptr); // no-warning +} + +// CStringChecker::evalCopyCommon +void fmemcpy() { + struct 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-warning +} + +// CStringChecker::evalStrcpyCommon +void fstrcpy() { + struct 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-warning +} + +void fstrncpy() { + struct mystruct x; + x.ptr = malloc(1); + clang_analyzer_dump(x.ptr); // expected-warning {{HeapSymRegion}} + strncpy(x.arr, "hi", SIZE); + clang_analyzer_dump(x.ptr); // expected-warning {{HeapSymRegion}} + free(x.ptr); // no-warning +} + +// CStringChecker::evalStrsep +void fstrsep() { + struct 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-warning +} diff --git a/clang/test/Analysis/issue-55019.cpp b/clang/test/Analysis/issue-55019.cpp new file mode 100644 --- /dev/null +++ b/clang/test/Analysis/issue-55019.cpp @@ -0,0 +1,29 @@ +// Refer issue 55019 for more details. + +// RUN: %clang_analyze_cc1 %s -verify \ +// RUN: -analyzer-checker=core,debug.ExprInspection + +#include "Inputs/system-header-simulator-cxx.h" + +struct mystruct { + char *ptr; + char arr[4]; +}; + +void clang_analyzer_dump(const void *); + +// 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 cannot know whether the copy overflows, we will invalidate the + // entire object. Modify the verify direction from SymRegion to HeapSymRegion + // when the size if modeled in CStringChecker. + clang_analyzer_dump(x.ptr); // expected-warning {{SymRegion}} + delete x.ptr; // no-warning +} diff --git a/clang/test/Analysis/pr22954.c b/clang/test/Analysis/pr22954.c --- a/clang/test/Analysis/pr22954.c +++ b/clang/test/Analysis/pr22954.c @@ -557,11 +557,12 @@ char input[] = {'a', 'b', 'c', 'd'}; memcpy(x263.s1, input, *(len + n)); clang_analyzer_eval(x263.s1[0] == 0); // expected-warning{{UNKNOWN}} + // expected-warning@-1{{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; }