[analyzer] Better modeling of memcpy by the CStringChecker (PR16731).
authorAnton Yartsev <anton.yartsev@gmail.com>
Sun, 17 Nov 2013 09:18:48 +0000 (09:18 +0000)
committerAnton Yartsev <anton.yartsev@gmail.com>
Sun, 17 Nov 2013 09:18:48 +0000 (09:18 +0000)
New rules of invalidation/escape of the source buffer of memcpy: the source buffer contents is invalidated and escape while the source buffer region itself is neither invalidated, nor escape.
In the current modeling of memcpy the information about allocation state of regions, accessible through the source buffer, is not copied to the destination buffer and we can not track the allocation state of those regions anymore. So we invalidate/escape the source buffer indirect regions in anticipation of their being invalidated for real later. This eliminates false-positive leaks reported by the unix.Malloc and alpha.cplusplus.NewDeleteLeaks checkers for the cases like

char *f() {
  void *x = malloc(47);
  char *a;
  memcpy(&a, &x, sizeof a);
  return a;
}

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@194953 91177308-0d34-0410-b5e6-96231b3b80d8

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 a037bd448a57d5b77cbd50a0075b08c85f6bae7b..cf7cf051d2255b3ae7afa8241abdcd7aa6eb0f3c 100644 (file)
@@ -337,7 +337,9 @@ class PointerEscape {
     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 @@ class ConstPointerEscape {
     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 7edafa86a6448b09a97a225fd9e4b9dec3119db9..a84dcb0df08d0756c49c5daf6e7561717a31be26 100644 (file)
@@ -1329,7 +1329,9 @@ public:
   /// \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 a223d1d803fb73dca84d60ea2a8adf0fb539b7a2..03739ed9284df93f09413f324cf60d547d3182bf 100644 (file)
@@ -232,21 +232,21 @@ public:
   /// \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 e642c2974ea1153bd3f76321de8b63b374cafbc9..c3736d7e5d71618c1b04a7859d95c4e8edb65e00 100644 (file)
@@ -141,8 +141,9 @@ public:
                                          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 @@ const StringLiteral *CStringChecker::getCStringLiteral(CheckerContext &C,
 }
 
 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,20 @@ ProgramStateRef CStringChecker::InvalidateBuffer(CheckerContext &C,
 
     // Invalidate this region.
     const LocationContext *LCtx = C.getPredecessor()->getLocationContext();
-    return state->invalidateRegions(R, E, C.blockCount(), LCtx,
-                                    /*CausesPointerEscape*/ false);
+
+    bool CausesPointerEscape = false;
+    RegionAndSymbolInvalidationTraits ITraits;
+    // Invalidate and escape only indirect regions accessible through the source
+    // buffer.
+    if (IsSourceBuffer) {
+      ITraits.setTrait(R, 
+                       RegionAndSymbolInvalidationTraits::TK_PreserveContents);
+      ITraits.setTrait(R, RegionAndSymbolInvalidationTraits::TK_SuppressEscape);
+      CausesPointerEscape = true;
+    }
+
+    return state->invalidateRegions(R, E, C.blockCount(), LCtx, 
+                                    CausesPointerEscape, 0, 0, &ITraits);
   }
 
   // If we have a non-region value by chance, just remove the binding.
@@ -968,13 +982,20 @@ void CStringChecker::evalCopyCommon(CheckerContext &C,
       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 +1598,19 @@ void CStringChecker::evalStrcpyCommon(CheckerContext &C, const CallExpr *CE,
         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 +1832,8 @@ void CStringChecker::evalStrsep(CheckerContext &C, const CallExpr *CE) const {
 
     // 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 ef9c1c7e51087ee0cda650614fc8214a4b6840a9..8b8c9c4fe17b6402d285852c838667560993ee65 100644 (file)
@@ -32,6 +32,7 @@ typedef __typeof(sizeof(int)) size_t;
 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 2e5213e374679c2f3fd7e62d93735b7ccff7540a..a0296cb0036dc5252e59efa954d62796fd32fddf 100644 (file)
@@ -627,8 +627,49 @@ void doNotInvalidateWhenPassedToSystemCalls(char *s) {
   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
+
+  void *p1 = malloc(7);
+  char *a;
+  memcpy(&a, &p1, sizeof a);
+  // FIXME: No warning due to limitations imposed by current modelling of
+  // 'memcpy' (regions metadata is not copied).
+
+  int *ptrs[2];
+  int *allocated = (int *)malloc(4);
+  memcpy(&ptrs[0], &allocated, sizeof(int *));
+  // FIXME: No warning due to limitations imposed by current modelling of
+  // 'memcpy' (regions metadata is not copied).
+}
+
+void invalidateDestinationContents() {
+  int *null = 0;
+  int *p = (int *)malloc(4);
+  memcpy(&p, &null, sizeof(int *));
+
+  int *ptrs1[2]; // expected-warning {{Potential leak of memory pointed to by}}
+  ptrs1[0] = (int *)malloc(4);
+  memcpy(ptrs1,  &null, sizeof(int *));
+
+  int *ptrs2[2]; // expected-warning {{Potential memory leak}}
+  ptrs2[0] = (int *)malloc(4);
+  memcpy(&ptrs2[1],  &null, sizeof(int *));
+
+  int *ptrs3[2]; // expected-warning {{Potential memory leak}}
+  ptrs3[0] = (int *)malloc(4);
+  memcpy(&ptrs3[0],  &null, sizeof(int *));
+} // expected-warning {{Potential memory leak}}
+
 // 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);