[PATCH] D48831: alpha.unix.cstring.OutOfBounds checker enable/disable fix

2018-07-13 Thread Balogh , Ádám via Phabricator via cfe-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rC337000: [Analyzer] alpha.unix.cstring.OutOfBounds checker 
enable/disable fix (authored by baloghadamsoftware, committed by ).

Repository:
  rC Clang

https://reviews.llvm.org/D48831

Files:
  lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  test/Analysis/cstring-plist.c
  test/Analysis/malloc.c
  test/Analysis/string.c

Index: test/Analysis/malloc.c
===
--- test/Analysis/malloc.c
+++ test/Analysis/malloc.c
@@ -375,16 +375,16 @@
 // 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
 }
 
@@ -1232,7 +1232,7 @@
 
   if (myValueSize <= sizeof(stackBuffer))
 buffer = stackBuffer;
-  else 
+  else
 buffer = malloc(myValueSize);
 
   // do stuff with the buffer
@@ -1246,15 +1246,15 @@
 
   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}}
-}
+}
 //  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.
@@ -1698,7 +1698,7 @@
 void *smallocNoWarn(size_t size) {
   if (size == 0) {
 return malloc(1); // this branch is never called
-  } 
+  }
   else {
 return malloc(size);
   }
@@ -1777,21 +1777,12 @@
   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;
Index: test/Analysis/cstring-plist.c
===
--- test/Analysis/cstring-plist.c
+++ test/Analysis/cstring-plist.c
@@ -0,0 +1,22 @@
+// 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: diagnostics
+// CHECK-NEXT: 
+// CHECK-NEXT: 
+// CHECK-NEXT: 
+// CHECK-NEXT: 
Index: test/Analysis/string.c
===
--- test/Analysis/string.c
+++ test/Analysis/string.c
@@ -31,6 +31,8 @@
 void clang_analyzer_eval(int);
 
 int scanf(const char *restrict format, ...);
+void *malloc(size_t);
+void free(void *);
 
 //===--===
 // strlen()
@@ -110,7 +112,7 @@
   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.
@@ -309,11 +311,13 @@
   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];
@@ -348,11 +352,13 @@
   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); // ex

[PATCH] D48831: alpha.unix.cstring.OutOfBounds checker enable/disable fix

2018-07-13 Thread Artem Dergachev via Phabricator via cfe-commits 
NoQ accepted this revision.
NoQ added a comment.

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


https://reviews.llvm.org/D48831



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

[PATCH] D48831: alpha.unix.cstring.OutOfBounds checker enable/disable fix

2018-07-13 Thread Daniel Krupp via Phabricator via cfe-commits 
dkrupp marked an inline comment as done.
dkrupp added a comment.

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


https://reviews.llvm.org/D48831



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

[PATCH] D48831: alpha.unix.cstring.OutOfBounds checker enable/disable fix

2018-07-05 Thread Balogh , Ádám via Phabricator via cfe-commits 
baloghadamsoftware accepted this revision.
baloghadamsoftware added a comment.
This revision is now accepted and ready to land.

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




Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:311
+if (!Filter.CheckCStringOutOfBounds)
+  return StOutBound;
 

dkrupp wrote:
> NoQ wrote:
> > 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?
> 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.
> 
I think the assertion could be preserved after the early exit, but it has no 
point to repeat the same condition in subsequent lines.


https://reviews.llvm.org/D48831



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

[PATCH] D48831: alpha.unix.cstring.OutOfBounds checker enable/disable fix

2018-07-03 Thread Daniel Krupp via Phabricator via cfe-commits 
dkrupp marked 2 inline comments as done.
dkrupp added inline comments.



Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:311
+if (!Filter.CheckCStringOutOfBounds)
+  return StOutBound;
 

NoQ wrote:
> 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?
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.



https://reviews.llvm.org/D48831



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

[PATCH] D48831: alpha.unix.cstring.OutOfBounds checker enable/disable fix

2018-07-03 Thread Daniel Krupp via Phabricator via cfe-commits 
dkrupp updated this revision to Diff 153905.
dkrupp added a comment.

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.


https://reviews.llvm.org/D48831

Files:
  lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  test/Analysis/cstring-plist.c
  test/Analysis/malloc.c
  test/Analysis/string.c

Index: test/Analysis/string.c
===
--- test/Analysis/string.c
+++ test/Analysis/string.c
@@ -31,6 +31,8 @@
 void clang_analyzer_eval(int);
 
 int scanf(const char *restrict format, ...);
+void *malloc(size_t);
+void free(void *);
 
 //===--===
 // strlen()
@@ -110,7 +112,7 @@
   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.
@@ -309,11 +311,13 @@
   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];
@@ -348,11 +352,13 @@
   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];
@@ -403,6 +409,7 @@
   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)
@@ -420,6 +427,7 @@
   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";
@@ -496,6 +504,15 @@
   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)
@@ -516,6 +533,7 @@
   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];
@@ -592,6 +610,7 @@
   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)
@@ -615,6 +634,8 @@
   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)
@@ -632,6 +653,7 @@
   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);
@@ -649,6 +671,7 @@
   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".
Index: test/Analysis/malloc.c
===
--- test/Analysis/malloc.c
+++ test/Analysis/malloc.c
@@ -375,16 +375,16 @@
 // 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
 }
 
@@ -1232,7 +1232,7 @@
 
   if (myValueSize <= sizeof(stackBuf

[PATCH] D48831: alpha.unix.cstring.OutOfBounds checker enable/disable fix

2018-07-02 Thread Artem Dergachev via Phabricator via cfe-commits 
NoQ added a comment.

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




Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:308
 // 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.

Accidental double space.



Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:311
+if (!Filter.CheckCStringOutOfBounds)
+  return StOutBound;
 

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?


Repository:
  rC Clang

https://reviews.llvm.org/D48831



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