Fix -fsanitize=vptr badness in <__debug>

Summary: This patch fixes a lifetime bug when inserting a new container into the debug database. It is diagnosed by UBSAN when debug mode is enabled. This patch corrects how nodes are constructed during insertion. The fix requires unconditionally breaking the debug mode ABI. Users should not expect ABI stability from debug mode. Reviewers: ldionne, serge-sans-paille, EricWF Reviewed By: EricWF Subscribers: mclow.lists, christof, libcxx-commits Tags: #libc Differential Revision: https://reviews.llvm.org/D58011 llvm-svn: 355367
llvm · Mar 5, 2019 · 1c014d7 · 1c014d7
1 parent e69290d
commit 1c014d7
Show file tree

Hide file tree

Showing 8 changed files with 52 additions and 16 deletions.
diff --git a/libcxx/include/__debug b/libcxx/include/__debug
@@ -250,16 +250,22 @@ public:
     __db_c_const_iterator __c_end() const;
     __db_i_const_iterator __i_end() const;
 
+    typedef __c_node*(_InsertConstruct)(void*, void*, __c_node*);
+
+    template <class _Cont>
+    _LIBCPP_INLINE_VISIBILITY static __c_node* __create_C_node(void *__mem, void *__c, __c_node *__next) {
+        return ::new(__mem) _C_node<_Cont>(__c, __next);
+    }
+
     template <class _Cont>
     _LIBCPP_INLINE_VISIBILITY
     void __insert_c(_Cont* __c)
     {
-        __c_node* __n = __insert_c(static_cast<void*>(__c));
-        ::new(__n) _C_node<_Cont>(__n->__c_, __n->__next_);
+        __insert_c(static_cast<void*>(__c), &__create_C_node<_Cont>);
     }
 
     void __insert_i(void* __i);
-    __c_node* __insert_c(void* __c);
+    void __insert_c(void* __c, _InsertConstruct* __fn);
     void __erase_c(void* __c);
 
     void __insert_ic(void* __i, const void* __c);

diff --git a/libcxx/lib/abi/CHANGELOG.TXT b/libcxx/lib/abi/CHANGELOG.TXT
@@ -12,6 +12,32 @@ Afterwards the ABI list should be updated to include the new changes.
 
 New entries should be added directly below the "Version" header.
 
+-----------
+Version 9.0
+-----------
+
+* rTBD - Fix -fsanitize=vptr badness in <__debug>
+
+  This patch fixes a lifetime bug when inserting a new container into the debug database. It is
+  diagnosed by UBSAN when debug mode is enabled. This patch corrects how nodes are constructed
+  during insertion.
+
+  The fix requires unconditionally breaking the debug mode ABI. Users should not expect ABI
+  stability from debug mode.
+
+
+  x86_64-unknown-linux-gnu
+  ------------------------
+  Symbol added: _ZNSt3__111__libcpp_db10__insert_cEPvPFPNS_8__c_nodeES1_S1_S3_E
+  Symbol removed: _ZNSt3__111__libcpp_db10__insert_cEPv
+
+
+  x86_64-apple-apple-darwin
+  -------------------------
+  Symbol added: __ZNSt3__111__libcpp_db10__insert_cEPvPFPNS_8__c_nodeES1_S1_S3_E
+  Symbol removed: __ZNSt3__111__libcpp_db10__insert_cEPv
+
+
 -----------
 Version 8.0
 -----------

diff --git a/libcxx/lib/abi/x86_64-apple-darwin.v1.abilist b/libcxx/lib/abi/x86_64-apple-darwin.v1.abilist
@@ -599,7 +599,7 @@
 {'is_defined': True, 'name': '__ZNSt3__110to_wstringEx', 'type': 'FUNC'}
 {'is_defined': True, 'name': '__ZNSt3__110to_wstringEy', 'type': 'FUNC'}
 {'is_defined': True, 'name': '__ZNSt3__111__call_onceERVmPvPFvS2_E', 'type': 'FUNC'}
-{'is_defined': True, 'name': '__ZNSt3__111__libcpp_db10__insert_cEPv', 'type': 'FUNC'}
+{'is_defined': True, 'name': '__ZNSt3__111__libcpp_db10__insert_cEPvPFPNS_8__c_nodeES1_S1_S3_E', 'type': 'FUNC'}
 {'is_defined': True, 'name': '__ZNSt3__111__libcpp_db10__insert_iEPv', 'type': 'FUNC'}
 {'is_defined': True, 'name': '__ZNSt3__111__libcpp_db11__insert_icEPvPKv', 'type': 'FUNC'}
 {'is_defined': True, 'name': '__ZNSt3__111__libcpp_db15__iterator_copyEPvPKv', 'type': 'FUNC'}

diff --git a/libcxx/lib/abi/x86_64-apple-darwin.v2.abilist b/libcxx/lib/abi/x86_64-apple-darwin.v2.abilist
@@ -602,7 +602,7 @@
 {'is_defined': True, 'name': '__ZNSt3__210to_wstringEx', 'type': 'FUNC'}
 {'is_defined': True, 'name': '__ZNSt3__210to_wstringEy', 'type': 'FUNC'}
 {'is_defined': True, 'name': '__ZNSt3__211__call_onceERVmPvPFvS2_E', 'type': 'FUNC'}
