[safestack] Switch to sanitizer_common functions #98513
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
safestack currently uses its own portability layer in `safestack_platform.h`. This is both wasteful and leads to errors easily avoided by using the well-tested functions in `sanitizer_common` instead. This is particularly notable when ([safestack] Support multilib testing E.g. all Linux/i386 tests `FAIL`ed with ``` safestack CHECK failed: /vol/llvm/src/llvm-project/local/compiler-rt/lib/safestack/safestack.cpp:95 MAP_FAILED != addr ``` because the safestack `mmap` implementation doesn't work there. Thus this patch switches safestack to use the `sanitizer_common` code instead. It's mostly obvious, with the exception of `TgKill`: - The `sanitizer_common` Linux `TgKill` implementation is wrong: while safestack used `syscall(SYS_tgkill)`, `sanitizer_common` has `internal_syscall(SYSCALL(tgkill))` instead. However, the latter (based on using the `syscall` insn directly on x86_64) returns `errno` instead of the expeccted `-1`. Besides, `TgKill` had to be moved to `sanitizer_linux_libcdep.cpp`, otherwise the `Sanitizer-x86_64-Test-Nolibc` test failed to link with an undefined reference to `syscall`. - On Solaris, a similar issue with `thr_kill` went unnoticed before because `sanitizer_common` `TgKill` wasn't used. ([sanitizer_common] Fix TgKill on Solaris)[llvm#98000] was merged into this patch. - On 32-bit Linux/sparc, `pthread-cleanup.c` would `FAIL` because a `tid_t` (`uint64_t`) `tid` arg was passed to `syscall(SYS_tgkill)` while `tid` is actually a `pid_t` (`int`). Tested on `amd64-pc-solaris2.11`, `sparcv9-sun-solaris2.11`, `x86_64-pc-solaris2.11`, and `sparc64-unknown-linux-gnu`.
|
@llvm/pr-subscribers-compiler-rt-sanitizer Author: Rainer Orth (rorth) Changessafestack currently uses its own portability layer in E.g. all Linux/i386 tests because the safestack Thus this patch switches safestack to use the
Tested on Full diff: https://github.com/llvm/llvm-project/pull/98513.diff 5 Files Affected:
diff --git a/compiler-rt/lib/safestack/safestack.cpp b/compiler-rt/lib/safestack/safestack.cpp
index f8ef224f45494..5ef5e77ee8fb4 100644
--- a/compiler-rt/lib/safestack/safestack.cpp
+++ b/compiler-rt/lib/safestack/safestack.cpp
@@ -13,14 +13,18 @@
//
//===----------------------------------------------------------------------===//
-#include "safestack_platform.h"
-#include "safestack_util.h"
-
#include <errno.h>
+#include <sys/mman.h>
#include <sys/resource.h>
+#include <unistd.h>
#include "interception/interception.h"
+#include "safestack_util.h"
+#include "sanitizer_common/sanitizer_common.h"
+#include "sanitizer_common/sanitizer_platform.h"
+#include "sanitizer_common/sanitizer_posix.h"
+using namespace __sanitizer;
using namespace safestack;
// TODO: To make accessing the unsafe stack pointer faster, we plan to
@@ -90,10 +94,11 @@ __thread size_t unsafe_stack_guard = 0;
inline void *unsafe_stack_alloc(size_t size, size_t guard) {
SFS_CHECK(size + guard >= size);
- void *addr = Mmap(nullptr, size + guard, PROT_READ | PROT_WRITE,
- MAP_PRIVATE | MAP_ANON, -1, 0);
+ void *addr =
+ (void *)internal_mmap(nullptr, size + guard, PROT_READ | PROT_WRITE,
+ MAP_PRIVATE | MAP_ANON, -1, 0);
SFS_CHECK(MAP_FAILED != addr);
- Mprotect(addr, guard, PROT_NONE);
+ internal_mprotect(addr, guard, PROT_NONE);
return (char *)addr + guard;
}
@@ -146,7 +151,7 @@ struct thread_stack_ll {
void *stack_base;
size_t size;
pid_t pid;
- ThreadId tid;
+ tid_t tid;
};
/// Linked list of unsafe stacks for threads that are exiting. We delay
@@ -169,7 +174,7 @@ void thread_cleanup_handler(void *_iter) {
pthread_mutex_unlock(&thread_stacks_mutex);
pid_t pid = getpid();
- ThreadId tid = GetTid();
+ tid_t tid = GetTid();
// Free stacks for dead threads
thread_stack_ll **stackp = &temp_stacks;
@@ -177,7 +182,7 @@ void thread_cleanup_handler(void *_iter) {
thread_stack_ll *stack = *stackp;
if (stack->pid != pid ||
(-1 == TgKill(stack->pid, stack->tid, 0) && errno == ESRCH)) {
- Munmap(stack->stack_base, stack->size);
+ internal_munmap(stack->stack_base, stack->size);
*stackp = stack->next;
free(stack);
} else
diff --git a/compiler-rt/lib/safestack/safestack_platform.h b/compiler-rt/lib/safestack/safestack_platform.h
deleted file mode 100644
index 77eeb9cda6e15..0000000000000
--- a/compiler-rt/lib/safestack/safestack_platform.h
+++ /dev/null
@@ -1,161 +0,0 @@
-//===-- safestack_platform.h ----------------------------------------------===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===----------------------------------------------------------------------===//
-//
-// This file implements platform specific parts of SafeStack runtime.
-//
-//===----------------------------------------------------------------------===//
-
-#ifndef SAFESTACK_PLATFORM_H
-#define SAFESTACK_PLATFORM_H
-
-#include "safestack_util.h"
-#include "sanitizer_common/sanitizer_platform.h"
-
-#include <dlfcn.h>
-#include <errno.h>
-#include <stdint.h>
-#include <stdio.h>
-#include <stdlib.h>
-#include <sys/mman.h>
-#include <sys/syscall.h>
-#include <sys/types.h>
-#include <unistd.h>
-
-#if !(SANITIZER_NETBSD || SANITIZER_FREEBSD || SANITIZER_LINUX || \
- SANITIZER_SOLARIS)
-# error "Support for your platform has not been implemented"
-#endif
-
-#if SANITIZER_NETBSD
-#include <lwp.h>
-
-extern "C" void *__mmap(void *, size_t, int, int, int, int, off_t);
-#endif
-
-#if SANITIZER_FREEBSD
-#include <sys/thr.h>
-#endif
-
-#if SANITIZER_SOLARIS
-# include <thread.h>
-#endif
-
-namespace safestack {
-
-#if SANITIZER_NETBSD
-static void *GetRealLibcAddress(const char *symbol) {
- void *real = dlsym(RTLD_NEXT, symbol);
- if (!real)
- real = dlsym(RTLD_DEFAULT, symbol);
- if (!real) {
- fprintf(stderr, "safestack GetRealLibcAddress failed for symbol=%s",
- symbol);
- abort();
- }
- return real;
-}
-
-#define _REAL(func, ...) real##_##func(__VA_ARGS__)
-#define DEFINE__REAL(ret_type, func, ...) \
- static ret_type (*real_##func)(__VA_ARGS__) = NULL; \
- if (!real_##func) { \
- real_##func = (ret_type(*)(__VA_ARGS__))GetRealLibcAddress(#func); \
- } \
- SFS_CHECK(real_##func);
-#endif
-
-#if SANITIZER_SOLARIS
-# define _REAL(func) _##func
-# define DEFINE__REAL(ret_type, func, ...) \
- extern "C" ret_type _REAL(func)(__VA_ARGS__)
-
-# if !defined(_LP64) && _FILE_OFFSET_BITS == 64
-# define _REAL64(func) _##func##64
-# else
-# define _REAL64(func) _REAL(func)
-# endif
-# define DEFINE__REAL64(ret_type, func, ...) \
- extern "C" ret_type _REAL64(func)(__VA_ARGS__)
-
-DEFINE__REAL64(void *, mmap, void *a, size_t b, int c, int d, int e, off_t f);
-DEFINE__REAL(int, munmap, void *a, size_t b);
-DEFINE__REAL(int, mprotect, void *a, size_t b, int c);
-#endif
-
-using ThreadId = uint64_t;
-
-inline ThreadId GetTid() {
-#if SANITIZER_NETBSD
- DEFINE__REAL(int, _lwp_self);
- return _REAL(_lwp_self);
-#elif SANITIZER_FREEBSD
- long Tid;
- thr_self(&Tid);
- return Tid;
-#elif SANITIZER_SOLARIS
- return thr_self();
-#else
- return syscall(SYS_gettid);
-#endif
-}
-
-inline int TgKill(pid_t pid, ThreadId tid, int sig) {
-#if SANITIZER_NETBSD
- DEFINE__REAL(int, _lwp_kill, int a, int b);
- (void)pid;
- return _REAL(_lwp_kill, tid, sig);
-#elif SANITIZER_SOLARIS
- (void)pid;
- errno = thr_kill(tid, sig);
- // TgKill is expected to return -1 on error, not an errno.
- return errno != 0 ? -1 : 0;
-#elif SANITIZER_FREEBSD
- return syscall(SYS_thr_kill2, pid, tid, sig);
-#else
- return syscall(SYS_tgkill, pid, tid, sig);
-#endif
-}
-
-inline void *Mmap(void *addr, size_t length, int prot, int flags, int fd,
- off_t offset) {
-#if SANITIZER_NETBSD
- return __mmap(addr, length, prot, flags, fd, 0, offset);
-#elif SANITIZER_FREEBSD && (defined(__aarch64__) || defined(__x86_64__))
- return (void *)__syscall(SYS_mmap, addr, length, prot, flags, fd, offset);
-#elif SANITIZER_SOLARIS
- return _REAL64(mmap)(addr, length, prot, flags, fd, offset);
-#else
- return (void *)syscall(SYS_mmap, addr, length, prot, flags, fd, offset);
-#endif
-}
-
-inline int Munmap(void *addr, size_t length) {
-#if SANITIZER_NETBSD
- DEFINE__REAL(int, munmap, void *a, size_t b);
- return _REAL(munmap, addr, length);
-#elif SANITIZER_SOLARIS
- return _REAL(munmap)(addr, length);
-#else
- return syscall(SYS_munmap, addr, length);
-#endif
-}
-
-inline int Mprotect(void *addr, size_t length, int prot) {
-#if SANITIZER_NETBSD
- DEFINE__REAL(int, mprotect, void *a, size_t b, int c);
- return _REAL(mprotect, addr, length, prot);
-#elif SANITIZER_SOLARIS
- return _REAL(mprotect)(addr, length, prot);
-#else
- return syscall(SYS_mprotect, addr, length, prot);
-#endif
-}
-
-} // namespace safestack
-
-#endif // SAFESTACK_PLATFORM_H
diff --git a/compiler-rt/lib/safestack/safestack_util.h b/compiler-rt/lib/safestack/safestack_util.h
index da3f11b54cabc..9f571b36e08c4 100644
--- a/compiler-rt/lib/safestack/safestack_util.h
+++ b/compiler-rt/lib/safestack/safestack_util.h
@@ -28,11 +28,6 @@ namespace safestack {
}; \
} while (false)
-inline size_t RoundUpTo(size_t size, size_t boundary) {
- SFS_CHECK((boundary & (boundary - 1)) == 0);
- return (size + boundary - 1) & ~(boundary - 1);
-}
-
class MutexLock {
public:
explicit MutexLock(pthread_mutex_t &mutex) : mutex_(&mutex) {
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp
index f7f5698a5f32d..172a78e5a4e78 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp
@@ -566,17 +566,6 @@ tid_t GetTid() {
return internal_syscall(SYSCALL(gettid));
# endif
}
-
-int TgKill(pid_t pid, tid_t tid, int sig) {
-# if SANITIZER_LINUX
- return internal_syscall(SYSCALL(tgkill), pid, tid, sig);
-# elif SANITIZER_FREEBSD
- return internal_syscall(SYSCALL(thr_kill2), pid, tid, sig);
-# elif SANITIZER_SOLARIS
- (void)pid;
- return thr_kill(tid, sig);
-# endif
-}
# endif
# if SANITIZER_GLIBC
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_linux_libcdep.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_linux_libcdep.cpp
index c3c717bbdbe4c..38226fbf6d164 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_linux_libcdep.cpp
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_linux_libcdep.cpp
@@ -33,11 +33,13 @@
# endif
# include <dlfcn.h> // for dlsym()
+# include <errno.h>
# include <link.h>
# include <pthread.h>
# include <signal.h>
# include <sys/mman.h>
# include <sys/resource.h>
+# include <sys/syscall.h>
# include <syslog.h>
# if !defined(ElfW)
@@ -113,6 +115,23 @@ int internal_sigaction(int signum, const void *act, void *oldact) {
# endif
}
+# if !SANITIZER_NETBSD
+int TgKill(pid_t pid, tid_t tid, int sig) {
+# if SANITIZER_LINUX
+ // Cannot use internal_syscall here: returns -errno instead of -1 on failure
+ // on x86_64.
+ return syscall(SYS_tgkill, pid, (pid_t)tid, sig);
+# elif SANITIZER_FREEBSD
+ return internal_syscall(SYSCALL(thr_kill2), pid, tid, sig);
+# elif SANITIZER_SOLARIS
+ (void)pid;
+ errno = thr_kill(tid, sig);
+ // TgKill is expected to return -1 on error, not an errno.
+ return errno != 0 ? -1 : 0;
+# endif
+}
+# endif
+
void GetThreadStackTopAndBottom(bool at_initialization, uptr *stack_top,
uptr *stack_bottom) {
CHECK(stack_top);
|
|
We can't do that see #98541 |
|
Now superceded by #99455. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
safestack currently uses its own portability layer in
safestack_platform.h. This is both wasteful and leads to errors easily avoided by using the well-tested functions insanitizer_commoninstead. This is particularly notable when [safestack] Support multilib testing is applied.E.g. all Linux/i386 tests
FAILed withbecause the safestack
mmapimplementation doesn't work there.Thus this patch switches safestack to use the
sanitizer_commoncode instead. It's mostly obvious, with the exception ofTgKill:sanitizer_commonLinuxTgKillimplementation is wrong: while safestack usedsyscall(SYS_tgkill),sanitizer_commonhasinternal_syscall(SYSCALL(tgkill))instead. However, the latter (based on using thesyscallinsn directly on x86_64) returnserrnoinstead of the expeccted-1. Besides,TgKillhad to be moved tosanitizer_linux_libcdep.cpp, otherwise theSanitizer-x86_64-Test-Nolibctest failed to link with an undefined reference tosyscall.thr_killwent unnoticed before becausesanitizer_commonTgKillwasn't used. [sanitizer_common] Fix TgKill on Solaris was merged into this patch.pthread-cleanup.cwouldFAILbecause atid_t(uint64_t)tidarg was passed tosyscall(SYS_tgkill)whiletidis actually apid_t(int).Tested on
amd64-pc-solaris2.11,sparcv9-sun-solaris2.11,x86_64-pc-solaris2.11, andsparc64-unknown-linux-gnu.