CStringChecker can crash when uninitialized checks are disabled

[vlad.tsyrklevich]

Summary

CStringChecker assumes that SVals are not undefined at two points with comments stating that other checkers will check for that condition first; however, it can crash if a user chooses a particular configuration. I hit such an unlucky configuration while eliminating false positive heavy checks analyzing the Linux kernel and tracked down the crashes to this assumption.

Add UndefinedVal checks where appropriate.

[xazax.hun]

It is not supported to run the analyzer with some of the core checkers turned off. Maybe we should change the behavior such that turning off core checkers turn off the warnings from those checkers but not the checkers themselves?

[zaks.anna]

It is not supported to run the analyzer with some of the core checkers turned off.

Correct.

Maybe we should change the behavior such that turning off core checkers turn off the warnings from those checkers but not the checkers themselves?

Having this as the default behavior for "disable a checker" could be confusing. however, introducing a new flag for silencing warnings from a checker sounds fine.

What is the motivation for disabling the core checkers in this particular case?

[vlad.tsyrklevich]

The motivation was to make resulting output easier to navigate and to cut the result size by ~90%, one default run against the FreeBSD kernel takes 3 gigs of storage and I'm running several builds a day as I iterate on checks. I did not realize that running without core was unsupported. A silencing flag would have done the trick.

[xazax.hun]

You might want to give CodeChecker [1] a try as a workaround. It stores the results in a more compact format and you can do filtering.

[1] https://github.com/Ericsson/codechecker

lib/StaticAnalyzer/Checkers/CStringChecker.cpp

state = state->BindExpr(CE, LCtx,
                             svalBuilder.makeZeroVal(CE->getType()));
     C.addTransition(state);
   }
 
   // If the size can be nonzero, we have to check the other arguments.
   if (stateNonZeroSize) {
     state = stateNonZeroSize;
     // If we know the two buffers are the same, we know the result is 0.
-    // First, get the two buffers' addresses. Another checker will have already
-    // made sure they're not undefined.
-    DefinedOrUnknownSVal LV =
-        state->getSVal(Left, LCtx).castAs<DefinedOrUnknownSVal>();
-    DefinedOrUnknownSVal RV =
-        state->getSVal(Right, LCtx).castAs<DefinedOrUnknownSVal>();
+    // First, get the two buffers' addresses.
+    Optional<DefinedOrUnknownSVal> LV =
+        state->getSVal(Left, LCtx).getAs<DefinedOrUnknownSVal>();
+    Optional<DefinedOrUnknownSVal> RV =
+        state->getSVal(Right, LCtx).getAs<DefinedOrUnknownSVal>();
+
+    if (!LV || !RV)
+      return;
 
     // See if they are the same.
-    DefinedOrUnknownSVal SameBuf = svalBuilder.evalEQ(state, LV, RV);
+    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,
     // and we only need to check one size.
     if (StSameBuf) {
       state = StSameBuf;
       state = CheckBufferAccess(C, state, Size, Left);
void CStringChecker::evalStrcmpCommon(CheckerContext &C, const CallExpr *CE,
                                       bool isBounded, bool ignoreCase) const {
   CurrentFunctionDescription = "string comparison function";
   ProgramStateRef state = C.getState();
   const LocationContext *LCtx = C.getLocationContext();
 
   // Check that the first string is non-null
   const Expr *s1 = CE->getArg(0);
   SVal s1Val = state->getSVal(s1, LCtx);
+  if (s1Val.isUndef())
+    return;
+
   state = checkNonNull(C, state, s1, s1Val);
   if (!state)
     return;
 
   // Check that the second string is non-null.
   const Expr *s2 = CE->getArg(1);
   SVal s2Val = state->getSVal(s2, LCtx);
+  if (s2Val.isUndef())
+    return;
+
   state = checkNonNull(C, state, s2, s2Val);
   if (!state)
     return;
 
   // Get the string length of the first string or give up.
   SVal s1Length = getCStringLength(C, state, s1, s1Val);
   if (s1Length.isUndef())
     return;
 
   // Get the string length of the second string or give up.
   SVal s2Length = getCStringLength(C, state, s2, s2Val);
   if (s2Length.isUndef())
     return;
 
   // If we know the two buffers are the same, we know the result is 0.
-  // First, get the two buffers' addresses. Another checker will have already
-  // made sure they're not undefined.
+  // First, get the two buffers' addresses.
   DefinedOrUnknownSVal LV = s1Val.castAs<DefinedOrUnknownSVal>();
   DefinedOrUnknownSVal RV = s2Val.castAs<DefinedOrUnknownSVal>();
 
   // See if they are the same.
   SValBuilder &svalBuilder = C.getSValBuilder();
   DefinedOrUnknownSVal SameBuf = svalBuilder.evalEQ(state, LV, RV);
   ProgramStateRef StSameBuf, StNotSameBuf;
   std::tie(StSameBuf, StNotSameBuf) = state->assume(SameBuf);

test/Analysis/cstring-regressions.c

+// RUN: %clang_cc1 -analyze -analyzer-checker=core,unix.cstring,debug.ExprInspection -analyzer-disable-checker=core.UndefinedBinaryOperatorResult,core.CallAndMessage,core.NullDereference,core.uninitialized -analyzer-store=region -verify %s
+//
+// CStringChecker crashes found running analyzer over the Linux kernel source
+// with a very particular set of enabled/disabled checks.
+
+int strcmp(const char * s1, const char * s2);
+int memcmp(const void *s1, const void *s2, unsigned long n);
+void clang_analyzer_eval(int);
+
+void test1(void *ptr) {
+ int uninit;
+ clang_analyzer_eval(strcmp(ptr + uninit, "mcp_trace_meta") == 0); // expected-warning{{UNKNOWN}}
+}
+
+void test2(char *foo, void *ptr) {
+ int uninit;
+ clang_analyzer_eval(memcmp(foo, ptr + uninit, 48) == 0); // expected-warning{{UNKNOWN}}
+}