Page MenuHomePhabricator

Adapt -fsanitize=function to SANITIZER_NON_UNIQUE_TYPEINFO
ClosedPublic

Authored by sberg on Apr 16 2019, 1:06 AM.

Details

Summary

This follows up after b7692bc3e9ad2691fc07261904b88fb15f30696b "[UBSan] Fix
isDerivedFromAtOffset on iOS ARM64" fixed the RTTI comparison in
isDerivedFromAtOffset on just one platform and then
a25a2c7c9a7e1e328a5bd8274d2d86b1fadc4692 "Always compare C++ typeinfo (based on
libstdc++ implementation)" extended that fix to more platforms.

But there is another RTTI comparison for -fsanitize=function generated in
clang's CodeGenFunction::EmitCall as just a pointer comparison. For
SANITIZER_NON_UNIQUE_TYPEINFO platforms this needs to be extended to also do
string comparison. For that, __ubsan_handle_function_type_mismatch[_abort]
takes the two std::type_info pointers as additional parameters now, checks them
internally for potential equivalence, and returns without reporting failure if
they turn out to be equivalent after all. (NORETURN needed to be dropped from
the _abort variant for that.) Also these functions depend on ABI-specific RTTI
now, so needed to be moved from plain UBSAN_SOURCES (ubsan_handlers.h/cc) to
UBSAN_CXXABI_SOURCES (ubsan_handlers_cxx.h/cc), but as -fsanitize=function is
only supported in C++ mode that's not a problem.

Diff Detail

Event Timeline

sberg created this revision.Apr 16 2019, 1:06 AM
Herald added projects: Restricted Project, Restricted Project, Restricted Project. · View Herald TranscriptApr 16 2019, 1:06 AM
Herald added subscribers: llvm-commits, Restricted Project, cfe-commits and 3 others. · View Herald Transcript
sberg marked an inline comment as done.Apr 16 2019, 1:08 AM
sberg added inline comments.
compiler-rt/lib/ubsan/ubsan_type_hash_itanium.cc
264

My understanding of that GCC * prefix hack is that would should check symmetrically for both type_infos (which is different how isDerivedFromAtOffset above did it in the past)?

sberg added a comment.Apr 23 2019, 2:01 AM

friendly ping

This revision was not accepted when it landed; it landed in state Needs Review.May 1 2019, 11:40 PM
This revision was automatically updated to reflect the committed changes.

Did this get reviewed?

sberg added a comment.May 2 2019, 1:25 AM

Did this get reviewed?

I didn't get any responses at all, so decided to push it anyway.

Did this get reviewed?

I didn't get any responses at all, so decided to push it anyway.

That is not really how reviews work in LLVM.
Either it's a NFC / eligible for post-commit review, and can just be committed, or not.
E.g., is this missing test coverage?

sberg added a comment.May 2 2019, 11:56 PM

Added missing tests at https://reviews.llvm.org/D61479 "Add tests for 'Adapt -fsanitize=function to SANITIZER_NON_UNIQUE_TYPEINFO'".