Fix -fsanitize=vptr badness in <__debug>

[EricWF]

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.

[mclow.lists]

include/__debug

253

When did using replace typedef? Pretty sure it was post C++11

src/debug.cpp

241

This also appears to fix a bug where a memory allocation failure would corrupt the debug database.

[EricWF]

include/__debug

253

using is C++11. But this header still pretends to have C++03 support so I'll write it as a typedef.

src/debug.cpp

241

Cool!

[EricWF]

Address review comments.

[ldionne]

The fix requires unconditionally breaking the debug mode ABI. Users should not expect ABI stability from debug mode.

FWIW, this is not an issue for us.

src/debug.cpp

241–242

Are those still required?

utils/libcxx/test/config.py

942

Nice!

[EricWF]

• Address review comments.

[EricWF]

src/debug.cpp

241–242

No. Good catch.

include/__debug

#endif
     class __db_c_iterator;
     class __db_c_const_iterator;
     class __db_i_iterator;
     class __db_i_const_iterator;
 
     __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);
     void __iterator_copy(void* __i, const void* __i0);
     void __erase_i(void* __i);
 
     void* __find_c_from_i(void* __i) const;
     void __invalidate_all(void* __c);

lib/abi/CHANGELOG.TXT

 ABI Changelog
 ==============
 This changelog contains information about ABI changes in libc++. Specifically
 the addition and deletion of symbols from the libc++ dylib.
 
 Each entry should start with the revision number followed by a description of
 the change. The entry should contain a summary of the ABI changes made,
 including what symbols were added, removed, or changed.
 
 To generate a summary use "sym_diff.py" diffing against the appropriate ABI list.
 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
 -----------
 
 * r347903 - Remove std::bad_array_length
 
   The change removes the definition of std::bad_array_length (which never made
   it into the standard) from the headers and the dylib. This is technically an
   ABI break because the symbols are shipped starting with mac OSX 10.13, however

lib/abi/x86_64-apple-darwin.v1.abilist

 {'is_defined': True, 'name': '__ZNSt3__110to_wstringEf', 'type': 'FUNC'}
 {'is_defined': True, 'name': '__ZNSt3__110to_wstringEi', 'type': 'FUNC'}
 {'is_defined': True, 'name': '__ZNSt3__110to_wstringEj', 'type': 'FUNC'}
 {'is_defined': True, 'name': '__ZNSt3__110to_wstringEl', 'type': 'FUNC'}
 {'is_defined': True, 'name': '__ZNSt3__110to_wstringEm', 'type': 'FUNC'}
 {'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'}
 {'is_defined': True, 'name': '__ZNSt3__111__libcpp_db16__invalidate_allEPv', 'type': 'FUNC'}
 {'is_defined': True, 'name': '__ZNSt3__111__libcpp_db4swapEPvS1_', 'type': 'FUNC'}
 {'is_defined': True, 'name': '__ZNSt3__111__libcpp_db9__erase_cEPv', 'type': 'FUNC'}
 {'is_defined': True, 'name': '__ZNSt3__111__libcpp_db9__erase_iEPv', 'type': 'FUNC'}
 {'is_defined': True, 'name': '__ZNSt3__111__libcpp_dbC1Ev', 'type': 'FUNC'}

lib/abi/x86_64-apple-darwin.v2.abilist

 {'is_defined': True, 'name': '__ZNSt3__210to_wstringEf', 'type': 'FUNC'}
 {'is_defined': True, 'name': '__ZNSt3__210to_wstringEi', 'type': 'FUNC'}
 {'is_defined': True, 'name': '__ZNSt3__210to_wstringEj', 'type': 'FUNC'}
 {'is_defined': True, 'name': '__ZNSt3__210to_wstringEl', 'type': 'FUNC'}
 {'is_defined': True, 'name': '__ZNSt3__210to_wstringEm', 'type': 'FUNC'}
 {'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'}
 {'is_defined': True, 'name': '__ZNSt3__211__libcpp_db16__invalidate_allEPv', 'type': 'FUNC'}
 {'is_defined': True, 'name': '__ZNSt3__211__libcpp_db4swapEPvS1_', 'type': 'FUNC'}
 {'is_defined': True, 'name': '__ZNSt3__211__libcpp_db9__erase_cEPv', 'type': 'FUNC'}
 {'is_defined': True, 'name': '__ZNSt3__211__libcpp_db9__erase_iEPv', 'type': 'FUNC'}
 {'is_defined': True, 'name': '__ZNSt3__211__libcpp_dbC1Ev', 'type': 'FUNC'}

