[PATCH] D28765: CStringChecker can crash when uninitialized checks are disabled

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

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


https://reviews.llvm.org/D28765



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D28765: CStringChecker can crash when uninitialized checks are disabled

[Vlad Tsyrklevich via Phabricator via cfe-commits]

vlad.tsyrklevich abandoned this revision.
vlad.tsyrklevich added a comment.

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.


https://reviews.llvm.org/D28765



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D28765: CStringChecker can crash when uninitialized checks are disabled

[Anna Zaks via Phabricator via cfe-commits]

zaks.anna added a comment.

> 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?


https://reviews.llvm.org/D28765



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D28765: CStringChecker can crash when uninitialized checks are disabled

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

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?


https://reviews.llvm.org/D28765



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D28765: CStringChecker can crash when uninitialized checks are disabled

[Vlad Tsyrklevich via Phabricator via cfe-commits]

vlad.tsyrklevich created this revision.
vlad.tsyrklevich added reviewers: cfe-commits, zaks.anna, dcoughlin, NoQ.

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.


https://reviews.llvm.org/D28765

Files:
  lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  test/Analysis/cstring-regressions.c


Index: test/Analysis/cstring-regressions.c
===
--- /dev/null
+++ test/Analysis/cstring-regressions.c
@@ -0,0 +1,18 @@
+// 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}}
+}
Index: lib/StaticAnalyzer/Checkers/CStringChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/CStringChecker.cpp
+++ lib/StaticAnalyzer/Checkers/CStringChecker.cpp
@@ -1179,15 +1179,17 @@
   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 RV =
-state->getSVal(Right, LCtx).castAs();
+// First, get the two buffers' addresses.
+Optional LV =
+state->getSVal(Left, LCtx).getAs();
+Optional RV =
+state->getSVal(Right, LCtx).getAs();
+
+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);
 
@@ -1781,13 +1783,19 @@
   // 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;
@@ -1803,8 +1811,7 @@
 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 RV = s2Val.castAs();
 


Index: test/Analysis/cstring-regressions.c
===
--- /dev/null
+++ test/Analysis/cstring-regressions.c
@@ -0,0 +1,18 @@
+// 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}}
+}
Index: lib/StaticAnalyzer/Checkers/CStringChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/CStringChecker.cpp
+++ lib/StaticAnalyzer/Checkers/CStringChecker.cpp
@@ -1179,15 +1179,17 @@
   if (stateNonZeroSize) {
 state = stateNonZeroSize;