[Analyzer] alpha.unix.cstring.OutOfBounds checker enable/disable fix

[dkrupp]

Summary

It was not possible to disable alpha.unix.cstring.OutOfBounds checker's reports
since unix.Malloc checker always implicitly enabled the filter.
Moreover if the checker was disabled from command line (-analyzer-disable-checker ...)
the out of bounds warnings were nevertheless emitted under different
checker names such as unix.cstring.NullArg, or unix.Malloc.

This patch fixes the case sot that Malloc checker only enables implicitly the underlying
modeling of strcpy, memcpy etc. but not the warning messages that would have been
emmitted by alpha.unix.cstring.OutOfBounds

[NoQ]

Uhm, so we had an alpha checker enabled all along? Thanks for patching this up!

lib/StaticAnalyzer/Checkers/CStringChecker.cpp

307–308

Accidental double space.

310

Could we preserve the other portion of the assertion on this branch? I.e., assert(Filter.CheckCStringNullArg).

Additionally, do you really want to continue analysis on this path? Maybe return nullptr to sink?

[dkrupp]

The patch has been updated.
Changes:

-The analysis path is cut if overvlow is detected even if CStringOutOfBounds is disabled

The assert(Filter.CheckCStringOutOfBounds || Filter.CheckCStringNullArg); cannot be put back, because if both checkers are disabled, the assertion is not true.

[dkrupp]

lib/StaticAnalyzer/Checkers/CStringChecker.cpp

310

I was unsure whether to return nullptr or StOutBound. I thought that alpha.unix.cstring.OutOfBounds is in alpha because it may falsely detect buffer overflows and then we would cut the path unnecessarily.
But OK, it is safer this way.

I could not put back the assertion, because if only unix.Malloc checker is enabled (and CStringOutOfBounds and CStringNullArg are not) the assertion is not true.

[baloghadamsoftware]

LGTM! But wait for Artem's acceptance before submitting.

lib/StaticAnalyzer/Checkers/CStringChecker.cpp

310

I think the assertion could be preserved after the early exit, but it has no point to repeat the same condition in subsequent lines.

[dkrupp]

@NoQ do we need any more update to this patch? Thanks.

[NoQ]

Whoops, sry, yeah, looks good, please commit!

lib/StaticAnalyzer/Checkers/CStringChecker.cpp

