ubsan: check type_info equality via strcmp

[Richard Smith]

On Fri, Aug 16, 2013 at 2:38 AM, Stephan Bergmann <sbergman at redhat.com>wrote:

> I was trying out -fsanitizer=undefined on the LibreOffice code base (with
> a recent Clang trunk build), and ran into false positives claiming member
> calls were made on base class subobjects of wrong type.
>
> Turns out LibreOffice is somewhat over-eager with -fvisibility=hidden,
> leading to some classes that are used across multiple dynamic libraries
> nevertheless having their RTTI name symbols bound locally per library.
> While that is in violation of the Itanium ABI, it at least works with
> recent libstdc++ (which resorts to strcmp rather than pointer comparison
> these days, see [1])---and wouldn't normally get noticed anyway as there
> should not be uses of dynamic_cast involving those classes in the
> LibreOffice code base.
>

I would much prefer for this to be first resolved in the Itanium ABI. The
fix you suggest below will hide real problems in code that uses libc++
(where type_info comparison directly compares the __type_name fields).

>From a more abstract POV, I think libstdc++ may have been misguided in
trying to make the RTLD_LOCAL case "work". For instance, if I load two
versions of the same DSO into my binary using RTLD_LOCAL, and use their
type_info*s as keys in some container type (say, in a serialization
library), there is no way to avoid a broken program. Perhaps it would have
been better for them to admit that types with the same name from two
different RTLD_LOCAL DSOs are different, just like every other entity.

Also, it seems to me that you've found a real LibreOffice bug, if they're
hiding the type information for exported types, and I don't want to weaken
a sanitizer because it found a real bug.

If we want to match g++, we should:
 * Change the Itanium ABI to use g++/libstdc++'s strategy (that is, prefix
the string with '*' if pointer comparison should be used, and use string
comparison for externally-visible types)
 * Teach clang to emit the '*' prefix on non-externally-visible types
 * Teach libc++ to perform string comparisons if the type string doesn't
start with '*'
 * Teach ubsan to perform string comparisons if the type string doesn't
start with '*'

Another approach I'd be comfortable with would be to perform a second pass
looking for a string match if the pointer match fails, and to emit a
differently-worded message in that case (explaining that the problem might
be due to an RTLD_LOCAL library, or using hidden visibility for the type,
or linking against code produced by a compiler that violates the ABI).

Scenarios like this would easily be catered for in the ubsan code, by
> conservatively using strcmp rather than pointer comparison to determine
> type_info equality:
>
>  Index: lib/ubsan/ubsan_type_hash.cc
>> ==============================**==============================**=======
>> --- lib/ubsan/ubsan_type_hash.cc        (revision 188367)
>> +++ lib/ubsan/ubsan_type_hash.cc        (working copy)
>> @@ -13,6 +13,8 @@
>>  //
>>  //===-------------------------**------------------------------**
>> ---------------===//
>>
>> +#include <cstring>
>> +
>>  #include "ubsan_type_hash.h"
>>
>>  #include "sanitizer_common/sanitizer_**common.h"
>> @@ -116,7 +118,7 @@
>>  static bool isDerivedFromAtOffset(const abi::__class_type_info *Derived,
>>                                    const abi::__class_type_info *Base,
>>                                    sptr Offset) {
>> -  if (Derived->__type_name == Base->__type_name)
>> +  if (std::strcmp(Derived->__type_**name, Base->__type_name) == 0)
>
>      return Offset == 0;
>>
>>    if (const abi::__si_class_type_info *SI =
>>
>
> Stephan
>
>
> For [1], see libstdc++-v3/libsupc++/**typeinfo comment:
>
>  // Determine whether typeinfo names for the same type are merged (in which
>> // case comparison can just compare pointers) or not (in which case
>> strings
>> // must be compared), and whether comparison is to be implemented inline
>> or
>> // not.  We used to do inline pointer comparison by default if weak
>> symbols
>> // are available, but even with weak symbols sometimes names are not
>> merged
>> // when objects are loaded with RTLD_LOCAL, so now we always use strcmp by
>> // default.  For ABI compatibility, we do the strcmp inline if weak
>> symbols
>> // are available, and out-of-line if not.  Out-of-line pointer comparison
>> // is used where the object files are to be portable to multiple systems,
>> // some of which may not be able to use pointer comparison, but the
>> // particular system for which libstdc++ is being built can use pointer
>> // comparison; in particular for most ARM EABI systems, where the ABI
>> // specifies out-of-line comparison.  The compiler's target configuration
>> // can override the defaults by defining __GXX_TYPEINFO_EQUALITY_INLINE to
>> // 1 or 0 to indicate whether or not comparison is inline, and
>> // __GXX_MERGED_TYPEINFO_NAMES to 1 or 0 to indicate whether or not
>> pointer
>> // comparison can be used.
>>
> ______________________________**_________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/**mailman/listinfo/cfe-commits<http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130816/e6dc42c8/attachment.html>