From 5efc81166d869a94318134bbb1878b551503d115 Mon Sep 17 00:00:00 2001 From: Louis Dionne Date: Wed, 21 Dec 2022 10:08:54 -0500 Subject: [PATCH] [libc++] Remove HIDE_FROM_ABI from virtual functions _LIBCPP_HIDE_FROM_ABI (which is what _LIBCPP_INLINE_VISIBILITY is) uses ABI tags to avoid ODR violations when linking together object files compiled against different versions of libc++. However, pointer authentication uses the mangled name of the function to sign the function pointer in the vtable, which means that the ABI tag effectively changes how the pointers are signed. This leads to PAC failures when passing an object that holds one of these pointers in its vtable across an ABI boundary: one side will sign the pointer using one function mangling (with one ABI tag), and the other side will authenticate the pointer expecting it to have a different mangled name, which won't work. To make sure this does not regress in the future, this patch also adds a clang-query test to detect incorrect applications of _LIBCPP_HIDE_FROM_ABI. Differential Revision: https://reviews.llvm.org/D140453 --- libcxx/include/__config | 10 +++++ libcxx/include/__filesystem/filesystem_error.h | 2 +- libcxx/include/__functional/function.h | 2 +- libcxx/include/__memory/shared_ptr.h | 4 +- libcxx/include/future | 2 +- libcxx/include/locale | 48 +++++++++------------- libcxx/include/regex | 6 +-- libcxx/test/libcxx/clang_query.sh.cpp | 4 ++ .../libcxx/clang_query/abi_tag_on_virtual.query | 29 +++++++++++++ 9 files changed, 71 insertions(+), 36 deletions(-) create mode 100644 libcxx/test/libcxx/clang_query/abi_tag_on_virtual.query diff --git a/libcxx/include/__config b/libcxx/include/__config index 9f7aadd..1a519b9 100644 --- a/libcxx/include/__config +++ b/libcxx/include/__config @@ -610,6 +610,15 @@ typedef __char32_t char32_t; // Note that we use _LIBCPP_EXCLUDE_FROM_EXPLICIT_INSTANTIATION to ensure that we don't depend // on _LIBCPP_HIDE_FROM_ABI methods of classes explicitly instantiated in the dynamic library. // +// Also note that the _LIBCPP_HIDE_FROM_ABI_VIRTUAL macro should be used on virtual functions +// instead of _LIBCPP_HIDE_FROM_ABI. That macro does not use an ABI tag. Indeed, the mangled +// name of a virtual function is part of its ABI, since some architectures like arm64e can sign +// the virtual function pointer in the vtable based on the mangled name of the function. Since +// we use an ABI tag that changes with each released version, the mangled name of the virtual +// function would change, which is incorrect. Note that it doesn't make much sense to change +// the implementation of a virtual function in an ABI-incompatible way in the first place, +// since that would be an ABI break anyway. Hence, the lack of ABI tag should not be noticeable. +// // TODO: We provide a escape hatch with _LIBCPP_NO_ABI_TAG for folks who want to avoid increasing // the length of symbols with an ABI tag. In practice, we should remove the escape hatch and // use compression mangling instead, see https://github.com/itanium-cxx-abi/cxx-abi/issues/70. @@ -620,6 +629,7 @@ typedef __char32_t char32_t; # else # define _LIBCPP_HIDE_FROM_ABI _LIBCPP_HIDDEN _LIBCPP_EXCLUDE_FROM_EXPLICIT_INSTANTIATION # endif +# define _LIBCPP_HIDE_FROM_ABI_VIRTUAL _LIBCPP_HIDDEN _LIBCPP_EXCLUDE_FROM_EXPLICIT_INSTANTIATION # ifdef _LIBCPP_BUILDING_LIBRARY # if _LIBCPP_ABI_VERSION > 1 diff --git a/libcxx/include/__filesystem/filesystem_error.h b/libcxx/include/__filesystem/filesystem_error.h index eb5c38a..effe699 100644 --- a/libcxx/include/__filesystem/filesystem_error.h +++ b/libcxx/include/__filesystem/filesystem_error.h @@ -61,7 +61,7 @@ public: filesystem_error(const filesystem_error&) = default; ~filesystem_error() override; // key function - _LIBCPP_INLINE_VISIBILITY + _LIBCPP_HIDE_FROM_ABI_VIRTUAL const char* what() const noexcept override { return __storage_->__what_.c_str(); } diff --git a/libcxx/include/__functional/function.h b/libcxx/include/__functional/function.h index 436149e..8f34d01 100644 --- a/libcxx/include/__functional/function.h +++ b/libcxx/include/__functional/function.h @@ -262,7 +262,7 @@ class __base<_Rp(_ArgTypes...)> __base& operator=(const __base&); public: _LIBCPP_INLINE_VISIBILITY __base() {} - _LIBCPP_INLINE_VISIBILITY virtual ~__base() {} + _LIBCPP_HIDE_FROM_ABI_VIRTUAL virtual ~__base() {} virtual __base* __clone() const = 0; virtual void __clone(__base*) const = 0; virtual void destroy() _NOEXCEPT = 0; diff --git a/libcxx/include/__memory/shared_ptr.h b/libcxx/include/__memory/shared_ptr.h index e200a95..e598ad2 100644 --- a/libcxx/include/__memory/shared_ptr.h +++ b/libcxx/include/__memory/shared_ptr.h @@ -991,7 +991,7 @@ struct __unbounded_array_control_block<_Tp[], _Alloc> : __shared_weak_count return (__bytes + __align - 1) & ~(__align - 1); } - _LIBCPP_HIDE_FROM_ABI + _LIBCPP_HIDE_FROM_ABI_VIRTUAL ~__unbounded_array_control_block() override { } // can't be `= default` because of the sometimes-non-trivial union member __data_ private: @@ -1058,7 +1058,7 @@ struct __bounded_array_control_block<_Tp[_Count], _Alloc> std::__uninitialized_allocator_value_construct_n(__alloc_, std::addressof(__data_[0]), _Count); } - _LIBCPP_HIDE_FROM_ABI + _LIBCPP_HIDE_FROM_ABI_VIRTUAL ~__bounded_array_control_block() override { } // can't be `= default` because of the sometimes-non-trivial union member __data_ private: diff --git a/libcxx/include/future b/libcxx/include/future index a4747cd..dcacd7a 100644 --- a/libcxx/include/future +++ b/libcxx/include/future @@ -1626,7 +1626,7 @@ class _LIBCPP_AVAILABILITY_FUTURE __packaged_task_base<_Rp(_ArgTypes...)> public: _LIBCPP_INLINE_VISIBILITY __packaged_task_base() {} - _LIBCPP_INLINE_VISIBILITY + _LIBCPP_HIDE_FROM_ABI_VIRTUAL virtual ~__packaged_task_base() {} virtual void __move_to(__packaged_task_base*) _NOEXCEPT = 0; virtual void destroy() = 0; diff --git a/libcxx/include/locale b/libcxx/include/locale index f322a11..874866f 100644 --- a/libcxx/include/locale +++ b/libcxx/include/locale @@ -680,7 +680,7 @@ public: static locale::id id; protected: - _LIBCPP_HIDE_FROM_ABI ~num_get() override {} + _LIBCPP_HIDE_FROM_ABI_VIRTUAL ~num_get() override {} template _LIBCPP_METHOD_TEMPLATE_IMPLICIT_INSTANTIATION_VIS @@ -1350,7 +1350,7 @@ public: static locale::id id; protected: - _LIBCPP_HIDE_FROM_ABI ~num_put() override {} + _LIBCPP_HIDE_FROM_ABI_VIRTUAL ~num_put() override {} virtual iter_type do_put(iter_type __s, ios_base& __iob, char_type __fl, bool __v) const; @@ -1795,7 +1795,7 @@ public: static locale::id id; protected: - _LIBCPP_HIDE_FROM_ABI ~time_get() override {} + _LIBCPP_HIDE_FROM_ABI_VIRTUAL ~time_get() override {} virtual dateorder do_date_order() const; virtual iter_type do_get_time(iter_type __b, iter_type __e, ios_base& __iob, @@ -2414,25 +2414,17 @@ public: __time_get_storage<_CharT>(__nm) {} protected: - _LIBCPP_HIDE_FROM_ABI ~time_get_byname() override {} + _LIBCPP_HIDE_FROM_ABI_VIRTUAL ~time_get_byname() override {} - _LIBCPP_INLINE_VISIBILITY - dateorder do_date_order() const override {return this->__do_date_order();} + _LIBCPP_HIDE_FROM_ABI_VIRTUAL dateorder do_date_order() const override {return this->__do_date_order();} private: - _LIBCPP_INLINE_VISIBILITY - const string_type* __weeks() const override {return this->__weeks_;} - _LIBCPP_INLINE_VISIBILITY - const string_type* __months() const override {return this->__months_;} - _LIBCPP_INLINE_VISIBILITY - const string_type* __am_pm() const override {return this->__am_pm_;} - _LIBCPP_INLINE_VISIBILITY - const string_type& __c() const override {return this->__c_;} - _LIBCPP_INLINE_VISIBILITY - const string_type& __r() const override {return this->__r_;} - _LIBCPP_INLINE_VISIBILITY - const string_type& __x() const override {return this->__x_;} - _LIBCPP_INLINE_VISIBILITY - const string_type& __X() const override {return this->__X_;} + _LIBCPP_HIDE_FROM_ABI_VIRTUAL const string_type* __weeks() const override {return this->__weeks_;} + _LIBCPP_HIDE_FROM_ABI_VIRTUAL const string_type* __months() const override {return this->__months_;} + _LIBCPP_HIDE_FROM_ABI_VIRTUAL const string_type* __am_pm() const override {return this->__am_pm_;} + _LIBCPP_HIDE_FROM_ABI_VIRTUAL const string_type& __c() const override {return this->__c_;} + _LIBCPP_HIDE_FROM_ABI_VIRTUAL const string_type& __r() const override {return this->__r_;} + _LIBCPP_HIDE_FROM_ABI_VIRTUAL const string_type& __x() const override {return this->__x_;} + _LIBCPP_HIDE_FROM_ABI_VIRTUAL const string_type& __X() const override {return this->__X_;} }; extern template class _LIBCPP_EXTERN_TEMPLATE_TYPE_VIS time_get_byname; @@ -2482,7 +2474,7 @@ public: static locale::id id; protected: - _LIBCPP_HIDE_FROM_ABI ~time_put() override {} + _LIBCPP_HIDE_FROM_ABI_VIRTUAL ~time_put() override {} virtual iter_type do_put(iter_type __s, ios_base&, char_type, const tm* __tm, char __fmt, char __mod) const; @@ -2570,7 +2562,7 @@ public: : time_put<_CharT, _OutputIterator>(__nm, __refs) {} protected: - _LIBCPP_HIDE_FROM_ABI ~time_put_byname() override {} + _LIBCPP_HIDE_FROM_ABI_VIRTUAL ~time_put_byname() override {} }; extern template class _LIBCPP_EXTERN_TEMPLATE_TYPE_VIS time_put_byname; @@ -2618,7 +2610,7 @@ public: static const bool intl = _International; protected: - _LIBCPP_HIDE_FROM_ABI ~moneypunct() override {} + _LIBCPP_HIDE_FROM_ABI_VIRTUAL ~moneypunct() override {} virtual char_type do_decimal_point() const {return numeric_limits::max();} virtual char_type do_thousands_sep() const {return numeric_limits::max();} @@ -2668,7 +2660,7 @@ public: : moneypunct<_CharT, _International>(__refs) {init(__nm.c_str());} protected: - _LIBCPP_HIDE_FROM_ABI ~moneypunct_byname() override {} + _LIBCPP_HIDE_FROM_ABI_VIRTUAL ~moneypunct_byname() override {} char_type do_decimal_point() const override {return __decimal_point_;} char_type do_thousands_sep() const override {return __thousands_sep_;} @@ -2796,7 +2788,7 @@ public: static locale::id id; protected: - _LIBCPP_HIDE_FROM_ABI ~money_get() override {} + _LIBCPP_HIDE_FROM_ABI_VIRTUAL ~money_get() override {} virtual iter_type do_get(iter_type __b, iter_type __e, bool __intl, ios_base& __iob, ios_base::iostate& __err, @@ -3340,7 +3332,7 @@ public: static locale::id id; protected: - _LIBCPP_HIDE_FROM_ABI ~money_put() override {} + _LIBCPP_HIDE_FROM_ABI_VIRTUAL ~money_put() override {} virtual iter_type do_put(iter_type __s, bool __intl, ios_base& __iob, char_type __fl, long double __units) const; @@ -3508,7 +3500,7 @@ public: static locale::id id; protected: - _LIBCPP_HIDE_FROM_ABI ~messages() override {} + _LIBCPP_HIDE_FROM_ABI_VIRTUAL ~messages() override {} virtual catalog do_open(const basic_string&, const locale&) const; virtual string_type do_get(catalog, int __set, int __msgid, @@ -3597,7 +3589,7 @@ public: : messages<_CharT>(__refs) {} protected: - _LIBCPP_HIDE_FROM_ABI ~messages_byname() override {} + _LIBCPP_HIDE_FROM_ABI_VIRTUAL ~messages_byname() override {} }; extern template class _LIBCPP_EXTERN_TEMPLATE_TYPE_VIS messages_byname; diff --git a/libcxx/include/regex b/libcxx/include/regex index 365a4c9..06c017f 100644 --- a/libcxx/include/regex +++ b/libcxx/include/regex @@ -1446,12 +1446,12 @@ public: _LIBCPP_INLINE_VISIBILITY __node() {} - _LIBCPP_INLINE_VISIBILITY + _LIBCPP_HIDE_FROM_ABI_VIRTUAL virtual ~__node() {} - _LIBCPP_INLINE_VISIBILITY + _LIBCPP_HIDE_FROM_ABI_VIRTUAL virtual void __exec(__state&) const {} - _LIBCPP_INLINE_VISIBILITY + _LIBCPP_HIDE_FROM_ABI_VIRTUAL virtual void __exec_split(bool, __state&) const {} }; diff --git a/libcxx/test/libcxx/clang_query.sh.cpp b/libcxx/test/libcxx/clang_query.sh.cpp index 7044459..d59d6a2 100644 --- a/libcxx/test/libcxx/clang_query.sh.cpp +++ b/libcxx/test/libcxx/clang_query.sh.cpp @@ -17,6 +17,10 @@ // RUN: cat %t.output // RUN: cat %t.output | wc -l | grep -Fxq 1 +// RUN: clang-query -f %S/clang_query/abi_tag_on_virtual.query %s --use-color -- -Wno-unknown-warning-option %{compile_flags} -fno-modules > %t.output +// RUN: cat %t.output +// RUN: cat %t.output | wc -l | grep -Fxq 1 + // Prevent from generating deprecated warnings for this test. #if defined(__DEPRECATED) # undef __DEPRECATED diff --git a/libcxx/test/libcxx/clang_query/abi_tag_on_virtual.query b/libcxx/test/libcxx/clang_query/abi_tag_on_virtual.query new file mode 100644 index 0000000..ffa465a --- /dev/null +++ b/libcxx/test/libcxx/clang_query/abi_tag_on_virtual.query @@ -0,0 +1,29 @@ +#===------------------------------------------------------------------------===# +# +# Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +# See https://llvm.org/LICENSE.txt for license information. +# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +# +#===------------------------------------------------------------------------===# + +# This clang-query test ensures that we don't place an abi_tag attribute on +# virtual functions. This can happen by mistakenly applying a macro like +# _LIBCPP_HIDE_FROM_ABI on a virtual function. +# +# The problem is that arm64e pointer authentication extensions use the mangled +# name of the function to sign the function pointer in the vtable, which means +# that the ABI tag effectively influences how the pointers are signed. +# +# This can lead to PAC failures when passing an object that holds one of these +# pointers in its vtable across an ABI boundary if the two sides have been compiled +# with different versions of libc++: one side will sign the pointer using one function +# mangling (with one ABI tag), and the other side will authenticate the pointer expecting +# it to have a different mangled name due to the ABI tag being different, which will crash. +# +# This test ensures that we don't re-introduce this issue in the code base. + +match +cxxMethodDecl(isVirtual(), + hasAttr("attr::AbiTag"), + unless(isExpansionInSystemHeader()) + ) -- 2.7.4