OikawaKirie created this revision.
OikawaKirie added reviewers: pgousseau, NoQ, steakhal, balazske, xazax.hun.
OikawaKirie added a project: clang.
Herald added subscribers: manas, ASDenysPetrov, martong, dkrupp, donat.nagy,
Szelethus, mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware.
Herald added a project: All.
OikawaKirie requested review of this revision.
Herald added a subscriber: cfe-commits.
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 flags have been introduced to inform CStringChecker::InvalidateBuffer
whether or not to
invalidate the super region that encompasses the destination buffer.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D152435
Files:
clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
clang/test/Analysis/Inputs/system-header-simulator.h
clang/test/Analysis/issue-55019.c
clang/test/Analysis/issue-55019.cpp
clang/test/Analysis/pr22954.c
Index: clang/test/Analysis/pr22954.c
===================================================================
--- clang/test/Analysis/pr22954.c
+++ 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;
}
Index: clang/test/Analysis/issue-55019.cpp
===================================================================
--- /dev/null
+++ 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
+}
Index: clang/test/Analysis/issue-55019.c
===================================================================
--- /dev/null
+++ 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
+}
Index: clang/test/Analysis/Inputs/system-header-simulator.h
===================================================================
--- clang/test/Analysis/Inputs/system-header-simulator.h
+++ 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;
Index: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
+++ 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<NonLoc> Length = LengthVal.getAs<NonLoc>();
+ std::optional<NonLoc> Length = Size.getAs<NonLoc>();
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<Loc> L = V.getAs<Loc>();
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();
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
