[analyzer] Suppress false positives in std::shared_ptr

[dcoughlin]

Summary

The analyzer does not model C++ temporary destructors completely and so reports false alarms about leaks of memory allocated by the internals of shared_ptr:

    
std::shared_ptr<int> p(new int(1));
p = nullptr; // 'Potential leak of memory pointed to by field __cntrl_'

To avoid these spurious diagnostics, this patch suppresses all diagnostics where the end of the path is inside a method in std::shared_ptr.

It also reorganizes the tests for suppressions in the C++ standard library to use a separate simulated header for library functions with bugs that were deliberately inserted to test suppression. This will prevent other tests from using these as models.

rdar://problem/23652766

[zaks.anna]

LGTM

[NoQ]

test/Analysis/Inputs/system-header-simulator-cxx.h

249

You mean std::shared_ptr here?

Also, perhaps it's better to move this example into separate fake method; if this header emulator is used in another test, wouldn't the author of that test be surprised to see that operator=() is modeled that way?

[dcoughlin]

test/Analysis/Inputs/system-header-simulator-cxx.h

249

Yes, this is copy pasta!

249

Also, perhaps it's better to move this example into separate fake method; if this header emulator is used in another test, wouldn't the author of that test be surprised to see that operator=() is modeled that way?

We don't really model much in this this fake header, but I could move all the truly gross suppression stuff (basic string, _independent_bits_engine, shared_ptr) into a separate header and name it something that clearly indicates other tests shouldn't use it.

I'll update the patch.

[dcoughlin]

Address Artem's comments: fix a copy-pasta mistake and separate out std stubs with deliberate divide-by-zero bugs into their own simulated header file. This required moving the suppression tests into their own separate test.

[NoQ]

LGTM, all clear to me now :)

[dcoughlin]

This was committed in r274691. I forgot to add the Differential Revision line so phabricator didn't pick it up.

lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

if (Options.shouldSuppressFromCXXStandardLibrary()) {
       if (const CXXConstructorDecl *MD = dyn_cast<CXXConstructorDecl>(D)) {
         const CXXRecordDecl *CD = MD->getParent();
         if (CD->getName() == "__independent_bits_engine") {
           BR.markInvalid(getTag(), nullptr);
           return nullptr;
         }
       }
 
-      // The analyzer issues a false positive on
-      //   std::basic_string<uint8_t> v; v.push_back(1);
-      // and
-      //   std::u16string s; s += u'a';
-      // because we cannot reason about the internal invariants of the
-      // datastructure.
       for (const LocationContext *LCtx = N->getLocationContext(); LCtx;
            LCtx = LCtx->getParent()) {
         const CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(LCtx->getDecl());
         if (!MD)
           continue;
 
         const CXXRecordDecl *CD = MD->getParent();
+        // The analyzer issues a false positive on
+        //   std::basic_string<uint8_t> v; v.push_back(1);
+        // and
+        //   std::u16string s; s += u'a';
+        // because we cannot reason about the internal invariants of the
+        // datastructure.
         if (CD->getName() == "basic_string") {
           BR.markInvalid(getTag(), nullptr);
           return nullptr;
         }
+
+        // The analyzer issues a false positive on
+        //    std::shared_ptr<int> p(new int(1)); p = nullptr;
+        // because it does not reason properly about temporary destructors.
+        if (CD->getName() == "shared_ptr") {
+          BR.markInvalid(getTag(), nullptr);
+          return nullptr;
+        }
       }
     }
   }
 
   // Skip reports within the sys/queue.h macros as we do not have the ability to
   // reason about data structure shapes.
   SourceManager &SM = BRC.getSourceManager();
   FullSourceLoc Loc = BR.getLocation(SM).asLocation();

test/Analysis/Inputs/system-header-simulator-cxx-std-suppression.h

