[PATCH] D71322: [analyzer] CStringChecker: Fix overly eager assumption that memcmp arguments overlap.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a_sidorin, rnkovacs, Szelethus, baloghadamsoftware, Charusso.
Herald added subscribers: cfe-commits, dkrupp, donat.nagy, mikhail.ramalho, a.sidorin, szepet.
Herald added a project: clang.
NoQ added a parent revision: D71321: [analyzer] CStringChecker: Warning text fixes..

While analyzing code

  memcmp(a, NULL, n);

where `a` has an unconstrained symbolic value, the analyzer is currently 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 that 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.

This could have been fixed by reordering the checks (first check null arguments, then check for overlap) but i went a bit further and entirely removed the state split for "same buffer vs. different buffers". This state split is not well-justified: we cannot deduce from the presence of `memcpy` in the code that the buffers may in fact overlap. Instead, i conservatively assume that they don't overlap, unless they're already known to certainly overlap on the current execution path. This loses a bit of coverage but the lost path is where they do actually overlap. This path is in my opinion not only rare but also fairly useless, as it immediately introduces an aliasing problem that we aren't quite ready to deal with.


Repository:
  rC Clang

https://reviews.llvm.org/D71322

Files:
  clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  clang/test/Analysis/bstring.c


Index: clang/test/Analysis/bstring.c
===================================================================
--- clang/test/Analysis/bstring.c
+++ clang/test/Analysis/bstring.c
@@ -462,6 +462,12 @@
          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()
 //===----------------------------------------------------------------------===
Index: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
@@ -1313,9 +1313,9 @@
     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 @@
                                     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);
     }
   }
 }


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D71322.233247.patch
Type: text/x-patch
Size: 2552 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20191211/d5b9e1bb/attachment-0001.bin>