lib/abi/x86_64-unknown-linux-gnu.v1.abilist

 {'is_defined': True, 'name': '_ZNSt3__110to_wstringEf', 'type': 'FUNC'}
 {'is_defined': True, 'name': '_ZNSt3__110to_wstringEi', 'type': 'FUNC'}
 {'is_defined': True, 'name': '_ZNSt3__110to_wstringEj', 'type': 'FUNC'}
 {'is_defined': True, 'name': '_ZNSt3__110to_wstringEl', 'type': 'FUNC'}
 {'is_defined': True, 'name': '_ZNSt3__110to_wstringEm', 'type': 'FUNC'}
 {'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'}
 {'is_defined': True, 'name': '_ZNSt3__111__libcpp_db16__invalidate_allEPv', 'type': 'FUNC'}
 {'is_defined': True, 'name': '_ZNSt3__111__libcpp_db4swapEPvS1_', 'type': 'FUNC'}
 {'is_defined': True, 'name': '_ZNSt3__111__libcpp_db9__erase_cEPv', 'type': 'FUNC'}
 {'is_defined': True, 'name': '_ZNSt3__111__libcpp_db9__erase_iEPv', 'type': 'FUNC'}
 {'is_defined': True, 'name': '_ZNSt3__111__libcpp_dbC1Ev', 'type': 'FUNC'}

src/debug.cpp

while (c->__c_ != __c)
         if (c == nullptr)
             return;
     }
     __i_node* i = __insert_iterator(__i);
     c->__add(i);
     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());
 #endif
     if (__csz_ + 1 > static_cast<size_t>(__cend_ - __cbeg_))
     {
         size_t nc = __next_prime(2*static_cast<size_t>(__cend_ - __cbeg_) + 1);
         __c_node** cbeg = static_cast<__c_node**>(calloc(nc, sizeof(void*)));
if (__csz_ + 1 > static_cast<size_t>(__cend_ - __cbeg_))
             }
         }
         free(__cbeg_);
         __cbeg_ = cbeg;
         __cend_ = __cbeg_ + nc;
     }
     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)
+    __cbeg_[hc] = __fn(buf, __c, p);
 
-    r->__c_ = __c;
-    r->__next_ = p;
     ++__csz_;
-    return r;
 }
 
 void
 __libcpp_db::__erase_i(void* __i)
 {
 #ifndef _LIBCPP_HAS_NO_THREADS
     WLock _(mut());
 #endif

test/libcxx/debug/containers/db_sequence_container_iterators.pass.cpp

struct SequenceContainerChecks : BasicContainerChecks<Container, CT> {
   using const_iterator = typename Container::const_iterator;
 
   using Base::makeContainer;
   using Base::makeValueType;
 public:
   static void run() {
     Base::run();
     try {
+      SanityTest();
       FrontOnEmptyContainer();
 
       if constexpr (CT != CT_ForwardList) {
         AssignInvalidates();
         BackOnEmptyContainer();
         InsertIterValue();
         InsertIterSizeValue();
         InsertIterIterIter();
try {
         SpliceFirstElem();
       }
     } catch (...) {
       assert(false && "uncaught debug exception");
     }
   }
 
 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>)");
     {
       Container C = makeContainer(1);
       auto FirstVal = *(C.begin());
       C.remove(FirstVal);
       assert(C.empty());

utils/libcxx/test/config.py

def configure_sanitizer(self):
                 symbolizer_search_paths = (
                     os.path.dirname(cxx_path) +
                     os.pathsep + symbolizer_search_paths)
             llvm_symbolizer = libcxx.util.which('llvm-symbolizer',
                                                 symbolizer_search_paths)
 
             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')
 
             # Setup the sanitizer compile flags
             self.cxx.flags += ['-g', '-fno-omit-frame-pointer']
             if san == 'Address' or san == 'Address;Undefined' or san == 'Undefined;Address':
                 self.cxx.flags += ['-fsanitize=address']