ayartsev added you to the CC list for the revision "[analyzer][Review request]
Better modelling of memcpy by the CStringChecker (PR16731)".
Hi jordan_rose, zaks.anna,
The approach establishes the following rules of invalidation/escape for the
source and destination buffers passed to memcpy():
- source buffer is const-invalidated without const-pointer-escape the address
of the top-level region.
- destination buffer is regularly invalidated without pointer-escape the
address of the top-level region.
This rules are derived from the rules suggested by Jordan in the days prior to
r191342 when it was unable to associate processing behavior with the particular
region/symbol (see D1486 for detailes).
Here are rules from Jordan:
>> 1. Const-invalidate 'src', with pointer escape.
- invalidate metadata of indirect regions
- invalidate contents of indirect regions
- (unwanted) const-pointer-escape the address of the top-level region
>> 2. Const-invalidate 'dst', with pointer escape.
- invalidate the metadata of indirect regions (that "in anticipation" thing
that goes with our loss of knowledge)
- invalidate the contents of indirect regions (ditto)
- (unwanted) const-pointer-escape the address of the top-level region
>> 3. Regular-invalidate 'dst' without pointer escape.
- (harmlessly repeated) invalidate the contents of indirect regions
- invalidate the contents of the top-level region
http://llvm-reviews.chandlerc.com/D1887
Files:
include/clang/StaticAnalyzer/Core/Checker.h
include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
lib/StaticAnalyzer/Checkers/CStringChecker.cpp
test/Analysis/Inputs/system-header-simulator.h
test/Analysis/malloc.c
Index: include/clang/StaticAnalyzer/Core/Checker.h
===================================================================
--- include/clang/StaticAnalyzer/Core/Checker.h
+++ include/clang/StaticAnalyzer/Core/Checker.h
@@ -337,7 +337,9 @@
for (InvalidatedSymbols::const_iterator I = Escaped.begin(),
E = Escaped.end(); I != E; ++I)
if (!ETraits->hasTrait(*I,
- RegionAndSymbolInvalidationTraits::TK_PreserveContents))
+ RegionAndSymbolInvalidationTraits::TK_PreserveContents) &&
+ !ETraits->hasTrait(*I,
+ RegionAndSymbolInvalidationTraits::TK_SuppressEscape))
RegularEscape.insert(*I);
if (RegularEscape.empty())
@@ -375,7 +377,9 @@
for (InvalidatedSymbols::const_iterator I = Escaped.begin(),
E = Escaped.end(); I != E; ++I)
if (ETraits->hasTrait(*I,
- RegionAndSymbolInvalidationTraits::TK_PreserveContents))
+ RegionAndSymbolInvalidationTraits::TK_PreserveContents) &&
+ !ETraits->hasTrait(*I,
+ RegionAndSymbolInvalidationTraits::TK_SuppressEscape))
ConstEscape.insert(*I);
if (ConstEscape.empty())
Index: include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
===================================================================
--- include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
+++ include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
@@ -1329,7 +1329,9 @@
/// \brief Describes different invalidation traits.
enum InvalidationKinds {
/// Tells that a region's contents is not changed.
- TK_PreserveContents = 0x1
+ TK_PreserveContents = 0x1,
+ /// Suppress pointer-escaping of a region.
+ TK_SuppressEscape = 0x2
// Do not forget to extend StorageTypeForKinds if number of traits exceed
// the number of bits StorageTypeForKinds can store.
Index: include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
===================================================================
--- include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
+++ include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
@@ -232,21 +232,21 @@
/// \param IS the set of invalidated symbols.
/// \param Call if non-null, the invalidated regions represent parameters to
/// the call and should be considered directly invalidated.
- /// \param HTraits information about special handling for a particular
+ /// \param ITraits information about special handling for a particular
/// region/symbol.
ProgramStateRef
invalidateRegions(ArrayRef<const MemRegion *> Regions, const Expr *E,
unsigned BlockCount, const LocationContext *LCtx,
bool CausesPointerEscape, InvalidatedSymbols *IS = 0,
const CallEvent *Call = 0,
- RegionAndSymbolInvalidationTraits *HTraits = 0) const;
+ RegionAndSymbolInvalidationTraits *ITraits = 0) const;
ProgramStateRef
invalidateRegions(ArrayRef<SVal> Regions, const Expr *E,
unsigned BlockCount, const LocationContext *LCtx,
bool CausesPointerEscape, InvalidatedSymbols *IS = 0,
const CallEvent *Call = 0,
- RegionAndSymbolInvalidationTraits *HTraits = 0) const;
+ RegionAndSymbolInvalidationTraits *ITraits = 0) const;
/// enterStackFrame - Returns the state for entry to the given stack frame,
/// preserving the current state.
Index: lib/StaticAnalyzer/Checkers/CStringChecker.cpp
===================================================================
--- lib/StaticAnalyzer/Checkers/CStringChecker.cpp
+++ lib/StaticAnalyzer/Checkers/CStringChecker.cpp
@@ -141,8 +141,9 @@
SVal val) const;
static ProgramStateRef InvalidateBuffer(CheckerContext &C,
- ProgramStateRef state,
- const Expr *Ex, SVal V);
+ ProgramStateRef state,
+ const Expr *Ex, SVal V,
+ bool IsSourceBuffer);
static bool SummarizeRegion(raw_ostream &os, ASTContext &Ctx,
const MemRegion *MR);
@@ -809,8 +810,9 @@
}
ProgramStateRef CStringChecker::InvalidateBuffer(CheckerContext &C,
- ProgramStateRef state,
- const Expr *E, SVal V) {
+ ProgramStateRef state,
+ const Expr *E, SVal V,
+ bool IsSourceBuffer) {
Optional<Loc> L = V.getAs<Loc>();
if (!L)
return state;
@@ -830,8 +832,19 @@
// Invalidate this region.
const LocationContext *LCtx = C.getPredecessor()->getLocationContext();
- return state->invalidateRegions(R, E, C.blockCount(), LCtx,
- /*CausesPointerEscape*/ false);
+
+ RegionAndSymbolInvalidationTraits ITraits;
+ // Neither source nor destination buffer region itself escapes, only buffer
+ // contents.
+ ITraits.setTrait(R, RegionAndSymbolInvalidationTraits::TK_SuppressEscape);
+
+ // Invalidate only indirect regions accessible through the source buffer.
+ if (IsSourceBuffer)
+ ITraits.setTrait(R,
+ RegionAndSymbolInvalidationTraits::TK_PreserveContents);
+
+ return state->invalidateRegions(R, E, C.blockCount(), LCtx, true, 0, 0,
+ &ITraits);
}
// If we have a non-region value by chance, just remove the binding.
@@ -968,13 +981,20 @@
state = state->BindExpr(CE, LCtx, destVal);
}
- // Invalidate the destination.
+ // 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,
- state->getSVal(Dest, C.getLocationContext()));
+ state = InvalidateBuffer(C, state, Dest, C.getSVal(Dest),
+ /*IsSourceBuffer*/false);
+
+ // Invalidate the source (const-invalidation without const-pointer-escaping
+ // the address of the top-level region).
+ state = InvalidateBuffer(C, state, Source, C.getSVal(Source),
+ /*IsSourceBuffer*/true);
+
C.addTransition(state);
}
}
@@ -1577,13 +1597,19 @@
Result = lastElement;
}
- // Invalidate the destination. This must happen before we set the C string
- // length because invalidation will clear the length.
+ // 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, *dstRegVal);
+ state = InvalidateBuffer(C, state, Dst, *dstRegVal,
+ /*IsSourceBuffer*/false);
+
+ // Invalidate the source (const-invalidation without const-pointer-escaping
+ // the address of the top-level region).
+ state = InvalidateBuffer(C, state, srcExpr, srcVal, /*IsSourceBuffer*/true);
// Set the C string length of the destination, if we know it.
if (isBounded && !isAppending) {
@@ -1805,7 +1831,8 @@
// Invalidate the search string, representing the change of one delimiter
// character to NUL.
- State = InvalidateBuffer(C, State, SearchStrPtr, Result);
+ State = InvalidateBuffer(C, State, SearchStrPtr, Result,
+ /*IsSourceBuffer*/false);
// 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.
Index: test/Analysis/Inputs/system-header-simulator.h
===================================================================
--- test/Analysis/Inputs/system-header-simulator.h
+++ test/Analysis/Inputs/system-header-simulator.h
@@ -32,6 +32,7 @@
size_t strlen(const char *);
char *strcpy(char *restrict, const char *restrict);
+void *memcpy(void *dst, const void *src, size_t n);
typedef unsigned long __darwin_pthread_key_t;
typedef __darwin_pthread_key_t pthread_key_t;
Index: test/Analysis/malloc.c
===================================================================
--- test/Analysis/malloc.c
+++ test/Analysis/malloc.c
@@ -627,8 +627,19 @@
char *p = malloc(12);
strlen(p);
strcpy(p, s);
+ strcpy(s, p);
+ strcpy(p, p);
+ memcpy(p, s, 1);
+ memcpy(s, p, 1);
+ memcpy(p, p, 1);
} // expected-warning {{leak}}
+// Treat source buffer contents as escaped.
+void escapeSourceContents(char *s) {
+ char *p = malloc(12);
+ memcpy(s, &p, 12); // no warning
+}
+
// Rely on the CString checker evaluation of the strcpy API to convey that the result of strcpy is equal to p.
void symbolLostWithStrcpy(char *s) {
char *p = malloc(12);
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
