From 1c014d75b4cdcfab5cef304e5f9c5def96468751 Mon Sep 17 00:00:00 2001 From: Eric Fiselier Date: Tue, 5 Mar 2019 02:10:31 +0000 Subject: [PATCH] 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 --- libcxx/include/__debug | 12 ++++++--- libcxx/lib/abi/CHANGELOG.TXT | 26 +++++++++++++++++++ libcxx/lib/abi/x86_64-apple-darwin.v1.abilist | 2 +- libcxx/lib/abi/x86_64-apple-darwin.v2.abilist | 2 +- .../abi/x86_64-unknown-linux-gnu.v1.abilist | 2 +- libcxx/src/debug.cpp | 15 +++++------ .../db_sequence_container_iterators.pass.cpp | 7 +++++ libcxx/utils/libcxx/test/config.py | 2 +- 8 files changed, 52 insertions(+), 16 deletions(-) diff --git a/libcxx/include/__debug b/libcxx/include/__debug index 6ccb72cb8ad0e2..281cf6675c9bc4 100644 --- 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 + _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 _LIBCPP_INLINE_VISIBILITY void __insert_c(_Cont* __c) { - __c_node* __n = __insert_c(static_cast(__c)); - ::new(__n) _C_node<_Cont>(__n->__c_, __n->__next_); + __insert_c(static_cast(__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 index 95bdb714e682d2..60227e3248a39c 100644 --- 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 index 6349acf0534c61..e861fcab8b38bd 100644 --- 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 index adabbf6e033317..90ea1ebb0ec66a 100644 --- 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 index 8a31ff82ae9dda..3051d4cb72bce0 100644 --- 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 index 2e88b859be333b..28a1f70a59b02a 100644 --- 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()(__c) % static_cast(__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 index d05f9df3b98b96..6ead98ebcb797c 100644 --- 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 { static void run() { Base::run(); try { + SanityTest(); FrontOnEmptyContainer(); if constexpr (CT != CT_ForwardList) { @@ -71,6 +72,12 @@ struct SequenceContainerChecks : BasicContainerChecks { } private: + static void SanityTest() { + CHECKPOINT("sanity test"); + Container C = {1, 1, 1, 1}; + ::DoNotOptimize(&C); + } + static void RemoveFirstElem() { // See llvm.org/PR35564 CHECKPOINT("remove()"); diff --git a/libcxx/utils/libcxx/test/config.py b/libcxx/utils/libcxx/test/config.py index c619086b61d62c..6daf356ef9f2f8 100644 --- 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')