[safestack] Various Solaris fixes #98001
Conversation
At the end of the recent flurry of Illumos (and implicitly Solaris) safestack patches, the tests were disabled in b0260c5 without explanation. After re-enabling them, there were many problems on Solaris: - Every single testcase failed to link: ``` Undefined first referenced symbol in file __safestack_unsafe_stack_ptr buffer-copy-vla.o __safestack_init (command line) ld: fatal: symbol referencing errors ``` The problem is that `-u __safestack_init` was passed to the linker after the corresponding version of `libclang_rt.safestack-*.a`. Since the Solaris linker (like Unix linkers for decades) respects the command line argument order (unlike e.g. GNU ld which uses GNU getopt), this cannot work. Fixed by moving the `-u` arg further to the front. Two affected testcases were fixed accordingly. * While 540fd42 enabled safestack on Solaris unconditionally, it ignored that Solaris also exists on SPARC and forgot to enable SPARC support. This patch fixes that. - Afterwards, the tests still fail to link with undefined references to `__sanitizer_internal_memset` etc. These are from `sanitizer_redefine_builtins.h`. Definitions live in `sanitizer_libc.cpp.o` and were added to the safestack runtime lib as is already the case e.g. for asan and ubsan. Why GNU ld allows the link to complete with those undefined is beyond me. - The `pthread*.c` tests `FAIL` with ``` safestack CHECK failed: /vol/llvm/src/llvm-project/local/compiler-rt/lib/safestack/safestack.cpp:227 size ``` The problem is that `pthread_attr_init` initializes the `stacksize` attribute to 0, signifying the default. Unless explicitly overridded, it stays that way. I think this is allowed by XPG7. Since safestack cannot deal with this, I set `size` to the defaults documented in `pthread_create(3C)`. Unfortunately, there's no macro for those values outside of private `libc` headers. - The Solaris `syscall` interface isn't stable. This is not just a theoretical concern, but the syscalls have changed incompatibly several times in the past. Therefore this patch switches the implementations of `TgKill` (where `SYS_lwp_kill` doesn't exist on Solaris 11.4 anyway), `Mmap`, `Munmap`, and `Mprotect` to the same `_REAL*` solution already used in `sanitizer_solaris.cpp`. Instead of duplicating what's already in `sanitizer_common`, it seems way better to me to just reuse those implementations, though. A subsequent patch does just that. With those changes, safestack compiles and all tests `PASS`. Tested on `amd64-pc-solaris2.11`, `sparcv9-sun-solaris2.11`, `x86_64-pc-linux-gnu`, and `sparc64-unknown-linux-gnu`.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-compiler-rt-sanitizer Author: Rainer Orth (rorth) ChangesAt the end of the recent flurry of Illumos (and implicitly Solaris) safestack patches, the tests were disabled in After re-enabling them, there were many problems on Solaris:
With those changes, safestack compiles and all tests Tested on Full diff: https://github.com/llvm/llvm-project/pull/98001.diff 8 Files Affected:
diff --git a/clang/lib/Driver/ToolChains/CommonArgs.cpp b/clang/lib/Driver/ToolChains/CommonArgs.cpp
index c56a0c2c46c47..fd70398027be9 100644
--- a/clang/lib/Driver/ToolChains/CommonArgs.cpp
+++ b/clang/lib/Driver/ToolChains/CommonArgs.cpp
@@ -1543,6 +1543,12 @@ bool tools::addSanitizerRuntimes(const ToolChain &TC, const ArgList &Args,
RequiredSymbols);
}
+ // -u options must be added before the runtime libs that resolve them.
+ for (auto S : RequiredSymbols) {
+ CmdArgs.push_back("-u");
+ CmdArgs.push_back(Args.MakeArgString(S));
+ }
+
// Inject libfuzzer dependencies.
if (SanArgs.needsFuzzer() && SanArgs.linkRuntimes() &&
!Args.hasArg(options::OPT_shared)) {
@@ -1575,10 +1581,6 @@ bool tools::addSanitizerRuntimes(const ToolChain &TC, const ArgList &Args,
addSanitizerRuntime(TC, Args, CmdArgs, RT, false, false);
AddExportDynamic |= !addSanitizerDynamicList(TC, Args, CmdArgs, RT);
}
- for (auto S : RequiredSymbols) {
- CmdArgs.push_back("-u");
- CmdArgs.push_back(Args.MakeArgString(S));
- }
// If there is a static runtime with no dynamic list, force all the symbols
// to be dynamic to be sure we export sanitizer interface functions.
if (AddExportDynamic)
diff --git a/clang/test/Driver/ohos.c b/clang/test/Driver/ohos.c
index b1ce61e7227b6..8de4e6de57f7f 100644
--- a/clang/test/Driver/ohos.c
+++ b/clang/test/Driver/ohos.c
@@ -95,8 +95,8 @@
// RUN: | FileCheck %s -check-prefix=CHECK-SAFESTACK
// CHECK-SAFESTACK: "-resource-dir" "[[RESOURCE_DIR:[^"]+]]"
// CHECK-SAFESTACK: "-fsanitize=safe-stack"
-// CHECK-SAFESTACK: "[[RESOURCE_DIR]]{{/|\\\\}}lib{{/|\\\\}}arm-liteos-ohos{{/|\\\\}}libclang_rt.safestack.a"
// CHECK-SAFESTACK: "__safestack_init"
+// CHECK-SAFESTACK: "[[RESOURCE_DIR]]{{/|\\\\}}lib{{/|\\\\}}arm-liteos-ohos{{/|\\\\}}libclang_rt.safestack.a"
// RUN: %clang %s -### --target=arm-liteos \
// RUN: -fsanitize=address 2>&1 \
diff --git a/clang/test/Driver/sanitizer-ld.c b/clang/test/Driver/sanitizer-ld.c
index 93702f456229f..ca2db69bc09d8 100644
--- a/clang/test/Driver/sanitizer-ld.c
+++ b/clang/test/Driver/sanitizer-ld.c
@@ -728,8 +728,8 @@
// CHECK-SAFESTACK-LINUX: "{{(.*[^-.0-9A-Z_a-z])?}}ld{{(.exe)?}}"
// CHECK-SAFESTACK-LINUX-NOT: "-lc"
// CHECK-SAFESTACK-LINUX-NOT: whole-archive
-// CHECK-SAFESTACK-LINUX: libclang_rt.safestack.a"
// CHECK-SAFESTACK-LINUX: "-u" "__safestack_init"
+// CHECK-SAFESTACK-LINUX: libclang_rt.safestack.a"
// CHECK-SAFESTACK-LINUX: "-lpthread"
// CHECK-SAFESTACK-LINUX: "-ldl"
// CHECK-SAFESTACK-LINUX: "-lresolv"
diff --git a/compiler-rt/cmake/Modules/AllSupportedArchDefs.cmake b/compiler-rt/cmake/Modules/AllSupportedArchDefs.cmake
index ac4a71202384d..5369e944a2e81 100644
--- a/compiler-rt/cmake/Modules/AllSupportedArchDefs.cmake
+++ b/compiler-rt/cmake/Modules/AllSupportedArchDefs.cmake
@@ -74,7 +74,7 @@ set(ALL_UBSAN_SUPPORTED_ARCH ${X86} ${X86_64} ${ARM32} ${ARM64} ${RISCV64}
${MIPS32} ${MIPS64} ${PPC64} ${S390X} ${SPARC} ${SPARCV9} ${HEXAGON}
${LOONGARCH64})
set(ALL_SAFESTACK_SUPPORTED_ARCH ${X86} ${X86_64} ${ARM64} ${MIPS32} ${MIPS64}
- ${HEXAGON} ${LOONGARCH64})
+ ${HEXAGON} ${LOONGARCH64} ${SPARC} ${SPARCV9})
set(ALL_CFI_SUPPORTED_ARCH ${X86} ${X86_64} ${ARM32} ${ARM64} ${MIPS64}
${HEXAGON} ${LOONGARCH64})
set(ALL_SCUDO_STANDALONE_SUPPORTED_ARCH ${X86} ${X86_64} ${ARM32} ${ARM64}
diff --git a/compiler-rt/lib/safestack/CMakeLists.txt b/compiler-rt/lib/safestack/CMakeLists.txt
index 316ab69ecfdbe..730043eb87cab 100644
--- a/compiler-rt/lib/safestack/CMakeLists.txt
+++ b/compiler-rt/lib/safestack/CMakeLists.txt
@@ -14,6 +14,8 @@ foreach(arch ${SAFESTACK_SUPPORTED_ARCH})
ARCHS ${arch}
SOURCES ${SAFESTACK_SOURCES}
$<TARGET_OBJECTS:RTInterception.${arch}>
+ OBJECT_LIBS RTSanitizerCommon
+ RTSanitizerCommonLibc
CFLAGS ${SAFESTACK_CFLAGS}
PARENT_TARGET safestack)
endforeach()
diff --git a/compiler-rt/lib/safestack/safestack.cpp b/compiler-rt/lib/safestack/safestack.cpp
index 0751f3988b9c1..f8ef224f45494 100644
--- a/compiler-rt/lib/safestack/safestack.cpp
+++ b/compiler-rt/lib/safestack/safestack.cpp
@@ -224,6 +224,17 @@ INTERCEPTOR(int, pthread_create, pthread_t *thread,
pthread_attr_destroy(&tmpattr);
}
+#if SANITIZER_SOLARIS
+ // Solaris pthread_attr_init initializes stacksize to 0 (the default), so
+ // hardcode the actual values as documented in pthread_create(3C).
+ if (size == 0)
+# if defined(_LP64)
+ size = 2 * 1024 * 1024;
+# else
+ size = 1024 * 1024;
+# endif
+#endif
+
SFS_CHECK(size);
size = RoundUpTo(size, kStackAlign);
diff --git a/compiler-rt/lib/safestack/safestack_platform.h b/compiler-rt/lib/safestack/safestack_platform.h
index d4b2e2ef7391c..77eeb9cda6e15 100644
--- a/compiler-rt/lib/safestack/safestack_platform.h
+++ b/compiler-rt/lib/safestack/safestack_platform.h
@@ -17,6 +17,7 @@
#include "sanitizer_common/sanitizer_platform.h"
#include <dlfcn.h>
+#include <errno.h>
#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>
@@ -68,6 +69,24 @@ static void *GetRealLibcAddress(const char *symbol) {
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() {
@@ -91,11 +110,10 @@ inline int TgKill(pid_t pid, ThreadId tid, int sig) {
(void)pid;
return _REAL(_lwp_kill, tid, sig);
#elif SANITIZER_SOLARIS
-# ifdef SYS_lwp_kill
- return syscall(SYS_lwp_kill, tid, sig);
-# else
- return -1;
-# endif
+ (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
@@ -110,8 +128,7 @@ inline void *Mmap(void *addr, size_t length, int prot, int flags, int fd,
#elif SANITIZER_FREEBSD && (defined(__aarch64__) || defined(__x86_64__))
return (void *)__syscall(SYS_mmap, addr, length, prot, flags, fd, offset);
#elif SANITIZER_SOLARIS
- return (void *)(uintptr_t)syscall(SYS_mmap, addr, length, prot, flags, fd,
- offset);
+ return _REAL64(mmap)(addr, length, prot, flags, fd, offset);
#else
return (void *)syscall(SYS_mmap, addr, length, prot, flags, fd, offset);
#endif
@@ -121,6 +138,8 @@ 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
@@ -130,6 +149,8 @@ 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
diff --git a/compiler-rt/test/safestack/lit.cfg.py b/compiler-rt/test/safestack/lit.cfg.py
index aadb8bf0d5c77..17dfae46a412b 100644
--- a/compiler-rt/test/safestack/lit.cfg.py
+++ b/compiler-rt/test/safestack/lit.cfg.py
@@ -33,5 +33,5 @@
)
)
-if config.host_os not in ["Linux", "FreeBSD", "NetBSD"]:
+if config.host_os not in ["Linux", "FreeBSD", "NetBSD", "SunOS"]:
config.unsupported = True
|
|
The mentioned patch to switch safestack to use the |
You can test this locally with the following command:git-clang-format --diff d6af73e9fbc84315100499a096f17ec5eeeeea23 f8de9c3c2dbde4021f0d32a55c8a385221c0f749 -- clang/lib/Driver/ToolChains/CommonArgs.cpp clang/test/Driver/ohos.c clang/test/Driver/sanitizer-ld.c compiler-rt/lib/safestack/safestack.cpp compiler-rt/lib/safestack/safestack_platform.hView the diff from clang-format here.diff --git a/compiler-rt/lib/safestack/safestack_platform.h b/compiler-rt/lib/safestack/safestack_platform.h
index 77eeb9cda6..27e29d6ead 100644
--- a/compiler-rt/lib/safestack/safestack_platform.h
+++ b/compiler-rt/lib/safestack/safestack_platform.h
@@ -13,9 +13,6 @@
#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>
@@ -26,6 +23,9 @@
#include <sys/types.h>
#include <unistd.h>
+#include "safestack_util.h"
+#include "sanitizer_common/sanitizer_platform.h"
+
#if !(SANITIZER_NETBSD || SANITIZER_FREEBSD || SANITIZER_LINUX || \
SANITIZER_SOLARIS)
# error "Support for your platform has not been implemented"
|
I had seen that when running |
|
you do not need to worry I won t do any illumos/solaris work anytime soon, feel free to do xray port and all the rest. |
|
They look quite independent. |
With the exception of the
Just let me know. |
-u Must have own patch The rest is up to you. Usually small fixes easier to bisect/revert, but it's less important if it's completely broken yet. |
I agree this part should be extracted since it affects other systems. Other parts are Solaris specific and since the support is currently broken, it is safe to combine changes. We also don't have Solaris build bots, so the change, if correctly restricted to Solaris, would not cause any problems. |
We sure have Solaris buildbots, both Solaris/sparcv9 and Solaris/amd64. Actually, two parts of the patch do affect non-Solaris targets:
|
|
Dropped; superceded by [clang][Driver] Fix safestack -u ordering and [safestack] Various Solaris fixes. |
At the end of the recent flurry of Illumos (and implicitly Solaris) safestack patches, the tests were disabled in
b0260c5 without explanation.
After re-enabling them, there were many problems on Solaris:
Every single testcase failed to link:
The problem is that
-u __safestack_initwas passed to the linker after the corresponding version oflibclang_rt.safestack-*.a. Since the Solaris linker (like Unix linkers for decades) respects the command line argument order (unlike e.g. GNU ld which uses GNU getopt), this cannot work. Fixed by moving the-uarg further to the front. Two affected testcases were adjusted accordingly.Afterwards, the tests still fail to link with undefined references to
__sanitizer_internal_memsetetc. These are fromsanitizer_redefine_builtins.h. Definitions live insanitizer_libc.cpp.oand were added to the safestack runtime lib as is already the case e.g. for asan and ubsan. Why GNU ld allows the link to complete with those symbols undefined is beyond me.The
pthread*.ctestsFAILwithsafestack CHECK failed: /vol/llvm/src/llvm-project/local/compiler-rt/lib/safestack/safestack.cpp:227 sizeThe problem is that
pthread_attr_initinitializes thestacksizeattribute to 0, signifying the default. Unless explicitly overridded, it stays that way. I think this is allowed by XPG7. Since safestack cannot deal with this, I setsizeto the defaults documented inpthread_create(3C). Unfortunately, there's no macro for those values outside of privatelibcheaders.The Solaris
syscallinterface isn't stable. This is not just a theoretical concern, but the syscalls have changed incompatibly several times in the past. Therefore this patch switches the implementations ofTgKill(whereSYS_lwp_killdoesn't exist on Solaris 11.4 anyway),Mmap,Munmap, andMprotectto the same_REAL*solution already used insanitizer_solaris.cpp. Instead of duplicating what's already insanitizer_common, it seems way better to me to just reuse those implementations, though. A subsequent patch does just that.With those changes, safestack compiles and all tests
PASS.Tested on
amd64-pc-solaris2.11,sparcv9-sun-solaris2.11,x86_64-pc-linux-gnu, andsparc64-unknown-linux-gnu.