[analyzer] Assume that strings are no longer than SIZE_MAX/4.

This keeps the analyzer from making silly assumptions, like thinking
strlen(foo)+1 could wrap around to 0. This fixes PR16558.

Patch by Karthik Bhat!

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

diff --git a/lib/StaticAnalyzer/Checkers/CStringChecker.cpp b/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
index 932f6316b555919694c069f237dd9b965809d81b..ba1d9b9ff6bd516fa4b4049d7e0b00a1fffc8ec7 100644 (file)
--- a/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
+++ b/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
@@ -661,7 +661,7 @@ SVal CStringChecker::getCStringLengthForRegion(CheckerContext &C,
     if (Recorded)
       return *Recorded;
   }
-  
+
   // Otherwise, get a new symbol and update the state.
   SValBuilder &svalBuilder = C.getSValBuilder();
   QualType sizeTy = svalBuilder.getContext().getSizeType();
@@ -669,8 +669,21 @@ SVal CStringChecker::getCStringLengthForRegion(CheckerContext &C,
                                                     MR, Ex, sizeTy,
                                                     C.blockCount());
 
-  if (!hypothetical)
+  if (!hypothetical) {
+    if (Optional<NonLoc> strLn = strLength.getAs<NonLoc>()) {
+      // 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);
+      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;
 }

diff --git a/test/Analysis/malloc.c b/test/Analysis/malloc.c
index 83946c83a889dbe3bc9282c326c1b7bcb9c4976e..2e5213e374679c2f3fd7e62d93735b7ccff7540a 100644 (file)
--- a/test/Analysis/malloc.c
+++ b/test/Analysis/malloc.c
@@ -1216,6 +1216,38 @@ void testReallocEscaped(void **memory) {
   }
 }
 
+// PR16558
+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;
+}
 
 // ----------------------------------------------------------------------------
 // False negatives.

diff --git a/test/Analysis/string.c b/test/Analysis/string.c
index 6cf52f7a557b2fde0144d3f5517356e62ac12cd2..9fd3efb5c2d773a16c00e9a3c0e84ab15d15f0a7 100644 (file)
--- a/test/Analysis/string.c
+++ b/test/Analysis/string.c
@@ -430,11 +430,12 @@ void strcat_unknown_src_length(char *src, int offset) {
 // length for the "before" strlen, we won't be able to set one for "after".
 
 void strcat_too_big(char *dst, char *src) {
+  // We assume this can never actually happen, so we don't get a warning.
        if (strlen(dst) != (((size_t)0) - 2))
                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);
 }
 
 
@@ -653,11 +654,12 @@ void strncat_unknown_limit(float limit) {
 }
 
 void strncat_too_big(char *dst, char *src) {
+  // We assume this will never actually happen, so we don't get a warning.
   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}}
+  strncat(dst, src, 2);
 }
 
 void strncat_zero(char *src) {

diff --git a/www/analyzer/open_projects.html b/www/analyzer/open_projects.html
index a5f5538662faecb27c8cbdf7e7a9e60320cf1f7b..4a888adbb68c1a7b8802808e619c0102288895c6 100644 (file)
--- a/www/analyzer/open_projects.html
+++ b/www/analyzer/open_projects.html
@@ -174,11 +174,6 @@ mailing list</a> to notify other members of the community.</p>
     <i>(Difficulty: Easy)</i></p>
     </li>
 
-    <li>Teach CStringChecker that strings are never longer than, say, <code>SIZE_MAX/4</code> characters.</li>
-    <p>Though most of CStringChecker's functionality is disabled (due to poor diagnostics for error edge cases), it's still used to model certain operations like <code>strlen</code>, which should give the same result each time it's called on a string. However, assuming that the string length is an arbitrary symbolic value can give strange results -- for example, <code>strlen(str)+1</code> could wrap around to 0. (This is the root cause of <a href="http://llvm.org/bugs/show_bug.cgi?id=16558">PR16558</a>.) In practice, strings are never that long, so picking some large upper bound and recording that in the state would make plenty of sense, and would fix these false positives.
-    <i>(Difficulty: Easy)</i></p>
-    </li>
-
     <li>Implement iterators invalidation checker.
     <p><i>(Difficulty: Easy)</i></p>
     </li>