boostorg / locale

Boost.Locale
Boost Software License 1.0
32 stars 70 forks source link

Fix visibility of classes #81

Closed Flamefire closed 2 years ago

Flamefire commented 2 years ago

UBSAN on OSX reports some invalid downcasts, for example:

runtime error: downcast of address 0x7fdd33c08a40 which does not point to an object of type 'const boost::locale::info'
0x7fdd33c08a40: note: object is of type 'boost::locale::util::simple_info'
 dd 7f 00 00  28 ee fe 02 01 00 00 00  00 00 00 00 00 00 00 00  04 65 6e 00 ff 7f 00 00  10 b6 b2 93
              ^~~~~~~~~~~~~~~~~~~~~~~
              vptr for 'boost::locale::util::simple_info'
    #0 0x102edf7a3 in std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > boost::locale::conv::from_utf<char>(char const*, char const*, std::__1::locale const&, boost::locale::conv::method_type)+0x153 (test_std_formatting:x86_64+0x1000207a3)
    #1 0x102edf5e2 in std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > boost::locale::conv::from_utf<char>(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, std::__1::locale const&, boost::locale::conv::method_type)+0x62 (test_std_formatting:x86_64+0x1000205e2)
    #2 0x102ec6a2b in void test_by_char<char, char>(std::__1::locale const&, std::__1::locale const&)+0x88b (test_std_formatting:x86_64+0x100007a2b)
    #3 0x102ec3db7 in main+0xb27 (test_std_formatting:x86_64+0x100004db7)
    #4 0x7fff6d62acc8 in start+0x0 (libdyld.dylib:x86_64+0x1acc8)

Note that boost::locale::util::simple_info is a subclass of boost::locale::info, hence the cast is valid.

In most cases the classes were already correctly decorated with BOOST_LOCALE_DECL but as the whole class was defined inline the library and the executable linking to it may get different VTables (i.e. an own copy each which the runtime linker doesn't combine)

So far 2 solutions work:

@pdimov explained via Slack

number two is the correct fix, as it emits the vtable in the library without it, the vtable is emitted everywhere as a weak definition, and the one in the client isn't VISIBLE so the types are considered different by ubsan [number two] emits the vtable once, in the library, which is the right thing for classes that are supposed to reside in the library (what DECL implies)

Hence for most classes I made sure they are decorated with BOOST_LOCALE_DECL and have a (virtual) destructor defined in one of the libraries source files.

For template classes this doesn't work directly as defining a member requires defining the class specialization. Hence 2 solutions are used (I mostly kept what was used before):

There might be a better solution which avoids that C&P specializations by only instantiating the template and define the static data member specialization:

An explicit specialization of a static data member of a template is a definition if the declaration includes an initializer; otherwise, it is a declaration. These definitions must use braces for default initialization:

But I can't do that for std::locale::id as it isn't copyable and brace-init is C++11 but we still support C++98. Also I'm not sure what happens to the VTable in that case, i.e. how to define and correctly export/import the virtual destructor (and with it the VTable)

codecov[bot] commented 2 years ago

Codecov Report

Merging #81 (f89dccf) into develop (608e015) will decrease coverage by 0.03%. The diff coverage is 77.77%.

@@             Coverage Diff             @@
##           develop      #81      +/-   ##
===========================================
- Coverage    80.07%   80.04%   -0.04%     
===========================================
  Files           76       76              
  Lines         5958     5959       +1     
===========================================
- Hits          4771     4770       -1     
- Misses        1187     1189       +2     
Impacted Files Coverage Δ
include/boost/locale/conversion.hpp 100.00% <ø> (ø)
include/boost/locale/info.hpp 100.00% <ø> (ø)
include/boost/locale/util.hpp 0.00% <ø> (-12.00%) :arrow_down:
src/util/info.cpp 76.92% <ø> (ø)
src/shared/ids.cpp 88.88% <66.66%> (-11.12%) :arrow_down:
include/boost/locale/message.hpp 92.90% <100.00%> (-0.10%) :arrow_down:
src/util/codecvt_converter.cpp 71.92% <100.00%> (+0.16%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update dbeb8f1...f89dccf. Read the comment docs.