[PATCH] D30312: Fix unix.Malloc analysis crasher when allocating dynamic arrays w/unbound statements (fix PR32050)

2017-02-23 Thread Kevin Marshall via Phabricator via cfe-commits 
kmarshall created this revision.

The extent calculation function had a bug which caused it to ignore if the size 
value was defined prior to casting it. As a result, size expressions with free 
variables would trigger assertion failures during the cast operation.

This patch adds that missing check, and replaces the redundant call to 
castAs<>() with the SVar that is returned by the checked cast.

Added a regression test "Malloc+NewDynamicArray" that exercises the fix.


https://reviews.llvm.org/D30312

Files:
  lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  test/Analysis/Malloc+NewDynamicArray.cpp


Index: test/Analysis/Malloc+NewDynamicArray.cpp
===
--- test/Analysis/Malloc+NewDynamicArray.cpp
+++ test/Analysis/Malloc+NewDynamicArray.cpp
@@ -0,0 +1,17 @@
+// RUN: %clang_cc1 -analyze -analyzer-checker=unix.Malloc -verify %s
+
+//---
+// Check that arrays sized using expressions with free variables
+// do not cause the unix.Malloc checker to crash.
+//
+// The function should not actually be called from anywhere, otherwise
+// the compiler will optimize the length expression and replace it with
+// with precomputed literals.
+//---
+
+void AllocateExpr(int lhs, int rhs) {
+  new int[lhs + rhs];
+}
+
+// expected-no-diagnostics
+
Index: lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -1026,12 +1026,11 @@
   ASTContext  = C.getASTContext();
   CharUnits TypeSize = AstContext.getTypeSizeInChars(ElementType);
 
