[analyzer] Finish size argument checking for strncat (and strncpy).

llvm-svn: 133472

clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp

@@ -1217,8 +1217,14 @@ void CStringChecker::evalStrcpyCommon(CheckerContext &C, const CallExpr *CE,
 
   SValBuilder &svalBuilder = C.getSValBuilder();
   QualType cmpTy = svalBuilder.getConditionType();
+  QualType sizeTy = svalBuilder.getContext().getSizeType();
 
+  // These two values allow checking two kinds of errors:
+  // - actual overflows caused by a source that doesn't fit in the destination
+  // - potential overflows caused by a bound that could exceed the destination
   SVal amountCopied = UnknownVal();
+  SVal maxLastElementIndex = UnknownVal();
+  const char *boundWarning = NULL;
 
   // If the function is strncpy, strncat, etc... it is bounded.
   if (isBounded) {
@@ -1227,9 +1233,7 @@ void CStringChecker::evalStrcpyCommon(CheckerContext &C, const CallExpr *CE,
     SVal lenVal = state->getSVal(lenExpr);
 
     // Protect against misdeclared strncpy().
-    lenVal = svalBuilder.evalCast(lenVal,
+    lenVal = svalBuilder.evalCast(lenVal, sizeTy, lenExpr->getType());
-                                  svalBuilder.getContext().getSizeType(),
-                                  lenExpr->getType());
 
     NonLoc *strLengthNL = dyn_cast<NonLoc>(&strLength);
     NonLoc *lenValNL = dyn_cast<NonLoc>(&lenVal);
@@ -1240,9 +1244,11 @@ void CStringChecker::evalStrcpyCommon(CheckerContext &C, const CallExpr *CE,
       const GRState *stateSourceTooLong, *stateSourceNotTooLong;
 
       // Check if the max number to copy is less than the length of the src.
+      // If the bound is equal to the source length, strncpy won't null-
+      // terminate the result!
       llvm::tie(stateSourceTooLong, stateSourceNotTooLong) =
         state->assume(cast<DefinedOrUnknownSVal>
-                      (svalBuilder.evalBinOpNN(state, BO_GT, *strLengthNL,
+                      (svalBuilder.evalBinOpNN(state, BO_GE, *strLengthNL,
                                                *lenValNL, cmpTy)));
 
       if (stateSourceTooLong && !stateSourceNotTooLong) {
@@ -1259,19 +1265,43 @@ 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 =
+    if (lenValNL) {
-      "Size argument is greater than the length of the destination buffer";
+      if (isAppending) {
-    state = CheckBufferAccess(C, state, lenExpr, Dst, warningMsg,
+        // For strncat, the check is strlen(dst) + lenVal < sizeof(dst)
-                              /* WarnAboutSize = */ true);
+
-    // FIXME: It'd be nice for this not to be a hard error, so we can warn
+        // Get the string length of the destination. If the destination is
-    // about more than one thing, but the multiple calls to CheckLocation
+        // memory that can't have a string length, we shouldn't be copying
-    // result in the same ExplodedNode, which means we don't keep emitting
+        // into it anyway.
-    // bug reports.
+        SVal dstStrLength = getCStringLength(C, state, Dst, DstVal);
-    if (!state)
+        if (dstStrLength.isUndef())
-      return;
+          return;
+
+        if (NonLoc *dstStrLengthNL = dyn_cast<NonLoc>(&dstStrLength)) {
+          maxLastElementIndex = svalBuilder.evalBinOpNN(state, BO_Add,
+                                                        *lenValNL,
+                                                        *dstStrLengthNL,
+                                                        sizeTy);
+          boundWarning = "Size argument is greater than the free space in the "
+                         "destination buffer";
+        }
+
+      } else {
+        // For strncpy, this is just checking that lenVal <= sizeof(dst)
+        // (Yes, strncpy and strncat differ in how they treat termination.
+        // strncat ALWAYS terminates, but strncpy doesn't.)
+        NonLoc one = cast<NonLoc>(svalBuilder.makeIntVal(1, sizeTy));
+        maxLastElementIndex = svalBuilder.evalBinOpNN(state, BO_Sub, *lenValNL,
+                                                      one, sizeTy);
+        boundWarning = "Size argument is greater than the length of the "
+                       "destination buffer";
+      }
+    }
 
     // If we couldn't pin down the copy length, at least bound it.
-    if (amountCopied.isUnknown()) {
+    // FIXME: We should actually run this code path for append as well, but
+    // right now it creates problems with constraints (since we can end up
+    // trying to pass constraints from symbol to symbol).
+    if (amountCopied.isUnknown() && !isAppending) {
       // Try to get a "hypothetical" string length symbol, which we can later
       // set as a real value if that turns out to be the case.
       amountCopied = getCStringLength(C, state, lenExpr, srcVal, true);
@@ -1327,8 +1357,6 @@ void CStringChecker::evalStrcpyCommon(CheckerContext &C, const CallExpr *CE,
     if (dstStrLength.isUndef())
       return;
 
-    QualType sizeTy = svalBuilder.getContext().getSizeType();
-
     NonLoc *srcStrLengthNL = dyn_cast<NonLoc>(&amountCopied);
     NonLoc *dstStrLengthNL = dyn_cast<NonLoc>(&dstStrLength);
     
@@ -1393,17 +1421,34 @@ void CStringChecker::evalStrcpyCommon(CheckerContext &C, const CallExpr *CE,
   // If the destination is a MemRegion, try to check for a buffer overflow and
   // record the new string length.
   if (loc::MemRegionVal *dstRegVal = dyn_cast<loc::MemRegionVal>(&DstVal)) {
-    // If the final length is known, we can check for an overflow.
+    QualType ptrTy = Dst->getType();
+
+    // If we have an exact value on a bounded copy, use that to check for
+    // overflows, rather than our estimate about how much is actually copied.
+    if (boundWarning) {
+      if (NonLoc *maxLastNL = dyn_cast<NonLoc>(&maxLastElementIndex)) {
+        SVal maxLastElement = svalBuilder.evalBinOpLN(state, BO_Add, *dstRegVal,
+                                                      *maxLastNL, ptrTy);
+        state = CheckLocation(C, state, CE->getArg(2), maxLastElement, 
+                              boundWarning);
+        if (!state)
+          return;
+      }
+    }
+
+    // Then, if the final length is known...
     if (NonLoc *knownStrLength = dyn_cast<NonLoc>(&finalStrLength)) {
       SVal lastElement = svalBuilder.evalBinOpLN(state, BO_Add, *dstRegVal,
-                                                 *knownStrLength,
+                                                 *knownStrLength, ptrTy);
-                                                 Dst->getType());
 
-      const char * const warningMsg =
+      // ...and we haven't checked the bound, we'll check the actual copy.
-        "String copy function overflows destination buffer";
+      if (!boundWarning) {
-      state = CheckLocation(C, state, Dst, lastElement, warningMsg);
+        const char * const warningMsg =
-      if (!state)
+          "String copy function overflows destination buffer";
-        return;
+        state = CheckLocation(C, state, Dst, lastElement, warningMsg);
+        if (!state)
+          return;
+      }
 
       // If this is a stpcpy-style copy, the last element is the return value.
       if (returnEnd)
@@ -1419,10 +1464,15 @@ void CStringChecker::evalStrcpyCommon(CheckerContext &C, const CallExpr *CE,
     state = InvalidateBuffer(C, state, Dst, *dstRegVal);
 
     // Set the C string length of the destination, if we know it.
-    if (!isBounded || (amountCopied == strLength))
+    if (isBounded && !isAppending) {
-      state = setCStringLength(state, dstRegVal->getRegion(), finalStrLength);
+      // strncpy is annoying in that it doesn't guarantee to null-terminate
-    else
+      // the result string. If the original string didn't fit entirely inside
-      state = setCStringLength(state, dstRegVal->getRegion(), UnknownVal());
+      // the bound (including the null-terminator), we don't know how long the
+      // result is.
+      if (amountCopied != strLength)
+        finalStrLength = UnknownVal();
+    }
+    state = setCStringLength(state, dstRegVal->getRegion(), finalStrLength);
   }
 
   assert(state);

clang/test/Analysis/string.c

@@ -604,25 +604,25 @@ void strncat_effects(char *y) {
 void strncat_overflow_0(char *y) {
   char x[4] = "12";
   if (strlen(y) == 4)
-    strncat(x, y, strlen(y)); // expected-warning{{String copy function overflows destination buffer}}
+    strncat(x, y, strlen(y)); // expected-warning{{Size argument is greater than the free space in the destination buffer}}
 }
 
 void strncat_overflow_1(char *y) {
   char x[4] = "12";
   if (strlen(y) == 3)
-    strncat(x, y, strlen(y)); // expected-warning{{String copy function overflows destination buffer}}
+    strncat(x, y, strlen(y)); // expected-warning{{Size argument is greater than the free space in the destination buffer}}
 }
 
 void strncat_overflow_2(char *y) {
   char x[4] = "12";
   if (strlen(y) == 2)
-    strncat(x, y, strlen(y)); // expected-warning{{String copy function overflows destination buffer}}
+    strncat(x, y, strlen(y)); // expected-warning{{Size argument is greater than the free space in the destination buffer}}
 }
 
 void strncat_overflow_3(char *y) {
   char x[4] = "12";
   if (strlen(y) == 4)
-    strncat(x, y, 2); // expected-warning{{String copy function overflows destination buffer}}
+    strncat(x, y, 2); // expected-warning{{Size argument is greater than the free space in the destination buffer}}
 }
 void strncat_no_overflow_1(char *y) {
   char x[5] = "12";
@@ -636,6 +636,63 @@ void strncat_no_overflow_2(char *y) {
     strncat(x, y, 1); // no-warning
 }
 
+void strncat_symbolic_dst_length(char *dst) {
+  strncat(dst, "1234", 5);
+  if (strlen(dst) < 4)
+    (void)*(char*)0; // no-warning
+}
+
+void strncat_symbolic_src_length(char *src) {
+  char dst[8] = "1234";
+  strncat(dst, src, 3);
+  if (strlen(dst) < 4)
+    (void)*(char*)0; // no-warning
+
+  char dst2[8] = "1234";
+  strncat(dst2, src, 4); // expected-warning{{Size argument is greater than the free space in the destination buffer}}
+}
+
+void strncat_unknown_src_length(char *src, int offset) {
+  char dst[8] = "1234";
+  strncat(dst, &src[offset], 3);
+  if (strlen(dst) < 4)
+    (void)*(char*)0; // no-warning
+
+  char dst2[8] = "1234";
+  strncat(dst2, &src[offset], 4); // expected-warning{{Size argument is greater than the free space in the destination buffer}}
+}
+
+// There is no strncat_unknown_dst_length because if we can't get a symbolic
+// length for the "before" strlen, we won't be able to set one for "after".
+
+void strncat_symbolic_limit(unsigned limit) {
+  char dst[6] = "1234";
+  char src[] = "567";
+  strncat(dst, src, limit); // no-warning
+  if (strlen(dst) < 4)
+    (void)*(char*)0; // no-warning
+  if (strlen(dst) == 4)
+    (void)*(char*)0; // expected-warning{{null}}
+}
+
+void strncat_unknown_limit(float limit) {
+  char dst[6] = "1234";
+  char src[] = "567";
+  strncat(dst, src, (size_t)limit); // no-warning
+  if (strlen(dst) < 4)
+    (void)*(char*)0; // no-warning
+  if (strlen(dst) == 4)
+    (void)*(char*)0; // expected-warning{{null}}
+}
+
+void strncat_too_big(char *dst, char *src) {
+  if (strlen(dst) != (((size_t)0) - 2))
+    return;
+  if (strlen(src) != 2)
+    return;
+  strncat(dst, src, 2); // expected-warning{{This expression will create a string whose length is too big to be represented as a size_t}}
+}
+
 //===----------------------------------------------------------------------===
 // strcmp()
 //===----------------------------------------------------------------------===