Even on ABIs that normally unique typeinfo names, it is easy to end up in situations where that fails. Consider a shared library implemented with lib.hh: struct foo { virtual ~foo(){} }; struct bar : public foo { virtual void zed(); }; lib.cc: #include "lib.hh" void bar::zed() {} and being used by the program (could be another library): test.cc: #include "lib.hh" int main(int argc, char** argv) { bar t; } if the program is compiled with -fvisibility=hidden, it will have a hidden _ZTI3foo which isDerivedFromAtOffset will think doesn't match the _ZTI3foo in the library. The above test is a reduction of #include <boost/test/unit_test.hpp> int main(int argc, char **argv) { return 0; } compiled with -fvisibility=hidden, which complains that /usr/include/boost/test/unit_test_log.hpp:112:23: runtime error: member call on address 0x000006583060 which does not point to an object of type 'test_observer'
I can confirm that: $ g++ lib.cc -shared -fPIC -o libx.so -fsanitize=undefined && g++ test.cc -lx -L. -fvisibility=hidden -fsanitize=undefined && ./a.out 3bar (0x7ffff7ff2007), 3bar (0x7ffff7ff2007) 3bar (0x7ffff7ff2007), 3foo (0x402048) 3foo (0x7ffff7ff200c), 3foo (0x402048) lib.hh:4:8: runtime error: member call on address 0x7fffffffdc38 which does not point to an object of type 'foo' 0x7fffffffdc38: note: object is of type 'bar' ff 7f 00 00 50 3d 40 00 00 00 00 00 70 14 40 00 00 00 00 00 eb 6f cf f6 ff 7f 00 00 68 ff ff ff ^~~~~~~~~~~~~~~~~~~~~~~ vptr for 'bar' with the suggested patch: $ g++ lib.cc -shared -fPIC -o libx.so -fsanitize=undefined && g++ test.cc -lx -L. -fsanitize=undefined && ./a.out 3bar (0x7ffff7ff2007), 3bar (0x7ffff7ff2007) 3bar (0x7ffff7ff2007), 3foo (0x402048) 3foo (0x402048), 3foo (0x402048) Jakub do you prefer to come up with a run-time option or hard-code SANITIZER_NON_UNIQUE_TYPEINFO == 1? Btw. llvm looks to survive the test, the difference I see in nm is: GCC: $ nm -C a.out | grep 'typeinfo name' 0000000000402048 V typeinfo name for foo $ nm -C libx.so | grep 'typeinfo name' 0000000000002007 V typeinfo name for bar 000000000000200c V typeinfo name for foo clang7: $ nm -C a.out | grep 'typeinfo name' 000000000042ca70 V typeinfo name for foo 000000000042c9f7 V typeinfo name for int (int, char**) $ nm -C libx.so | grep 'typeinfo name' 000000000000201c R typeinfo name for bar 0000000000002034 V typeinfo name for foo
Started with r241977 when merge from trunk was done.
https://reviews.llvm.org/D11502
Given that the GCC default is #ifndef __GXX_MERGED_TYPEINFO_NAMES // By default, typeinfo names are not merged. #define __GXX_MERGED_TYPEINFO_NAMES 0 #endif we should use strcmp on Linux too. Note, what libsanitizer does doesn't match what typeinfo does, which is: return ((__name == __arg.__name) || (__name[0] != '*' && __builtin_strcmp (__name, __arg.__name) == 0)); for equality and { return (__name[0] == '*' && __arg.__name[0] == '*') ? __name < __arg.__name : __builtin_strcmp (__name, __arg.__name) < 0; } for before.
(In reply to Jakub Jelinek from comment #4) > Given that the GCC default is > #ifndef __GXX_MERGED_TYPEINFO_NAMES > // By default, typeinfo names are not merged. > #define __GXX_MERGED_TYPEINFO_NAMES 0 > #endif > we should use strcmp on Linux too. > Note, what libsanitizer does doesn't match what typeinfo does, which is: > return ((__name == __arg.__name) > || (__name[0] != '*' && > __builtin_strcmp (__name, __arg.__name) == 0)); > for equality and > { return (__name[0] == '*' && __arg.__name[0] == '*') > ? __name < __arg.__name > : __builtin_strcmp (__name, __arg.__name) < 0; } > for before. Do you suggest to propose an upstream change where I'll enable that for Linux and I'll also adjust the equality operator? Or do we prefer to adjust that locally GCC project?
I'd check what would they say about upstream change and if they are strongly against, make a local change. Even when clang++ is used with libstdc++ the default is still strcmp, dunno about libc++.
(In reply to Jakub Jelinek from comment #6) > I'd check what would they say about upstream change and if they are strongly > against, make a local change. Even when clang++ is used with libstdc++ the > default is still strcmp, dunno about libc++. I'll prepare patch for it. Btw. according to git grep: $ git grep __GXX_MERGED_TYPEINFO_NAMES gcc/config/arm/symbian.h: builtin_define ("__GXX_MERGED_TYPEINFO_NAMES=0"); \ gcc/config/i386/cygming.h: builtin_define ("__GXX_MERGED_TYPEINFO_NAMES=0"); \ gcc/config/m68k/uclinux.h: builtin_define ("__GXX_MERGED_TYPEINFO_NAMES=0"); \ libstdc++-v3/doc/html/manual/api.html:in <code class="filename">typeinfo</code>, <code class="literal">__GXX_MERGED_TYPEINFO_NAMES</code> libstdc++-v3/doc/xml/manual/evolution.xml:in <filename class="headerfile">typeinfo</filename>, <literal>__GXX_MERGED_TYPEINFO_NAMES</literal> libstdc++-v3/libsupc++/tinfo.cc:#if __GXX_MERGED_TYPEINFO_NAMES libstdc++-v3/libsupc++/tinfo2.cc:#if __GXX_MERGED_TYPEINFO_NAMES libstdc++-v3/libsupc++/typeinfo:// __GXX_MERGED_TYPEINFO_NAMES to 1 or 0 to indicate whether or not pointer libstdc++-v3/libsupc++/typeinfo:#ifndef __GXX_MERGED_TYPEINFO_NAMES libstdc++-v3/libsupc++/typeinfo:#define __GXX_MERGED_TYPEINFO_NAMES 0 libstdc++-v3/libsupc++/typeinfo: #if !__GXX_MERGED_TYPEINFO_NAMES libstdc++-v3/libsupc++/typeinfo:# if !__GXX_MERGED_TYPEINFO_NAMES There are 3 header files that define __GXX_MERGED_TYPEINFO_NAMES=0 (which is default): can that be removed? And as __GXX_MERGED_TYPEINFO_NAMES is default, should I create a patch that will always use strcmp in libsanitizer?
I created upstream patch candidate: https://reviews.llvm.org/D56485
Why is this actually considered a regression? While in GCC 6 and older libsanitizer didn't have SANITIZER_NON_UNIQUE_TYPEINFO macro, it performed only == comparisons of the string pointers, so in the end it acted as if the current default of SANITIZER_NON_UNIQUE_TYPEINFO. Not suitable for libstdc++, sure, but not a regression. That said, I'm willing to ack it for GCC9 even then if upstream comes up with something or if they don't care, eventually as a GCC only tweak.
> That said, I'm willing to ack it for GCC9 even then if upstream comes up > with something or if they don't care, eventually as a GCC only tweak. Works for me. Note that so far there has been no reply to my patch.
(In reply to Martin Liška from comment #10) > > That said, I'm willing to ack it for GCC9 even then if upstream comes up > > with something or if they don't care, eventually as a GCC only tweak. > > Works for me. Note that so far there has been no reply to my patch. You might want to CC: Filipe Cabecinhas <me@filcab.net>
(In reply to Rafael Avila de Espindola from comment #11) > (In reply to Martin Liška from comment #10) > > > That said, I'm willing to ack it for GCC9 even then if upstream comes up > > > with something or if they don't care, eventually as a GCC only tweak. > > > > Works for me. Note that so far there has been no reply to my patch. > > You might want to CC: > Filipe Cabecinhas <me@filcab.net> I've just done that, thanks.
Author: marxin Date: Wed Mar 6 11:46:15 2019 New Revision: 269419 URL: https://gcc.gnu.org/viewcvs?rev=269419&root=gcc&view=rev Log: Charry pick libsanitizer r355488 (PR sanitizer/88684). 2019-03-06 Martin Liska <mliska@suse.cz> PR sanitizer/88684 * sanitizer_common/sanitizer_platform.h (defined): Cherry pick. (SANITIZER_NON_UNIQUE_TYPEINFO): Likewise. * ubsan/ubsan_type_hash_itanium.cc (isDerivedFromAtOffset): Likewise. Modified: trunk/libsanitizer/ChangeLog trunk/libsanitizer/sanitizer_common/sanitizer_platform.h trunk/libsanitizer/ubsan/ubsan_type_hash_itanium.cc
Fixed on trunk, not planning to backport that.