Revert "[safestack] Various Solaris fixes"

[vitalybuka]

Reverts #98469

We can't add this dependency

   OBJECT_LIBS RTSanitizerCommon
                RTSanitizerCommonLibc

safestack is security hardening, and RTSanitizerCommon is too fat for that.

[github-actions]

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff d5285fef00f6c5a725a515118192dd117fc3c665 da6d977b46989435207f015dd3d6d80fef92430b --extensions h,cpp -- compiler-rt/lib/safestack/safestack.cpp compiler-rt/lib/safestack/safestack_platform.h

View 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 d4b2e2ef73..5bc3542410 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 <stdint.h>
 #include <stdio.h>
@@ -25,6 +22,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"

[vitalybuka]

@rorth Sorry, that I didn't notice that on the review.

Some parts probably do not need the revert, but I guess it would be easier for you to figure out what to related, with full revert.

[rorth]

I think we should add a prominent comment to safestack_platform.h why this is needed despite sanitizer_common.

Good thing you didn't revert only partially: with the RTSanitizerCommon part gone, every single safestack test would have failed to link on Solaris, breaking both buildbots.

I guess the way forward is to add stub versions of __sanitizer_internal_memset etc. that just abort: on Linux/x86_64, the test executables still contains undefined references to those, with no definition in sight, so it doesn't seem the are needed. With PR #98469 modified like this, safestack should be restored on Solaris.

As for PR #98513, I'll have to figure out how to fix safestack_platform.h to include what sanitizer_linux.cpp does differently for mmap and TgKill, otherwise the multilib testing patch cannot go in.

[rorth]

Dealing with __sanitizer_internal_memset turns out to be more involved than I hoped: when I added stub versions of __sanitizer_internal_mem* to safestack.cpp, testing went fine on all of Solaris/sparcv9, Solaris/amd64, and Linux/x86_64 (32 and 64-bit each). However, Linux/sparc64 was different: while 64-bit was ok as well, 32-bit aborted in a call to __sanitizer_internal_memset in the pthread_create interceptor. Here's what I found:

• safestack.cpp is compiled with -ftrivial-auto-var-init=pattern (which isn't documented in the clang manual AFAICT).
• This causes tmpattr in the pthread_create interceptor to be initialized to -1. On most targets, this is done inline: Solaris pthread_attr_init is sizeof(void *) so this is trivial, while on Linux it's way larger (36 bytes for 32-bit, 56-bytes for 64-bit). Only on 32-bit Linux/sparc, this is done with a call to memset which ultimately results in the failing call to __sanitizer_internal_memset.

It seems the safestack developers have just been lucky (or lazy) here.

I wonder how to deal with this: just add a copy of __sanitizer_internal_mem* to safestack.cpp to cope?

[vitalybuka]

How that's possi

Dealing with __sanitizer_internal_memset turns out to be more involved than I hoped: when I added stub versions of __sanitizer_internal_mem* to safestack.cpp, testing went fine on all of Solaris/sparcv9, Solaris/amd64, and Linux/x86_64 (32 and 64-bit each). However, Linux/sparc64 was different: while 64-bit was ok as well, 32-bit aborted in a call to __sanitizer_internal_memset in the pthread_create interceptor. Here's what I found:

• safestack.cpp is compiled with -ftrivial-auto-var-init=pattern (which isn't documented in the clang manual AFAICT).
• This causes tmpattr in the pthread_create interceptor to be initialized to -1. On most targets, this is done inline: Solaris pthread_attr_init is sizeof(void *) so this is trivial, while on Linux it's way larger (36 bytes for 32-bit, 56-bytes for 64-bit). Only on 32-bit Linux/sparc, this is done with a call to memset which ultimately results in the failing call to __sanitizer_internal_memset.

I believe just defining #define SANITIZER_COMMON_NO_REDEFINE_BUILTINS before including #include "sanitizer_common/sanitizer_platform.h" in compiler-rt/lib/safestack/safestack_platform.h is enough.

-ftrivial-auto-var-init=pattern is optional for compiler-rt, most helps to reduce false negative with lsan.

__sanitizer_internal_mem is from sanitizer_redefine_builtins.h which suppose to avoid libc mem* calls from sanitizers with mem* interceptors. Luckly for us safe stack does not have them, so SANITIZER_COMMON_NO_REDEFINE_BUILTINS is appropriate.

It seems the safestack developers have just been lucky (or lazy) here.

Both: -ftrivial-auto-var-init=pattern SANITIZER_COMMON_NO_REDEFINE_BUILTINS were added after safestack implementation. So this "sanitizer_common/sanitizer_platform.h" from safestack is unfortunate, we missed that adding pattern and REDEFINE for others sanitizers. Note iterception also use that header.

[rorth]

I believe just defining #define SANITIZER_COMMON_NO_REDEFINE_BUILTINS before including #include "sanitizer_common/sanitizer_platform.h" in compiler-rt/lib/safestack/safestack_platform.h is enough.

Unfortunately not. On Solaris at least, all tests still FAIL to link with

Undefined                       first referenced 
 symbol                             in file
__sanitizer_internal_memset         /var/llvm/local-amd64-release-stage2-A-flang-clang18/tools/clang/stage2-bins/lib/clang/19/lib/sunos/libclang_rt.safestack-i386.a(interception_linux.cpp.o)
__sanitizer_internal_memcpy         /var/llvm/local-amd64-release-stage2-A-flang-clang18/tools/clang/stage2-bins/lib/clang/19/lib/sunos/libclang_rt.safestack-i386.a(interception_linux.cpp.o)
__sanitizer_internal_memmove        /var/llvm/local-amd64-release-stage2-A-flang-clang18/tools/clang/stage2-bins/lib/clang/19/lib/sunos/libclang_rt.safestack-i386.a(interception_linux.cpp.o)

which indeed is ultimately due to sanitizer_redefine_builtins.h. Since the existing interception objects are just included in libclang_rt.safestack-${arch}.a as is, they aren't affected. I'm unclear if also adding #define SANITIZER_COMMON_NO_REDEFINE_BUILTINS at the top of interception_linux.cpp would be appropriate.

-ftrivial-auto-var-init=pattern is optional for compiler-rt, most helps to reduce false negative with lsan.

Ah, thanks. It still would be nice to document beyond clang --help.

__sanitizer_internal_mem is from sanitizer_redefine_builtins.h which suppose to avoid libc mem* calls from sanitizers with mem* interceptors. Luckly for us safe stack does not have them, so SANITIZER_COMMON_NO_REDEFINE_BUILTINS is appropriate.

It seems the safestack developers have just been lucky (or lazy) here.

Both: -ftrivial-auto-var-init=pattern SANITIZER_COMMON_NO_REDEFINE_BUILTINS were added after safestack implementation. So this "sanitizer_common/sanitizer_platform.h" from safestack is unfortunate, we missed that adding pattern and REDEFINE for others sanitizers. Note iterception also use that header.

Exactly, and it shows on Solaris ;-)