Skip to content

[compiler-rt][TSan] Add support for Android #147580

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Oct 6, 2025
Merged

[compiler-rt][TSan] Add support for Android #147580

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
98b8be2
[TSan] Add support for Android
airpfei Jul 17, 2025
e0e8666
Rearrange LONG_JMP_SP_ENV_SLOT defines
airpfei Sep 29, 2025
e316d29
Fix issues raised in comments
airpfei Sep 29, 2025
b35d234
Merge branch 'main' into main
airpfei Sep 29, 2025
98929e0
fix code format
airpfei Sep 29, 2025
eceee1f
Merge branch 'main' into main
airpfei Sep 29, 2025
0abdf7d
update comment
airpfei Sep 30, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2412,7 +2412,11 @@ TSAN_INTERCEPTOR(int, vfork, int fake) {
}
#endif

#if SANITIZER_LINUX
#if SANITIZER_LINUX && !SANITIZER_ANDROID
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why?

seems like we need to intercept clone() for tsan especially?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

glibc's pthread_create calls __clone_internal, however bionic's pthread_create calls clone. When enabling incept clone on Android, this check failed. https://github.com/llvm/llvm-project/blob/main/compiler-rt/lib/tsan/rtl/tsan_rtl.cpp#L1062

I'm not sure if this is the best way

https://codebrowser.dev/glibc/glibc/nptl/pthread_create.c.html#297

https://android.googlesource.com/platform/bionic/+/refs/heads/android16-release/libc/bionic/pthread_create.cpp#466

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's an interesting argument for adding an equivalent of glibc's __clone_internal(), though we'd have to work with the ndk-translation folks, who deliberately intercept clone().

this is probably the "least worst" option until/unless someone comes complaining that they can't use tsan with clone()...