+// This is a fake system header with divide-by-zero bugs introduced in
+// c++ std library functions. We use these bugs to test hard-coded
+// suppression of diagnostics within standard library functions that are known
+// to produce false positives.
+
+#pragma clang system_header
+
+typedef unsigned char uint8_t;
+
+typedef __typeof__(sizeof(int)) size_t;
+void *memmove(void *s1, const void *s2, size_t n);
+
+namespace std {
+
+  template <class _Tp>
+  class allocator {
+  public:
+    void deallocate(void *p) {
+      ::delete p;
+    }
+  };
+
+  template <class _Alloc>
+  class allocator_traits {
+  public:
+    static void deallocate(void *p) {
+      _Alloc().deallocate(p);
+    }
+  };
+
+  template <class _Tp, class _Alloc>
+  class __list_imp
+  {};
+
+  template <class _Tp, class _Alloc = allocator<_Tp> >
+  class list
+  : private __list_imp<_Tp, _Alloc>
+  {
+  public:
+    void pop_front() {
+      // Fake use-after-free.
+      // No warning is expected as we are suppressing warning coming
+      // out of std::list.
+      int z = 0;
+      z = 5/z;
+    }
+    bool empty() const;
+  };
+
+  // basic_string
+  template<class _CharT, class _Alloc = allocator<_CharT> >
+  class __attribute__ ((__type_visibility__("default"))) basic_string {
+    bool isLong;
+    union {
+      _CharT localStorage[4];
+      _CharT *externalStorage;
+
+      void assignExternal(_CharT *newExternal) {
+        externalStorage = newExternal;
+      }
+    } storage;
+
+    typedef allocator_traits<_Alloc> __alloc_traits;
+
+  public:
+    basic_string();
+
+    void push_back(int c) {
+      // Fake error trigger.
+      // No warning is expected as we are suppressing warning coming
+      // out of std::basic_string.
+      int z = 0;
+      z = 5/z;
+    }
+
+    _CharT *getBuffer() {
+      return isLong ? storage.externalStorage : storage.localStorage;
+    }
+
+    basic_string &operator +=(int c) {
+      // Fake deallocate stack-based storage.
+      // No warning is expected as we are suppressing warnings within
+      // std::basic_string.
+      __alloc_traits::deallocate(getBuffer());
+    }
+
+    basic_string &operator =(const basic_string &other) {
+      // Fake deallocate stack-based storage, then use the variable in the
+      // same union.
+      // No warning is expected as we are suppressing warnings within
+      // std::basic_string.
+      __alloc_traits::deallocate(getBuffer());
+      storage.assignExternal(new _CharT[4]);
+    }
+  };
+
+template<class _Engine, class _UIntType>
+class __independent_bits_engine {
+public:
+  // constructors and seeding functions
+  __independent_bits_engine(_Engine& __e, size_t __w);
+};
+
+template<class _Engine, class _UIntType>
+__independent_bits_engine<_Engine, _UIntType>
+    ::__independent_bits_engine(_Engine& __e, size_t __w)
+{
+  // Fake error trigger.
+  // No warning is expected as we are suppressing warning coming
+  // out of std::__independent_bits_engine.
+  int z = 0;
+  z = 5/z;
+}
+
+#if __has_feature(cxx_decltype)
+typedef decltype(nullptr) nullptr_t;
+
+template<class _Tp>
+class shared_ptr
+{
+public:
+  constexpr shared_ptr(nullptr_t);
+  explicit shared_ptr(_Tp* __p);
+
+  shared_ptr(shared_ptr&& __r) { }
+
+  ~shared_ptr();
+
+  shared_ptr& operator=(shared_ptr&& __r) {
+    // Fake error trigger.
+    // No warning is expected as we are suppressing warning coming
+    // out of std::shared_ptr.
+    int z = 0;
+    z = 5/z;
+  }
+};
+
+template<class _Tp>
+inline
+constexpr
+shared_ptr<_Tp>::shared_ptr(nullptr_t) {
+}
+
+#endif // __has_feature(cxx_decltype)
+}
+

test/Analysis/Inputs/system-header-simulator-cxx.h

namespace std {
   }
 
   struct input_iterator_tag { };
   struct output_iterator_tag { };
   struct forward_iterator_tag : public input_iterator_tag { };
   struct bidirectional_iterator_tag : public forward_iterator_tag { };
   struct random_access_iterator_tag : public bidirectional_iterator_tag { };
 
