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

[NoQ]

Summary

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.

[Charusso]

Could you write a test for strcmp please? I have found out the evalMemcmp behaves differently than evalStrcmp. However when the memcmp is going to be used like strcmp/strncmp the ideal would be to delegate the work to evalStrcmp. At the moment there is no connection between the two evaluation at all, which surprised me at first.

Other than that I really like the idea to remove overhead. Thanks!

[NoQ]

Fair point. Yeah, i wonder what the real difference is :/ Added tests.

[NoQ]

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.

Wait, no, that's not what i'm doing. I'm simply forgetting about the assumption that the buffers don't overlap. So never mind, we're not losing any coverage and we're not stepping into an aliasing problem.

[xazax.hun]

Less state split, yay!

clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp

if (stateNonZeroSize) {
     DefinedOrUnknownSVal RV =
         state->getSVal(Right, LCtx).castAs<DefinedOrUnknownSVal>();
 
     // See if they are the same.
     DefinedOrUnknownSVal SameBuf = svalBuilder.evalEQ(state, LV, RV);
     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) {
         state = StSameBuf->BindExpr(CE, LCtx,
                                     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);
+      SVal CmpV =
+          svalBuilder.conjureSymbolVal(nullptr, CE, LCtx, C.blockCount());
 
 void CStringChecker::evalstrLength(CheckerContext &C,
                                    const CallExpr *CE) const {
   // size_t strlen(const char *s);
   evalstrLengthCommon(C, CE, /* IsStrnlen = */ false);
 }
 
 void CStringChecker::evalstrnLength(CheckerContext &C,

clang/test/Analysis/bstring.c

 }
 
 int memcmp7 (char *a, size_t x, size_t y, size_t n) {
   // We used to crash when either of the arguments was unknown.
   return memcmp(a, &a[x*y], 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()
 //===----------------------------------------------------------------------===
 
 #define bcopy BUILTIN(bcopy)
 // __builtin_bcopy is not defined with const in Builtins.def.
 void bcopy(/*const*/ void *s1, void *s2, size_t n);