-{'is_defined': True, 'name': '__ZNSt3__211__libcpp_db10__insert_cEPv', 'type': 'FUNC'}
+{'is_defined': True, 'name': '__ZNSt3__211__libcpp_db10__insert_cEPvPFPNS_8__c_nodeES1_S1_S3_E', 'type': 'FUNC'}
 {'is_defined': True, 'name': '__ZNSt3__211__libcpp_db10__insert_iEPv', 'type': 'FUNC'}
 {'is_defined': True, 'name': '__ZNSt3__211__libcpp_db11__insert_icEPvPKv', 'type': 'FUNC'}
 {'is_defined': True, 'name': '__ZNSt3__211__libcpp_db15__iterator_copyEPvPKv', 'type': 'FUNC'}

diff --git a/libcxx/lib/abi/x86_64-unknown-linux-gnu.v1.abilist b/libcxx/lib/abi/x86_64-unknown-linux-gnu.v1.abilist
@@ -512,7 +512,7 @@
 {'is_defined': True, 'name': '_ZNSt3__110to_wstringEx', 'type': 'FUNC'}
 {'is_defined': True, 'name': '_ZNSt3__110to_wstringEy', 'type': 'FUNC'}
 {'is_defined': True, 'name': '_ZNSt3__111__call_onceERVmPvPFvS2_E', 'type': 'FUNC'}
-{'is_defined': True, 'name': '_ZNSt3__111__libcpp_db10__insert_cEPv', 'type': 'FUNC'}
+{'is_defined': True, 'name': '_ZNSt3__111__libcpp_db10__insert_cEPvPFPNS_8__c_nodeES1_S1_S3_E', 'type': 'FUNC'}
 {'is_defined': True, 'name': '_ZNSt3__111__libcpp_db10__insert_iEPv', 'type': 'FUNC'}
 {'is_defined': True, 'name': '_ZNSt3__111__libcpp_db11__insert_icEPvPKv', 'type': 'FUNC'}
 {'is_defined': True, 'name': '_ZNSt3__111__libcpp_db15__iterator_copyEPvPKv', 'type': 'FUNC'}

diff --git a/libcxx/src/debug.cpp b/libcxx/src/debug.cpp
@@ -203,8 +203,8 @@ __libcpp_db::__insert_ic(void* __i, const void* __c)
     i->__c_ = c;
 }
 
-__c_node*
-__libcpp_db::__insert_c(void* __c)
+void
+__libcpp_db::__insert_c(void* __c, __libcpp_db::_InsertConstruct *__fn)
 {
 #ifndef _LIBCPP_HAS_NO_THREADS
     WLock _(mut());
@@ -234,15 +234,12 @@ __libcpp_db::__insert_c(void* __c)
     }
     size_t hc = hash<void*>()(__c) % static_cast<size_t>(__cend_ - __cbeg_);
     __c_node* p = __cbeg_[hc];
-    __c_node* r = __cbeg_[hc] =
-      static_cast<__c_node*>(malloc(sizeof(__c_node)));
-    if (__cbeg_[hc] == nullptr)
-        __throw_bad_alloc();
+    void *buf = malloc(sizeof(__c_node));
+    if (buf == nullptr)
+      __throw_bad_alloc();
+    __cbeg_[hc] = __fn(buf, __c, p);
 
-    r->__c_ = __c;
-    r->__next_ = p;
     ++__csz_;
-    return r;
 }
 
 void

diff --git a/libcxx/test/libcxx/debug/containers/db_sequence_container_iterators.pass.cpp b/libcxx/test/libcxx/debug/containers/db_sequence_container_iterators.pass.cpp
@@ -40,6 +40,7 @@ struct SequenceContainerChecks : BasicContainerChecks<Container, CT> {
   static void run() {
     Base::run();
     try {
+      SanityTest();
       FrontOnEmptyContainer();
 
       if constexpr (CT != CT_ForwardList) {
@@ -71,6 +72,12 @@ struct SequenceContainerChecks : BasicContainerChecks<Container, CT> {
   }
 
 private:
+  static void SanityTest() {
+    CHECKPOINT("sanity test");
+    Container C = {1, 1, 1, 1};
+    ::DoNotOptimize(&C);
+  }
+
   static void RemoveFirstElem() {
     // See llvm.org/PR35564
     CHECKPOINT("remove(<first-elem>)");

diff --git a/libcxx/utils/libcxx/test/config.py b/libcxx/utils/libcxx/test/config.py
@@ -939,7 +939,7 @@ def configure_sanitizer(self):
 
             def add_ubsan():
                 self.cxx.flags += ['-fsanitize=undefined',
-                                   '-fno-sanitize=vptr,function,float-divide-by-zero',
+                                   '-fno-sanitize=float-divide-by-zero',
                                    '-fno-sanitize-recover=all']
                 self.exec_env['UBSAN_OPTIONS'] = 'print_stacktrace=1'
                 self.config.available_features.add('ubsan')