-  template <class _Tp>
-  class allocator {
-  public:
-    void deallocate(void *p) {
-      ::delete p;
-    }
-  };
-
-  template <class _Alloc>
-  class allocator_traits {
-  public:
-    static void deallocate(void *p) {
-      _Alloc().deallocate(p);
-    }
-  };
-
-  template <class _Tp, class _Alloc>
-  class __list_imp
-  {};
-
-  template <class _Tp, class _Alloc = allocator<_Tp> >
-  class list
-  : private __list_imp<_Tp, _Alloc>
-  {
-  public:
-    void pop_front() {
-      // Fake use-after-free.
-      // No warning is expected as we are suppressing warning coming
-      // out of std::list.
-      int z = 0;
-      z = 5/z;
-    }
-    bool empty() const;
-  };
-
-  // basic_string
-  template<class _CharT, class _Alloc = allocator<_CharT> >
-  class __attribute__ ((__type_visibility__("default"))) basic_string {
-    bool isLong;
-    union {
-      _CharT localStorage[4];
-      _CharT *externalStorage;
-
-      void assignExternal(_CharT *newExternal) {
-        externalStorage = newExternal;
-      }
-    } storage;
-
-    typedef allocator_traits<_Alloc> __alloc_traits;
-
-  public:
-    basic_string();
-
-    void push_back(int c) {
-      // Fake error trigger.
-      // No warning is expected as we are suppressing warning coming
-      // out of std::basic_string.
-      int z = 0;
-      z = 5/z;
-    }
-
-    _CharT *getBuffer() {
-      return isLong ? storage.externalStorage : storage.localStorage;
-    }
-
-    basic_string &operator +=(int c) {
-      // Fake deallocate stack-based storage.
-      // No warning is expected as we are suppressing warnings within
-      // std::basic_string.
-      __alloc_traits::deallocate(getBuffer());
-    }
-
-    basic_string &operator =(const basic_string &other) {
-      // Fake deallocate stack-based storage, then use the variable in the
-      // same union.
-      // No warning is expected as we are suppressing warnings within
-      // std::basic_string.
-      __alloc_traits::deallocate(getBuffer());
-      storage.assignExternal(new _CharT[4]);
-    }
-  };
-
-template<class _Engine, class _UIntType>
-class __independent_bits_engine {
-public:
-  // constructors and seeding functions
-  __independent_bits_engine(_Engine& __e, size_t __w);
-};
-
-template<class _Engine, class _UIntType>
-__independent_bits_engine<_Engine, _UIntType>
-    ::__independent_bits_engine(_Engine& __e, size_t __w)
-{
-  // Fake error trigger.
-  // No warning is expected as we are suppressing warning coming
-  // out of std::basic_string.
-  int z = 0;
-  z = 5/z;
-}
-
 }
 
 void* operator new(std::size_t, const std::nothrow_t&) throw();
 void* operator new[](std::size_t, const std::nothrow_t&) throw();
 void operator delete(void*, const std::nothrow_t&) throw();
 void operator delete[](void*, const std::nothrow_t&) throw();
 
 void* operator new (std::size_t size, void* ptr) throw() { return ptr; };
 void* operator new[] (std::size_t size, void* ptr) throw() { return ptr; };
 void operator delete (void* ptr, void*) throw() {};
 void operator delete[] (void* ptr, void*) throw() {};

test/Analysis/diagnostics/implicit-cxx-std-suppression.cpp

+// RUN: %clang_cc1 -analyze -analyzer-checker=core,unix.Malloc,cplusplus.NewDelete,debug.ExprInspection -analyzer-config c++-container-inlining=true -analyzer-config c++-stdlib-inlining=false -std=c++11 -verify %s
+// RUN: %clang_cc1 -analyze -analyzer-checker=core,unix.Malloc,cplusplus.NewDelete,debug.ExprInspection -analyzer-config c++-container-inlining=true -analyzer-config c++-stdlib-inlining=true -std=c++11 -verify %s
+
+// expected-no-diagnostics
+
+#include "../Inputs/system-header-simulator-cxx-std-suppression.h"
+
+void testList_pop_front(std::list<int> list) {
+  while(!list.empty())
+    list.pop_front();  // no-warning
+}
+
+void testBasicStringSuppression() {
+  std::basic_string<uint8_t> v;
+  v.push_back(1); // no-warning
+}
+
+void testBasicStringSuppression_append() {
+  std::basic_string<char32_t> v;
+  v += 'c'; // no-warning
+}
+
+void testBasicStringSuppression_assign(std::basic_string<char32_t> &v,
+                                       const std::basic_string<char32_t> &v2) {
+  v = v2; // no-warning
+}
+
+class MyEngine;
+void testSuppression_independent_bits_engine(MyEngine& e) {
+  std::__independent_bits_engine<MyEngine, unsigned int> x(e, 64); // no-warning
+}
+
+void testSuppression_std_shared_pointer() {
+  std::shared_ptr<int> p(new int(1));
+
+  p = nullptr; // no-warning
+}

test/Analysis/inlining/stl.cpp

void testException(std::exception e) {
   const char *x = e.what();
   clang_analyzer_eval(x == 0);
 #if INLINE
   // expected-warning@-2 {{TRUE}}
 #else
   // expected-warning@-4 {{UNKNOWN}}
 #endif
 }
-
-void testList_pop_front(std::list<int> list) {
-  while(!list.empty())
-    list.pop_front();  // no-warning
-}
-
-void testBasicStringSuppression() {
-  std::basic_string<uint8_t> v;
-  v.push_back(1); // no-warning
-}
-
-void testBasicStringSuppression_append() {
-  std::basic_string<char32_t> v;
-  v += 'c'; // no-warning
-}
-
-void testBasicStringSuppression_assign(std::basic_string<char32_t> &v,
-                                       const std::basic_string<char32_t> &v2) {
-  v = v2;
-}
-
-class MyEngine;
-void testSupprerssion_independent_bits_engine(MyEngine& e) {
-  std::__independent_bits_engine<MyEngine, unsigned int> x(e, 64); // no-warning
-}