-  if (Optional DefinedSize =
-  ElementCount.getAs()) {
+  if (Optional DefinedSize = ElementCount.getAs()) {
 DefinedOrUnknownSVal Extent = Region->getExtent(svalBuilder);
 // size in Bytes = ElementCount*TypeSize
 SVal SizeInBytes = svalBuilder.evalBinOpNN(
-State, BO_Mul, ElementCount.castAs(),
+State, BO_Mul, *DefinedSize,
 svalBuilder.makeArrayIndex(TypeSize.getQuantity()),
 svalBuilder.getArrayIndexType());
 DefinedOrUnknownSVal extentMatchesSize = svalBuilder.evalEQ(


Index: test/Analysis/Malloc+NewDynamicArray.cpp
===
--- test/Analysis/Malloc+NewDynamicArray.cpp
+++ test/Analysis/Malloc+NewDynamicArray.cpp
@@ -0,0 +1,17 @@
+// RUN: %clang_cc1 -analyze -analyzer-checker=unix.Malloc -verify %s
+
+//---
+// Check that arrays sized using expressions with free variables
+// do not cause the unix.Malloc checker to crash.
+//
+// The function should not actually be called from anywhere, otherwise
+// the compiler will optimize the length expression and replace it with
+// with precomputed literals.
+//---
+
+void AllocateExpr(int lhs, int rhs) {
+  new int[lhs + rhs];
+}
+
+// expected-no-diagnostics
+
Index: lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -1026,12 +1026,11 @@
   ASTContext  = C.getASTContext();
   CharUnits TypeSize = AstContext.getTypeSizeInChars(ElementType);
 
-  if (Optional DefinedSize =
-  ElementCount.getAs()) {
+  if (Optional DefinedSize = ElementCount.getAs()) {
 DefinedOrUnknownSVal Extent = Region->getExtent(svalBuilder);
 // size in Bytes = ElementCount*TypeSize
 SVal SizeInBytes = svalBuilder.evalBinOpNN(
-State, BO_Mul, ElementCount.castAs(),
+State, BO_Mul, *DefinedSize,
 svalBuilder.makeArrayIndex(TypeSize.getQuantity()),
 svalBuilder.getArrayIndexType());
 DefinedOrUnknownSVal extentMatchesSize = svalBuilder.evalEQ(
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30593: Add correct "-isystem"/"-isysroot" warning handling to static analysis' BugReporter.

2017-03-09 Thread Kevin Marshall via Phabricator via cfe-commits 
kmarshall added a comment.

OK. I'll make bugs for the false positives I'm confident about.


Repository:
  rL LLVM

https://reviews.llvm.org/D30593



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

[PATCH] D30593: Add correct "-isystem" warning handling to static analysis' BugReporter.

2017-03-03 Thread Kevin Marshall via Phabricator via cfe-commits 
kmarshall added a comment.

Note to reviewers:  this diff is relative to "llvm/cfe" - I couldn't find a way 
to specify a repository subpath to use for this diff.


Repository:
  rL LLVM

https://reviews.llvm.org/D30593



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

[PATCH] D30593: Add correct "-isystem" warning handling to static analysis' BugReporter.

2017-03-03 Thread Kevin Marshall via Phabricator via cfe-commits 
kmarshall created this revision.

The Clang static analyzer doesn't follow the warning suppression semantics of 
the "-isystem" command line flag. This patch adds a check to BugReporter which 
causes it to drop any BugReports which originated from a system header 
(descendant of an -isystem path).


Repository:
  rL LLVM

https://reviews.llvm.org/D30593

Files:
  lib/StaticAnalyzer/Core/BugReporter.cpp


Index: lib/StaticAnalyzer/Core/BugReporter.cpp
===
--- lib/StaticAnalyzer/Core/BugReporter.cpp
+++ lib/StaticAnalyzer/Core/BugReporter.cpp
@@ -3251,6 +3251,12 @@
   return;
   }

+  // Suppress BugReports which originate from system headers (located beneath
+  // an -isystem include path).
+  if (getSourceManager().isInSystemHeader(
+  R->getLocation(getSourceManager()).asLocation()))
+return;
+
   bool ValidSourceLoc = R->getLocation(getSourceManager()).isValid();
   assert(ValidSourceLoc);
   // If we mess up in a release build, we'd still prefer to just drop the bug


Index: lib/StaticAnalyzer/Core/BugReporter.cpp
===
--- lib/StaticAnalyzer/Core/BugReporter.cpp
+++ lib/StaticAnalyzer/Core/BugReporter.cpp
@@ -3251,6 +3251,12 @@
   return;
   }

+  // Suppress BugReports which originate from system headers (located beneath
+  // an -isystem include path).
+  if (getSourceManager().isInSystemHeader(
+  R->getLocation(getSourceManager()).asLocation()))
+return;
+
   bool ValidSourceLoc = R->getLocation(getSourceManager()).isValid();
   assert(ValidSourceLoc);
   // If we mess up in a release build, we'd still prefer to just drop the bug
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30593: Add correct "-isystem"/"-isysroot" warning handling to static analysis' BugReporter.

2017-03-10 Thread Kevin Marshall via Phabricator via cfe-commits 
kmarshall added a comment.

NP, Anna. I filed a number of bugs for the false positives:

- https://bugs.llvm.org/show_bug.cgi?id=32229
- https://bugs.llvm.org/show_bug.cgi?id=32232
- https://bugs.llvm.org/show_bug.cgi?id=32233
- https://bugs.llvm.org/show_bug.cgi?id=32234
- https://bugs.llvm.org/show_bug.cgi?id=32235




Repository:
  rL LLVM

https://reviews.llvm.org/D30593



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

[PATCH] D30593: Add correct "-isystem"/"-isysroot" warning handling to static analysis' BugReporter.

2017-03-06 Thread Kevin Marshall via Phabricator via cfe-commits 
kmarshall added a comment.

Also, it looks like some instances of safe unique_ptr assignment using 
std::move() are tripping up the analyzer.

Error:

  
../../build/linux/debian_wheezy_amd64-sysroot/usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/bits/unique_ptr.h:190:4:
 warning: Potential memory leak

**C++ library code:**

  template::pointer,
   pointer>::value
   && !std::is_array<_Up>::value>::type>
unique_ptr&
operator=(unique_ptr<_Up, _Ep>&& __u)
{
  reset(__u.release());
  get_deleter() = std::forward<_Ep>(__u.get_deleter());
  return *this;  // <-- GENERATES WARNING
}

**Call site:**

  case Message::STRUCT: {
MessageReader sub_reader(NULL);
if (reader->PopStruct(_reader)) {
  std::unique_ptr list_value(new base::ListValue);
  if (PopListElements(_reader, list_value.get()))
result = std::move(list_value);  // <-- GENERATES WARNING. |result| is 
a std::unique_ptr, so the std::move should be safe.
}
break;
  }

**Analysis trace:**

  In file included from 
../../build/linux/debian_wheezy_amd64-sysroot/usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/memory:85:
  
../../build/linux/debian_wheezy_amd64-sysroot/usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/bits/unique_ptr.h:190:4:
 warning: Potential memory leak
return *this;
^
  ../../dbus/values_util.cc:94:3: note: Control jumps to 'case STRUCT:'  at 
line 197
switch (reader->GetDataType()) {
^
  ../../dbus/values_util.cc:199:11: note: Assuming the condition is true
if (reader->PopStruct(_reader)) {
^~
  ../../dbus/values_util.cc:199:7: note: Taking true branch
if (reader->PopStruct(_reader)) {
^
  ../../dbus/values_util.cc:200:53: note: Memory is allocated
  std::unique_ptr list_value(new base::ListValue);
  ^~~
  ../../dbus/values_util.cc:201:9: note: Taking true branch
  if (PopListElements(_reader, list_value.get()))
  ^
  ../../dbus/values_util.cc:202:11: note: Calling 'unique_ptr::operator='
result = std::move(list_value);
^~
  
../../build/linux/debian_wheezy_amd64-sysroot/usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/bits/unique_ptr.h:190:4:
 note: Potential memory leak
return *this;
^


Repository:
  rL LLVM

https://reviews.llvm.org/D30593



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

[PATCH] D30593: Add correct "-isystem"/"-isysroot" warning handling to static analysis' BugReporter.

2017-03-06 Thread Kevin Marshall via Phabricator via cfe-commits 
kmarshall added a comment.

That makes sense. In the cases that I'm finding, the original call site is in 
application source code, but the warning is generated from inside a system 
header. So, I suppose the suppressions are working as intended, and we just 
need to add one-off suppressions for the known noisy sources? Where is the 
suppression database located? FWIW, there aren't many unique error sources - I 
only counted 9 after running the warnings through "uniq | sort".

Re: specifics, here the most common one that I'm seeing. It seems like a bogus 
buffer overrun warning?

  
../../build/linux/debian_wheezy_amd64-sysroot/usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/bits/locale_facets.tcc:1258:9:
 note: Assigned value is garbage or undefined
  *__s++ = *__first++;
 ^ ~~

The error is generated from this trivial logging line 
:

  LOG(FATAL) << "Unknown type: " << type;  // |type| is an enum

The offending C++ library code snippet is here (note the line w/comment 
"GENERATES WARNING")

  template
_CharT*
__add_grouping(_CharT* __s, _CharT __sep,
   const char* __gbeg, size_t __gsize,
   const _CharT* __first, const _CharT* __last)
{
  size_t __idx = 0;
  size_t __ctr = 0;
  
  while (__last - __first > __gbeg[__idx]
 && static_cast(__gbeg[__idx]) > 0
 && __gbeg[__idx] != __gnu_cxx::__numeric_traits::__max)
{
  __last -= __gbeg[__idx];
  __idx < __gsize - 1 ? ++__idx : ++__ctr;
}
  
  while (__first != __last)
*__s++ = *__first++;  // <-- GENERATES WARNING
  
  while (__ctr--)
{
  *__s++ = __sep; 
  for (char __i = __gbeg[__idx]; __i > 0; --__i)
*__s++ = *__first++;
}
  
  while (__idx--)
{
  *__s++ = __sep; 
  for (char __i = __gbeg[__idx]; __i > 0; --__i)
*__s++ = *__first++;
}
  
  return __s;
}

Here's a full snippet of the analysis trace:

  ../../dbus/message.cc:233:9: note: Calling 'basic_ostream::operator<<'
  LOG(FATAL) << "Unknown type: " << type;
  ^~
  ../../base/logging.h:414:23: note: expanded from macro 'LOG'
  #define LOG(severity) LAZY_STREAM(LOG_STREAM(severity), LOG_IS_ON(severity))
^
  ../../base/logging.h:402:62: note: expanded from macro 'LAZY_STREAM'
!(condition) ? (void) 0 : ::logging::LogMessageVoidify() & (stream)
   ^
  
../../build/linux/debian_wheezy_amd64-sysroot/usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/bits/ostream.tcc:113:11:
 note: Assuming '__fmt' is not equal to 'oct'
if (__fmt == ios_base::oct || __fmt == ios_base::hex)
^~
  
../../build/linux/debian_wheezy_amd64-sysroot/usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/bits/ostream.tcc:113:11:
 note: Left side of '||' is false
  
../../build/linux/debian_wheezy_amd64-sysroot/usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/bits/ostream.tcc:113:37:
 note: Assuming '__fmt' is not equal to 'hex'
if (__fmt == ios_base::oct || __fmt == ios_base::hex)
  ^~
  
../../build/linux/debian_wheezy_amd64-sysroot/usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/bits/ostream.tcc:113:7:
 note: Taking false branch
if (__fmt == ios_base::oct || __fmt == ios_base::hex)
^
  
../../build/linux/debian_wheezy_amd64-sysroot/usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/bits/ostream.tcc:116:9:
 note: Calling 'basic_ostream::_M_insert'
  return _M_insert(static_cast(__n));
 ^
  
../../build/linux/debian_wheezy_amd64-sysroot/usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/bits/ostream.tcc:69:2:
 note: Taking true branch
  if (__cerb)
  ^
  
../../build/linux/debian_wheezy_amd64-sysroot/usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/bits/ostream.tcc:75:7:
 note: Calling 'num_put::put'
  if (__np.put(*this, *this, this->fill(), __v).failed())
  ^
  
../../build/linux/debian_wheezy_amd64-sysroot/usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/bits/locale_facets.h:2336:16:
 note: Calling 'num_put::do_put'
{ return this->do_put(__s, __f, __fill, __v); }
 ^~~
  
../../build/linux/debian_wheezy_amd64-sysroot/usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/bits/locale_facets.h:2475:16:
 note: Calling 'num_put::_M_insert_int'
{ return _M_insert_int(__s, __io, __fill, __v); }