This is an archive of the discontinued LLVM Phabricator instance.

[PATCH] tsan: Remove __pointer_chk_guard@GLIBC_PRIVATE requirement for AArch64
ClosedPublic

Authored by zatrazz on Aug 9 2016, 7:22 AM.

Details

Summary

Current AArch64 {sig}{set,long}jmp interposing requires accessing glibc
private __pointer_chk_guard to get process xor mask to demangled the
internal {sig}jmp_buf function pointers.

It causes some packing issues, as described in gcc PR#71042 [1], and is
is not a godd practice to rely on a private glibc namespace (since ABI is
not meant to be stable).

This patch fixes it by changing how libtsan obtains the guarded pointer
value: at initialization a specific routine issues a setjmp call and
using the mangled function pointer and the original value derive the
random guarded pointer.

Checked on aarch64 39-bit VMA.

[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=71042

Diff Detail

Event Timeline

zatrazz updated this revision to Diff 67342.Aug 9 2016, 7:22 AM
zatrazz retitled this revision from to [PATCH] tsan: Remove __pointer_chk_guard@GLIBC_PRIVATE requirement for AArch64.
zatrazz updated this object.
zatrazz added reviewers: samsonov, kcc, aizatsky, dvyukov.
zatrazz added a project: Restricted Project.
zatrazz added a subscriber: llvm-commits.
dvyukov accepted this revision.Aug 9 2016, 9:04 AM
dvyukov edited edge metadata.

Looks good to me. But I did not read the assembly. Please wait few days for Renato.

lib/tsan/rtl/tsan_platform_linux.cc
273

I think this comment belongs to tsan_rtl_aarch64.S.
This file is platform independent, and the things this comment talks about are all in tsan_rtl_aarch64.S.

This revision is now accepted and ready to land.Aug 9 2016, 9:04 AM

Adding a few people, as I'm on holiday.

I worry that we'll end up with our own private implementation, and create problems for TSAN in the future. Or maybe I didn't understand the problem... :-)

Adding a few people, as I'm on holiday.

I worry that we'll end up with our own private implementation, and create problems for TSAN in the future. Or maybe I didn't understand the problem... :-)

TSAN on Linux already requires to handle at lot of glibc internal implementation to work corretly and in this case its is to avoid use a internal linker symbol. What kind of issues are you worried about?

zatrazz added inline comments.Aug 9 2016, 11:26 AM
lib/tsan/rtl/tsan_platform_linux.cc
273

Alright, I think I can just add a comment explaining this calls a arch-specific code to init the glibc guard pointer.

Reading again, it seems you're just reimplementing a similar logic, with a known global, which is safer than relying on glibc's own copy. So, even for gcc using TSAN, there will be no deviation.

You're still relying on _ZN14interception12realsetjmpE, but that's a public interface so it should be OK, right?

Apart from the name nit, I have no real concern.

Cheers,
Renato

lib/tsan/rtl/tsan_rtl_aarch64.S
76

Maybe call it __tsan_pointer_chk_guard?

Reading again, it seems you're just reimplementing a similar logic, with a known global, which is safer than relying on glibc's own copy. So, even for gcc using TSAN, there will be no deviation.

Yes, in fact the requirement of this patch canme from GCC PR#71042.

You're still relying on _ZN14interception12realsetjmpE, but that's a public interface so it should be OK, right?

It is in fact 'interception::realsetjmp' which is defined by interceptors macros and initialized at 'InitializeInterceptors'. It requires to use this indirection because 'setjmp/_setjmp' is overriden in this same assembly, so linker won't call GLIBC one.

Apart from the name nit, I have no real concern.

Cheers,
Renato

lib/tsan/rtl/tsan_rtl_aarch64.S
76

Since it is a local symbol I didn't bother with the name, but I think your suggestion seems better. I will change it.

zatrazz closed this revision.Aug 10 2016, 2:47 PM