Patch for PR16558

[blitz.opensource]

Summary

Hi,
Please find the patch for PR16558. Please let me know your comments on the same.
Thanks!

[jordan_rose]

This looks quite reasonable. I have some general formatting notes, plus a request for more new tests -- in particular, one that tests the sort of wraparound that occurs in PR16558.

Thanks for working on this!

lib/StaticAnalyzer/Checkers/CStringChecker.cpp

673–680

It's probably cheaper to do this section entirely in APSInt-land:

llvm::APSInt fourInt = APSIntType(maxValInt).getValue(4);
const llvm::APSInt &maxLengthInt = BVF.evalAPSInt(BO_Div, maxValInt, fourInt);
NonLoc maxLength = svalBuilder.makeIntVal(maxLengthInt);

674

We tend not to put bug numbers inline in the source, though we do include them in tests sometimes. (Source is timeless, tests are not.)

681–688

It makes more sense to me to invert this, even though it's more nesting:

if (Optional<NonLoc> strLn = strLength.getAs<NonLoc>()) {
// ...
}

686–687

Style-wise we'd prefer to break within the argument list; if that doesn't work well we'd break after the equal sign, not the dot.

[blitz.opensource]

Implemented review comments from jordan.
Added test case to test wraparound issue mentioned in PR16558.

[jordan_rose]

Thanks, Karthik! I merged the PR16558 test into string.c so that we can keep invocations down, but otherwise this looks good. Committed with very minor changes in r188680.

lib/StaticAnalyzer/Checkers/CStringChecker.cpp

     if (Recorded)
       return *Recorded;
   }
-  
   // Otherwise, get a new symbol and update the state.
   SValBuilder &svalBuilder = C.getSValBuilder();
   QualType sizeTy = svalBuilder.getContext().getSizeType();

                                                     MR, Ex, sizeTy,
                                                     C.blockCount());
 
-  if (!hypothetical)
+  if (!hypothetical) {
+    // In case of unbounded calls strlen etc bound the range to SIZE_MAX/4
+    BasicValueFactory &BVF = svalBuilder.getBasicValueFactory();
+    const llvm::APSInt &maxValInt = BVF.getMaxValue(sizeTy);
+    llvm::APSInt fourInt = APSIntType(maxValInt).getValue(4);
+    const llvm::APSInt* maxLengthInt = BVF.evalAPSInt(BO_Div, maxValInt, fourInt);
+    NonLoc maxLength = svalBuilder.makeIntVal(*maxLengthInt);
+    if (Optional<NonLoc> strLn = strLength.getAs<NonLoc>()) {
+      SVal evalLength = svalBuilder.evalBinOpNN(state, BO_LE, *strLn,
+                                                maxLength, sizeTy);
+      state = state->assume(evalLength.castAs<DefinedOrUnknownSVal>(), true);
+    }
     state = state->set<CStringLength>(MR, strLength);
+  }
 
   return strLength;
 }

test/Analysis/pr16558.c

+//RUN: %clang_cc1 -analyze -analyzer-checker=core,unix -verify %s
+
+typedef typeof(sizeof(int)) size_t;
+extern void* malloc (size_t size);
+size_t strlen(const char *s);
+char *strcpy(char *restrict s1, const char *restrict s2);
+
+void *smallocNoWarn(size_t size) {
+  if (size == 0) {
+    return malloc(1); // this branch is never called
+  } 
+  else {
+    return malloc(size);
+  }
+}
+
+char *dupstrNoWarn(const char *s) {
+  const int len = strlen(s);
+  char *p = (char*) smallocNoWarn(len + 1);
+  strcpy(p, s); // no-warning
+  return p;
+}
+
+void *smallocWarn(size_t size) {
+  if (size == 2) {
+    return malloc(1);
+  }
+  else {
+    return malloc(size);
+  }
+}
+
+char *dupstrWarn(const char *s) {
+  const int len = strlen(s);
+  char *p = (char*) smallocWarn(len + 1);
+  strcpy(p, s); // expected-warning{{String copy function overflows destination buffer}}
+  return p;
+}

test/Analysis/string.c

 #else /* USE_BUILTINS */
 # define BUILTIN(f) f
 #endif /* USE_BUILTINS */
-
 #define NULL 0
 typedef typeof(sizeof(int)) size_t;
 

     return;
   if (strlen(src) != 2)
     return;
-  strcat(dst, src); // expected-warning{{This expression will create a string whose length is too big to be represented as a size_t}}
+  strcat(dst, src);
 }
 
 

     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}}
+  strncat(dst, src, 2);
 }
 
 void strncat_zero(char *src) {