ProgramStateRef CStringChecker::CheckLocation(CheckerContext &C,
   DefinedOrUnknownSVal Size = Extent.castAs<DefinedOrUnknownSVal>();
 
   // Get the index of the accessed element.
   DefinedOrUnknownSVal Idx = ER->getIndex().castAs<DefinedOrUnknownSVal>();
 
   ProgramStateRef StInBound = state->assumeInBound(Idx, Size, true);
   ProgramStateRef StOutBound = state->assumeInBound(Idx, Size, false);
   if (StOutBound && !StInBound) {
     // These checks are either enabled by the CString out-of-bounds checker
-    // explicitly or the "basic" CStringNullArg checker support that Malloc
-    // checker enables.
-    assert(Filter.CheckCStringOutOfBounds || Filter.CheckCStringNullArg);
-
+    // explicitly or implicitly by the Malloc checker.
+    // In the latter case we only do modeling but do not emit warning.
+    if (!Filter.CheckCStringOutOfBounds)
+      return nullptr;
     // Emit a bug report.
     if (warningMsg) {
       emitOutOfBoundsBug(C, StOutBound, S, warningMsg);
     } else {
       assert(CurrentFunctionDescription);
       assert(CurrentFunctionDescription[0] != '\0');
 
       SmallString<80> buf;
if (Offset.isValid() && !Offset.hasSymbolicOffset() &&
     // Get the base region's extent.
     auto *SubReg = cast<SubRegion>(BR);
     DefinedOrUnknownSVal Extent = SubReg->getExtent(svalBuilder);
 
     ProgramStateRef StateWholeReg, StateNotWholeReg;
     std::tie(StateWholeReg, StateNotWholeReg) =
         State->assume(svalBuilder.evalEQ(State, Extent, *SizeNL));
 
-    // With the semantic of 'memset()', we should convert the CharVal to 
+    // With the semantic of 'memset()', we should convert the CharVal to
     // unsigned char.
     CharVal = svalBuilder.evalCast(CharVal, Ctx.UnsignedCharTy, Ctx.IntTy);
 
     ProgramStateRef StateNullChar, StateNonNullChar;
     std::tie(StateNullChar, StateNonNullChar) =
         assumeZero(C, State, CharVal, Ctx.UnsignedCharTy);
 
     if (StateWholeReg && !StateNotWholeReg && StateNullChar &&
#define REGISTER_CHECKER(name)                                                 \
   }
 
   REGISTER_CHECKER(CStringNullArg)
   REGISTER_CHECKER(CStringOutOfBounds)
   REGISTER_CHECKER(CStringBufferOverlap)
 REGISTER_CHECKER(CStringNotNullTerm)
 
   void ento::registerCStringCheckerBasic(CheckerManager &Mgr) {
-    registerCStringNullArg(Mgr);
+    Mgr.registerChecker<CStringChecker>();
   }

test/Analysis/cstring-plist.c

+// RUN: rm -f %t
+// RUN: %clang_analyze_cc1 -fblocks -analyzer-checker=core,unix.Malloc,unix.cstring.NullArg -analyzer-disable-checker=alpha.unix.cstring.OutOfBounds -analyzer-output=plist -analyzer-config path-diagnostics-alternate=false -o %t %s
+// RUN: FileCheck -input-file %t %s
+
+typedef __typeof(sizeof(int)) size_t;
+void *malloc(size_t);
+void free(void *);
+char *strncpy(char *restrict s1, const char *restrict s2, size_t n);
+
+
+
+void cstringchecker_bounds_nocrash() {
+  char *p = malloc(2);
+  strncpy(p, "AAA", sizeof("AAA")); // we don't expect warning as the checker is disabled
+  free(p);
+}
+
+// CHECK: <key>diagnostics</key>
+// CHECK-NEXT: <array>
+// CHECK-NEXT: </array>
+// CHECK-NEXT: </dict>
+// CHECK-NEXT: </plist>

test/Analysis/malloc.c

void CheckUseZeroReallocatedPathWarn(_Bool b) {
   free(q);
 }
 
 // This case tests that storing malloc'ed memory to a static variable which is
 // then returned is not leaked.  In the absence of known contracts for functions
 // or inter-procedural analysis, this is a conservative answer.
 int *f3() {
   static int *p = 0;
-  p = malloc(12); 
+  p = malloc(12);
   return p; // no-warning
 }
 
 // This case tests that storing malloc'ed memory to a static global variable
 // which is then returned is not leaked.  In the absence of known contracts for
 // functions or inter-procedural analysis, this is a conservative answer.
 static int *p_f4 = 0;
 int *f4() {
-  p_f4 = malloc(12); 
+  p_f4 = malloc(12);
   return p_f4; // no-warning
 }
 
 int *f5() {
   int *q = malloc(12);
   q = realloc(q, 20);
   return q; // no-warning
 }

 // a heap allocation for small work sizes.  This tests the analyzer's
 // understanding that the malloc'ed memory is not the same as stackBuffer.
 void radar10978247(int myValueSize) {
   char stackBuffer[128];
   char *buffer;
 
   if (myValueSize <= sizeof(stackBuffer))
     buffer = stackBuffer;
-  else 
+  else
     buffer = malloc(myValueSize);
 
   // do stuff with the buffer
   if (buffer != stackBuffer)
     free(buffer);
 }
 
 void radar10978247_positive(int myValueSize) {
   char stackBuffer[128];
   char *buffer;
 
   if (myValueSize <= sizeof(stackBuffer))
     buffer = stackBuffer;
-  else 
+  else
     buffer = malloc(myValueSize);
 
   // do stuff with the buffer
   if (buffer == stackBuffer)
     return;
   else
     return; // expected-warning {{leak}}
-}␋
+}
 // <rdar://problem/11269741> Previously this triggered a false positive
 // because malloc() is known to return uninitialized memory and the binding
 // of 'o' to 'p->n' was not getting propertly handled.  Now we report a leak.
 struct rdar11269741_a_t {
   struct rdar11269741_b_t {
     int m;
   } n;
 };
if (new_memory != 0) {
     *memory = new_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);
void freeIndirectFunctionPtr() {
   void *p = (void *)fnptr;
   free(p); // expected-warning {{Argument to free() is a function pointer}}
 }
 
 void freeFunctionPtr() {
   free((void *)fnptr); // expected-warning {{Argument to free() is a function pointer}}
 }
 
-// Enabling the malloc checker enables some of the buffer-checking portions
-// of the C-string checker.
-void cstringchecker_bounds_nocrash() {
-  char *p = malloc(2);
-  strncpy(p, "AAA", sizeof("AAA")); // expected-warning {{Size argument is greater than the length of the destination buffer}}
-
-  free(p);
-}
-
 void allocateSomeMemory(void *offendingParameter, void **ptr) {
   *ptr = malloc(1);
 }
 
 void testNoCrashOnOffendingParameter() {
-  // "extern" is necessary to avoid unrelated warnings 
+  // "extern" is necessary to avoid unrelated warnings
   // on passing uninitialized value.
   extern void *offendingParameter;
   void* ptr;
   allocateSomeMemory(offendingParameter, &ptr);
 } // expected-warning {{Potential leak of memory pointed to by 'ptr'}}
 
 // ----------------------------------------------------------------------------
 // False negatives.

test/Analysis/string.c

 #endif /* USE_BUILTINS */
 
 #define NULL 0
 typedef typeof(sizeof(int)) size_t;
 
 void clang_analyzer_eval(int);
 
 int scanf(const char *restrict format, ...);
+void *malloc(size_t);
+void free(void *);
 
 //===----------------------------------------------------------------------===
 // strlen()
 //===----------------------------------------------------------------------===
 
 #define strlen BUILTIN(strlen)
 size_t strlen(const char *s);
 

 
 extern char global_str[];
 void strlen_global() {
   size_t a = strlen(global_str);
   size_t b = strlen(global_str);
   if (a == 0) {
     clang_analyzer_eval(b == 0); // expected-warning{{TRUE}}
     // Make sure clang_analyzer_eval does not invalidate globals.
-    clang_analyzer_eval(strlen(global_str) == 0); // expected-warning{{TRUE}}    
+    clang_analyzer_eval(strlen(global_str) == 0); // expected-warning{{TRUE}}
   }
 
   // Call a function with unknown effects, which should invalidate globals.
   use_string(0);
 
   size_t c = strlen(global_str);
   if (a == 0)
     clang_analyzer_eval(c == 0); // expected-warning{{UNKNOWN}}
if (globalInt != 42)
     return;
 
   clang_analyzer_eval(strcpy(x, y) == x); // expected-warning{{TRUE}}
   clang_analyzer_eval(strlen(x) == strlen(y)); // expected-warning{{TRUE}}
   clang_analyzer_eval(a == x[0]); // expected-warning{{UNKNOWN}}
   clang_analyzer_eval(globalInt == 42); // expected-warning{{TRUE}}
 }
 
+#ifndef SUPPRESS_OUT_OF_BOUND
 void strcpy_overflow(char *y) {
   char x[4];
   if (strlen(y) == 4)
     strcpy(x, y); // expected-warning{{String copy function overflows destination buffer}}
 }
+#endif
 
 void strcpy_no_overflow(char *y) {
   char x[4];
   if (strlen(y) == 3)
     strcpy(x, y); // no-warning
 }
 
 //===----------------------------------------------------------------------===

 void stpcpy_effect(char *x, char *y) {
   char a = x[0];
 
   clang_analyzer_eval(stpcpy(x, y) == &x[strlen(y)]); // expected-warning{{TRUE}}
   clang_analyzer_eval(strlen(x) == strlen(y)); // expected-warning{{TRUE}}
   clang_analyzer_eval(a == x[0]); // expected-warning{{UNKNOWN}}
 }
 
+#ifndef SUPPRESS_OUT_OF_BOUND
 void stpcpy_overflow(char *y) {
   char x[4];
   if (strlen(y) == 4)
     stpcpy(x, y); // expected-warning{{String copy function overflows destination buffer}}
 }
+#endif
 
 void stpcpy_no_overflow(char *y) {
   char x[4];
   if (strlen(y) == 3)
     stpcpy(x, y); // no-warning
 }
 
 //===----------------------------------------------------------------------===
void strcat_effects(char *y) {
 
   if (strlen(y) != 4)
     return;
 
   clang_analyzer_eval(strcat(x, y) == x); // expected-warning{{TRUE}}
   clang_analyzer_eval((int)strlen(x) == (orig_len + strlen(y))); // expected-warning{{TRUE}}
 }
 
+#ifndef SUPPRESS_OUT_OF_BOUND
 void strcat_overflow_0(char *y) {
   char x[4] = "12";
   if (strlen(y) == 4)
     strcat(x, y); // expected-warning{{String copy function overflows destination buffer}}
 }
 
 void strcat_overflow_1(char *y) {
   char x[4] = "12";
   if (strlen(y) == 3)
     strcat(x, y); // expected-warning{{String copy function overflows destination buffer}}
 }
 
 void strcat_overflow_2(char *y) {
   char x[4] = "12";
   if (strlen(y) == 2)
     strcat(x, y); // expected-warning{{String copy function overflows destination buffer}}
 }
+#endif
 
 void strcat_no_overflow(char *y) {
   char x[5] = "12";
   if (strlen(y) == 2)
     strcat(x, y); // no-warning
 }
 
 void strcat_symbolic_dst_length(char *dst) {

 void strncpy_effects(char *x, char *y) {
   char a = x[0];
 
   clang_analyzer_eval(strncpy(x, y, 5) == x); // expected-warning{{TRUE}}
   clang_analyzer_eval(strlen(x) == strlen(y)); // expected-warning{{UNKNOWN}}
   clang_analyzer_eval(a == x[0]); // expected-warning{{UNKNOWN}}
 }
 
+#ifndef SUPPRESS_OUT_OF_BOUND
+// Enabling the malloc checker enables some of the buffer-checking portions
+// of the C-string checker.
+void cstringchecker_bounds_nocrash() {
+  char *p = malloc(2);
+  strncpy(p, "AAA", sizeof("AAA")); // expected-warning {{Size argument is greater than the length of the destination buffer}}
+  free(p);
+}
+
 void strncpy_overflow(char *y) {
   char x[4];
   if (strlen(y) == 4)
     strncpy(x, y, 5); // expected-warning{{Size argument is greater than the length of the destination buffer}}
 }
 
 void strncpy_no_overflow(char *y) {
   char x[4];
   if (strlen(y) == 3)
     strncpy(x, y, 5); // expected-warning{{Size argument is greater than the length of the destination buffer}}
 }
 
 void strncpy_no_overflow2(char *y, int n) {
   if (n <= 4)
     return;
 
   char x[4];
   if (strlen(y) == 3)
     strncpy(x, y, n); // expected-warning{{Size argument is greater than the length of the destination buffer}}
 }
+#endif
 
 void strncpy_truncate(char *y) {
   char x[4];
   if (strlen(y) == 4)
     strncpy(x, y, 3); // no-warning
 }
 
 void strncpy_no_truncate(char *y) {
void strncat_effects(char *y) {
 
   if (strlen(y) != 4)
     return;
 
   clang_analyzer_eval(strncat(x, y, strlen(y)) == x); // expected-warning{{TRUE}}
   clang_analyzer_eval(strlen(x) == (orig_len + strlen(y))); // expected-warning{{TRUE}}
 }
 
+#ifndef SUPPRESS_OUT_OF_BOUND
 void strncat_overflow_0(char *y) {
   char x[4] = "12";
   if (strlen(y) == 4)
     strncat(x, y, strlen(y)); // expected-warning{{Size argument is greater than the free space in the destination buffer}}
 }
 
 void strncat_overflow_1(char *y) {
   char x[4] = "12";
   if (strlen(y) == 3)
     strncat(x, y, strlen(y)); // expected-warning{{Size argument is greater than the free space in the destination buffer}}
 }
 
 void strncat_overflow_2(char *y) {
   char x[4] = "12";
   if (strlen(y) == 2)
     strncat(x, y, strlen(y)); // expected-warning{{Size argument is greater than the free space in the destination buffer}}
 }
 
 void strncat_overflow_3(char *y) {
   char x[4] = "12";
   if (strlen(y) == 4)
     strncat(x, y, 2); // expected-warning{{Size argument is greater than the free space in the destination buffer}}
 }
+#endif
+
 void strncat_no_overflow_1(char *y) {
   char x[5] = "12";
   if (strlen(y) == 2)
     strncat(x, y, strlen(y)); // no-warning
 }
 
 void strncat_no_overflow_2(char *y) {
   char x[4] = "12";
   if (strlen(y) == 4)
     strncat(x, y, 1); // no-warning
 }
 
 void strncat_symbolic_dst_length(char *dst) {
   strncat(dst, "1234", 5);
   clang_analyzer_eval(strlen(dst) >= 4); // expected-warning{{TRUE}}
 }
 
+#ifndef SUPPRESS_OUT_OF_BOUND
 void strncat_symbolic_src_length(char *src) {
   char dst[8] = "1234";
   strncat(dst, src, 3);
   clang_analyzer_eval(strlen(dst) >= 4); // expected-warning{{TRUE}}
 
   char dst2[8] = "1234";
   strncat(dst2, src, 4); // expected-warning{{Size argument is greater than the free space in the destination buffer}}
 }
 
 void strncat_unknown_src_length(char *src, int offset) {
   char dst[8] = "1234";
   strncat(dst, &src[offset], 3);
   clang_analyzer_eval(strlen(dst) >= 4); // expected-warning{{TRUE}}
 
   char dst2[8] = "1234";
   strncat(dst2, &src[offset], 4); // expected-warning{{Size argument is greater than the free space in the destination buffer}}
 }
+#endif
 
 // There is no strncat_unknown_dst_length because if we can't get a symbolic
 // length for the "before" strlen, we won't be able to set one for "after".
 
 void strncat_symbolic_limit(unsigned limit) {
   char dst[6] = "1234";
   char src[] = "567";
   strncat(dst, src, limit); // no-warning