[analyzer] CStringChecker: Fix overly eager assumption that memcmp args overlap.

While analyzing code `memcmp(a, NULL, n);', where `a' has an unconstrained
symbolic value, the analyzer was emitting a warning about the *first* argument
being a null pointer, even though we'd rather have it warn about the *second*
argument.

This happens because CStringChecker first checks whether the two argument
buffers are in fact the same buffer, in order to take the fast path.
This boils down to assuming `a == NULL' to true. Then the subsequent check
for null pointer argument "discovers" that `a' is null.

Don't take the fast path unless we are *sure* that the buffers are the same.
Otherwise proceed as normal.

Differential Revision: https://reviews.llvm.org/D71322

3 files changed, 43 insertions, 14 deletions

diff --git a/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
index c05a09da3df..4203f790e21 100644
--- a/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
@@ -1313,9 +1313,9 @@ void CStringChecker::evalMemcmp(CheckerContext &C, const CallExpr *CE) const {
     ProgramStateRef StSameBuf, StNotSameBuf;
     std::tie(StSameBuf, StNotSameBuf) = state->assume(SameBuf);
 
-    // If the two arguments might be the same buffer, we know the result is 0,
+    // If the two arguments are the same buffer, we know the result is 0,
     // and we only need to check one size.
-    if (StSameBuf) {
+    if (StSameBuf && !StNotSameBuf) {
       state = StSameBuf;
       state = CheckBufferAccess(C, state, Size, Left);
       if (state) {
@@ -1323,20 +1323,19 @@ void CStringChecker::evalMemcmp(CheckerContext &C, const CallExpr *CE) const {
                                     svalBuilder.makeZeroVal(CE->getType()));
         C.addTransition(state);
       }
+      return;
     }
 
-    // If the two arguments might be different buffers, we have to check the
-    // size of both of them.
-    if (StNotSameBuf) {
-      state = StNotSameBuf;
-      state = CheckBufferAccess(C, state, Size, Left, Right);
-      if (state) {
-        // The return value is the comparison result, which we don't know.
-        SVal CmpV = svalBuilder.conjureSymbolVal(nullptr, CE, LCtx,
-                                                 C.blockCount());
-        state = state->BindExpr(CE, LCtx, CmpV);
-        C.addTransition(state);
-      }
+    // If the two arguments might be different buffers, we have to check
+    // the size of both of them.
+    assert(StNotSameBuf);
+    state = CheckBufferAccess(C, state, Size, Left, Right);
+    if (state) {
+      // The return value is the comparison result, which we don't know.
+      SVal CmpV =
+          svalBuilder.conjureSymbolVal(nullptr, CE, LCtx, C.blockCount());
+      state = state->BindExpr(CE, LCtx, CmpV);
+      C.addTransition(state);
     }
   }
 }
diff --git a/clang/test/Analysis/bstring.c b/clang/test/Analysis/bstring.c
index 8d8f64cebec..2d53402a9ad 100644
--- a/clang/test/Analysis/bstring.c
+++ b/clang/test/Analysis/bstring.c
@@ -462,6 +462,12 @@ int memcmp7 (char *a, size_t x, size_t y, size_t n) {
          memcmp(&a[x*y], a, n);
 }
 
+int memcmp8(char *a, size_t n) {
+  char *b = 0;
+  // Do not warn about the first argument!
+  return memcmp(a, b, n); // expected-warning{{Null pointer passed as 2nd argument to memory comparison function}}
+}
+
 //===----------------------------------------------------------------------===
 // bcopy()
 //===----------------------------------------------------------------------===
diff --git a/clang/test/Analysis/string.c b/clang/test/Analysis/string.c
index d21ec11c111..7612614bc09 100644
--- a/clang/test/Analysis/string.c
+++ b/clang/test/Analysis/string.c
@@ -867,6 +867,12 @@ void strcmp_union_function_pointer_cast(union argument a) {
   fPtr(&a);
 }
 
+int strcmp_null_argument(char *a) {
+  char *b = 0;
+  // Do not warn about the first argument!
+  return strcmp(a, b); // expected-warning{{Null pointer passed as 2nd argument to string comparison function}}
+}
+
 //===----------------------------------------------------------------------===
 // strncmp()
 //===----------------------------------------------------------------------===
@@ -976,6 +982,12 @@ void strncmp_embedded_null () {
 	clang_analyzer_eval(strncmp("ab\0zz", "ab\0yy", 4) == 0); // expected-warning{{TRUE}}
 }
 
+int strncmp_null_argument(char *a, size_t n) {
+  char *b = 0;
+  // Do not warn about the first argument!
+  return strncmp(a, b, n); // expected-warning{{Null pointer passed as 2nd argument to string comparison function}}
+}
+
 //===----------------------------------------------------------------------===
 // strcasecmp()
 //===----------------------------------------------------------------------===
@@ -1067,6 +1079,12 @@ void strcasecmp_embedded_null () {
 	clang_analyzer_eval(strcasecmp("ab\0zz", "ab\0yy") == 0); // expected-warning{{TRUE}}
 }
 
+int strcasecmp_null_argument(char *a) {
+  char *b = 0;
+  // Do not warn about the first argument!
+  return strcasecmp(a, b); // expected-warning{{Null pointer passed as 2nd argument to string comparison function}}
+}
+
 //===----------------------------------------------------------------------===
 // strncasecmp()
 //===----------------------------------------------------------------------===
@@ -1176,6 +1194,12 @@ void strncasecmp_embedded_null () {
 	clang_analyzer_eval(strncasecmp("ab\0zz", "ab\0yy", 4) == 0); // expected-warning{{TRUE}}
 }
 
+int strncasecmp_null_argument(char *a, size_t n) {
+  char *b = 0;
+  // Do not warn about the first argument!
+  return strncasecmp(a, b, n); // expected-warning{{Null pointer passed as 2nd argument to string comparison function}}
+}
+
 //===----------------------------------------------------------------------===
 // strsep()
 //===----------------------------------------------------------------------===