// Bionic's pthread_create internally calls clone. When the CLONE_THREAD flag is
// set, clone does not create a new process but a new thread. This is a
// workaround for Android. Disabling the interception of clone solves the
// problem in most scenarios.
TSAN_INTERCEPTOR(int, clone, int (*fn)(void *), void *stack, int flags,
void *arg, int *parent_tid, void *tls, pid_t *child_tid) {
SCOPED_INTERCEPTOR_RAW(clone, fn, stack, flags, arg, parent_tid, tls,
Expand Down Expand Up @@ -3135,7 +3139,7 @@ void InitializeInterceptors() {

TSAN_INTERCEPT(fork);
TSAN_INTERCEPT(vfork);
#if SANITIZER_LINUX
#if SANITIZER_LINUX && !SANITIZER_ANDROID
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fmayer --- what's the compatibility expectation for compiler-rt? i'm wondering whether i should make pthread_create() and fork() call clone() via a hidden alias so they don't get intercepted ... but but then there's nothing to do here other than wait a decade?

TSAN_INTERCEPT(clone);
#endif
#if !SANITIZER_ANDROID
Expand Down
52 changes: 42 additions & 10 deletions compiler-rt/lib/tsan/rtl/tsan_platform_linux.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -486,8 +486,20 @@ int ExtractRecvmsgFDs(void *msgp, int *fds, int nfd) {

// Reverse operation of libc stack pointer mangling
static uptr UnmangleLongJmpSp(uptr mangled_sp) {
#if defined(__x86_64__)
# if SANITIZER_LINUX
# if SANITIZER_ANDROID
if (longjmp_xor_key == 0) {
// bionic libc initialization process: __libc_init_globals ->
// __libc_init_vdso (calls strcmp) -> __libc_init_setjmp_cookie. strcmp is
// intercepted by TSan, so during TSan initialization the setjmp_cookie
// remains uninitialized. On Android, longjmp_xor_key must be set on first
// use.
InitializeLongjmpXorKey();
CHECK_NE(longjmp_xor_key, 0);
}
# endif

# if defined(__x86_64__)
# if SANITIZER_LINUX
// Reverse of:
// xor %fs:0x30, %rsi
// rol $0x11, %rsi
Expand Down Expand Up @@ -542,21 +554,31 @@ static uptr UnmangleLongJmpSp(uptr mangled_sp) {
# else
# define LONG_JMP_SP_ENV_SLOT 2
# endif
#elif SANITIZER_LINUX
# ifdef __aarch64__
# define LONG_JMP_SP_ENV_SLOT 13
# elif defined(__loongarch__)
# define LONG_JMP_SP_ENV_SLOT 1
# elif defined(__mips64)
# define LONG_JMP_SP_ENV_SLOT 1
# elif SANITIZER_ANDROID
# ifdef __aarch64__
# define LONG_JMP_SP_ENV_SLOT 3
# elif SANITIZER_RISCV64
# define LONG_JMP_SP_ENV_SLOT 3
# elif defined(__x86_64__)
# define LONG_JMP_SP_ENV_SLOT 6
# else
# error unsupported
# endif
# elif SANITIZER_LINUX
# ifdef __aarch64__
# define LONG_JMP_SP_ENV_SLOT 13
# elif defined(__loongarch__)
# define LONG_JMP_SP_ENV_SLOT 1
# elif defined(__mips64)
# define LONG_JMP_SP_ENV_SLOT 1
# elif SANITIZER_RISCV64
# define LONG_JMP_SP_ENV_SLOT 13
# elif defined(__s390x__)
# define LONG_JMP_SP_ENV_SLOT 9
# else
# define LONG_JMP_SP_ENV_SLOT 6
# endif
#endif
# endif

uptr ExtractLongJmpSp(uptr *env) {
uptr mangled_sp = env[LONG_JMP_SP_ENV_SLOT];
Expand Down Expand Up @@ -653,6 +675,16 @@ ThreadState *cur_thread() {
}
CHECK_EQ(0, internal_sigprocmask(SIG_SETMASK, &oldset, nullptr));
}

// https://android.googlesource.com/platform/external/skia/+/refs/tags/android-15.0.0_r36/src/ports/SkMemory_malloc.cpp#105
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this link doesn't seem relevant?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is the full call stacks.

From this document https://llvm.org/docs/ScudoHardenedAllocator.html. Scudo is currently the default allocator in Fuchsia, and in Android since Android 11.

mallopt(M_THREAD_DISABLE_MEM_INIT, 1); -> SCUDO_ALLOCATOR.setOption -> getTlsPtr() = getPlatformAllocatorTlsSlot() = __get_tls()[TLS_SLOT_SANITIZER]

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't think any of those links are useful either. i think the comment below where you talk about mallopt() is sufficient.

@cferris1000 as the scudo maintainer in case he disagrees and thinks it's useful to have all these links, but i think the comment from "Skia changes..." to "..ThreadState pointer." is fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed with enh, the links are not that useful.

// https://android.googlesource.com/platform/external/scudo/+/refs/tags/android-15.0.0_r36/standalone/tsd_shared.h#198
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably best to just refer to the copy in llvm directly?

// Workaround to get the correct ThreadState pointer. Scudo allocator uses the
// last bit of TLS_SLOT_SANITIZER as a flag of disable memory initialization.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/of/to/

// getPlatformAllocatorTlsSlot should not use TLS_SLOT_SANITIZER slot.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

delete this line, because i don't think it adds anything? (i think you mean "should not use TLS_SLOT_SANITIZER directly", but you already explained that better in the previous line where you said that the bottom bit may be invalid.)

uptr addr = reinterpret_cast<uptr>(thr);
if (addr % 2 != 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need for the if, right? this can just be

  return reinterpret_cast<ThreadState*>(reinterpret_cast<uptr>(thr) & ~1ULL);

return reinterpret_cast<ThreadState*>(addr & ~1ULL);
}
return thr;
}

Expand Down
6 changes: 5 additions & 1 deletion compiler-rt/lib/tsan/rtl/tsan_rtl_thread.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,10 +206,14 @@ void ThreadStart(ThreadState *thr, Tid tid, ThreadID os_id,
}
#endif

#if !SANITIZER_GO
#if !SANITIZER_GO && !SANITIZER_ANDROID
// Don't imitate stack/TLS writes for the main thread,
// because its initialization is synchronized with all
// subsequent threads anyway.
// Because thr is created by MmapOrDie, the thr object
// is not in tls, the pointer of thr object is in
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/of/to the/

// TLS_SLOT_SANITIZER slot. So skip this check on
// Android platform.
if (tid != kMainTid) {
if (stk_addr && stk_size) {
const uptr pc = StackTrace::GetNextInstructionPc(
Expand Down