[analyzer] Re-enable checking for strncpy, along with a new validation of the size...

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

diff --git a/lib/StaticAnalyzer/Checkers/CStringChecker.cpp b/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
index 9b6bcc8b88fc7aa5b0aa2c547507013ae668d3cb..82f2855d17ff3e28ccc0efc509d9f4535e7c8413 100644 (file)
--- a/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
+++ b/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
@@ -123,12 +123,15 @@ public:
                                    const Expr *FirstBuf,
                                    const Expr *SecondBuf,
                                    const char *firstMessage = NULL,
-                                   const char *secondMessage = NULL) const;
+                                   const char *secondMessage = NULL,
+                                   bool WarnAboutSize = false) const;
   const GRState *CheckBufferAccess(CheckerContext &C, const GRState *state,
                                    const Expr *Size, const Expr *Buf,
-                                   const char *message = NULL) const {
+                                   const char *message = NULL,
+                                   bool WarnAboutSize = false) const {
     // This is a convenience override.
-    return CheckBufferAccess(C, state, Size, Buf, NULL, message, NULL);
+    return CheckBufferAccess(C, state, Size, Buf, NULL, message, NULL,
+                             WarnAboutSize);
   }
   const GRState *CheckOverlap(CheckerContext &C, const GRState *state,
                               const Expr *Size, const Expr *First,
@@ -290,7 +293,8 @@ const GRState *CStringChecker::CheckBufferAccess(CheckerContext &C,
                                                  const Expr *FirstBuf,
                                                  const Expr *SecondBuf,
                                                  const char *firstMessage,
-                                              const char *secondMessage) const {
+                                                 const char *secondMessage,
+                                                 bool WarnAboutSize) const {
   // If a previous check has failed, propagate the failure.
   if (!state)
     return NULL;
@@ -323,9 +327,11 @@ const GRState *CStringChecker::CheckBufferAccess(CheckerContext &C,
   // Check that the first buffer is sufficiently long.
   SVal BufStart = svalBuilder.evalCast(BufVal, PtrTy, FirstBuf->getType());
   if (Loc *BufLoc = dyn_cast<Loc>(&BufStart)) {
+    const Expr *warningExpr = (WarnAboutSize ? Size : FirstBuf);
+
     SVal BufEnd = svalBuilder.evalBinOpLN(state, BO_Add, *BufLoc,
                                           LastOffset, PtrTy);
-    state = CheckLocation(C, state, FirstBuf, BufEnd, firstMessage);
+    state = CheckLocation(C, state, warningExpr, BufEnd, firstMessage);
 
     // If the buffer isn't large enough, abort.
     if (!state)
@@ -341,9 +347,11 @@ const GRState *CStringChecker::CheckBufferAccess(CheckerContext &C,
 
     BufStart = svalBuilder.evalCast(BufVal, PtrTy, SecondBuf->getType());
     if (Loc *BufLoc = dyn_cast<Loc>(&BufStart)) {
+      const Expr *warningExpr = (WarnAboutSize ? Size : SecondBuf);
+
       SVal BufEnd = svalBuilder.evalBinOpLN(state, BO_Add, *BufLoc,
                                             LastOffset, PtrTy);
-      state = CheckLocation(C, state, SecondBuf, BufEnd, secondMessage);
+      state = CheckLocation(C, state, warningExpr, BufEnd, secondMessage);
     }
   }
 
@@ -1251,6 +1259,18 @@ void CStringChecker::evalStrcpyCommon(CheckerContext &C, const CallExpr *CE,
       }
     }
 
+    // We still want to know if the bound is known to be too large.
+    const char * const warningMsg =
+      "Size argument is greater than the length of the destination buffer";
+    state = CheckBufferAccess(C, state, lenExpr, Dst, warningMsg,
+                              /* WarnAboutSize = */ true);
+    // FIXME: It'd be nice for this not to be a hard error, so we can warn
+    // about more than one thing, but the multiple calls to CheckLocation
+    // result in the same ExplodedNode, which means we don't keep emitting
+    // bug reports.
+    if (!state)
+      return;
+
     // If we couldn't pin down the copy length, at least bound it.
     if (amountCopied.isUnknown()) {
       // Try to get a "hypothetical" string length symbol, which we can later
@@ -1399,8 +1419,11 @@ void CStringChecker::evalStrcpyCommon(CheckerContext &C, const CallExpr *CE,
     // string, but that's still an improvement over blank invalidation.
     state = InvalidateBuffer(C, state, Dst, *dstRegVal);
 
-    // Set the C string length of the destination.
-    state = setCStringLength(state, dstRegVal->getRegion(), finalStrLength);
+    // Set the C string length of the destination, if we know it.
+    if (!isBounded || (amountCopied == strLength))
+      state = setCStringLength(state, dstRegVal->getRegion(), finalStrLength);
+    else
+      state = setCStringLength(state, dstRegVal->getRegion(), UnknownVal());
   }
 
   assert(state);
@@ -1589,7 +1612,7 @@ bool CStringChecker::evalCall(const CallExpr *CE, CheckerContext &C) const {
     .Cases("memcmp", "bcmp", &CStringChecker::evalMemcmp)
     .Cases("memmove", "__memmove_chk", &CStringChecker::evalMemmove)
     .Cases("strcpy", "__strcpy_chk", &CStringChecker::evalStrcpy)
-    //.Cases("strncpy", "__strncpy_chk", &CStringChecker::evalStrncpy)
+    .Cases("strncpy", "__strncpy_chk", &CStringChecker::evalStrncpy)
     .Cases("stpcpy", "__stpcpy_chk", &CStringChecker::evalStpcpy)
     .Cases("strcat", "__strcat_chk", &CStringChecker::evalStrcat)
     .Cases("strncat", "__strncat_chk", &CStringChecker::evalStrncat)

diff --git a/test/Analysis/string.c b/test/Analysis/string.c
index c50eca2f35e8f08e6b91d609c018dd48e9896741..c755d5ef120bc6dba549d64fc003419e801b676d 100644 (file)
--- a/test/Analysis/string.c
+++ b/test/Analysis/string.c
@@ -456,6 +456,105 @@ void strcat_too_big(char *dst, char *src) {
 }
 
 
+//===----------------------------------------------------------------------===
+// strncpy()
+//===----------------------------------------------------------------------===
+
+#ifdef VARIANT
+
+#define __strncpy_chk BUILTIN(__strncpy_chk)
+char *__strncpy_chk(char *restrict s1, const char *restrict s2, size_t n, size_t destlen);
+
+#define strncpy(a,b,n) __strncpy_chk(a,b,n,(size_t)-1)
+
+#else /* VARIANT */
+
+#define strncpy BUILTIN(strncpy)
+char *strncpy(char *restrict s1, const char *restrict s2, size_t n);
+
+#endif /* VARIANT */
+
+
+void strncpy_null_dst(char *x) {
+  strncpy(NULL, x, 5); // expected-warning{{Null pointer argument in call to string copy function}}
+}
+
+void strncpy_null_src(char *x) {
+  strncpy(x, NULL, 5); // expected-warning{{Null pointer argument in call to string copy function}}
+}
+
+void strncpy_fn(char *x) {
+  strncpy(x, (char*)&strcpy_fn, 5); // expected-warning{{Argument to string copy function is the address of the function 'strcpy_fn', which is not a null-terminated string}}
+}
+
+void strncpy_effects(char *x, char *y) {
+  char a = x[0];
+
+  if (strncpy(x, y, 5) != x)
+    (void)*(char*)0; // no-warning
+
+  if (strlen(x) != strlen(y))
+    (void)*(char*)0; // expected-warning{{null}}
+
+  if (a != x[0])
+    (void)*(char*)0; // expected-warning{{null}}
+}
+
+void strncpy_overflow(char *y) {
+  char x[4];
+  if (strlen(y) == 4)
+    strncpy(x, y, 5); // expected-warning{{Size argument is greater than the length of the destination buffer}}
+}
+
+void strncpy_no_overflow(char *y) {
+  char x[4];
+  if (strlen(y) == 3)
+    strncpy(x, y, 5); // expected-warning{{Size argument is greater than the length of the destination buffer}}
+}
+
+void strncpy_no_overflow2(char *y, int n) {
+       if (n <= 4)
+               return;
+
+  char x[4];
+  if (strlen(y) == 3)
+    strncpy(x, y, n); // expected-warning{{Size argument is greater than the length of the destination buffer}}
+}
+
+void strncpy_truncate(char *y) {
+  char x[4];
+  if (strlen(y) == 4)
+    strncpy(x, y, 3); // no-warning
+}
+
+void strncpy_no_truncate(char *y) {
+  char x[4];
+  if (strlen(y) == 3)
+    strncpy(x, y, 3); // no-warning
+}
+
+void strncpy_exactly_matching_buffer(char *y) {
+       char x[4];
+       strncpy(x, y, 4); // no-warning
+
+       // strncpy does not null-terminate, so we have no idea what the strlen is
+       // after this.
+       if (strlen(x) > 4)
+               (void)*(int*)0; // expected-warning{{null}}
+}
+
+void strncpy_exactly_matching_buffer2(char *y) {
+       if (strlen(y) >= 4)
+               return;
+
+       char x[4];
+       strncpy(x, y, 4); // no-warning
+
+       // This time, we know that y fits in x anyway.
+       if (strlen(x) > 3)
+               (void)*(int*)0; // no-warning
+}
+
 //===----------------------------------------------------------------------===
 // strncat()
 //===----------------------------------------------------------------------===