[PATCH] D88501: [asan][test] Several Posix/unpoison-alternate-stack.cpp fixes

Tue Sep 29 10:10:32 PDT 2020

ro created this revision.
ro added a reviewer: vitalybuka.
ro added a project: Sanitizers.
Herald added subscribers: Sanitizers, fedor.sergeev.
ro requested review of this revision.

`Posix/unpoison-alternate-stack.cpp` currently `FAIL`s on Solaris/i386 and the investigation revealed several generic problems:

- `clang` warns compiling the testcase:

  /vol/llvm/src/llvm-project/local/compiler-rt/test/asan/TestCases/Posix/unpoison-alternate-stack.cpp:83:7: warning: nested designators are a C99 extension [-Wc99-designator]
        .sa_sigaction = signalHandler,
        ^~~~~~~~~~~~~
  /vol/llvm/src/llvm-project/local/compiler-rt/test/asan/TestCases/Posix/unpoison-alternate-stack.cpp:84:7: warning: ISO C++ requires field designators to be specified in declaration order; field '_funcptr' will be initialized after field 'sa_flags' [-Wreorder-init-list]
        .sa_flags = SA_SIGINFO | SA_NODEFER | SA_ONSTACK,
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  /vol/llvm/src/llvm-project/local/compiler-rt/test/asan/TestCases/Posix/unpoison-alternate-stack.cpp:83:8: note: previous initialization for field '_funcptr' is here
        .sa_sigaction = signalHandler,
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
  /usr/include/sys/signal.h:71:30: note: expanded from macro 'sa_sigaction'
  #define sa_sigaction    _funcptr._sigaction
                                  ^
  /vol/llvm/src/llvm-project/local/compiler-rt/test/asan/TestCases/Posix/unpoison-alternate-stack.cpp:149:7: warning: ISO C++ requires field designators to be specified in declaration order; field 'ss_flags' will be initialized after field 'ss_size' [-Wreorder-init-list]
        .ss_size = AltStackSize,
        ^~~~~~~~~~~~~~~~~~~~~~~
  /vol/llvm/src/llvm-project/local/compiler-rt/test/asan/TestCases/Posix/unpoison-alternate-stack.cpp:148:19: note: previous initialization for field 'ss_flags' is here
        .ss_flags = 0,
                    ^

  This can all easily be avoided by initializing each field separately.

- The test `SEGV`s:

  Thread 4 received signal SIGSEGV, Segmentation fault.
  [Switching to Thread 2 (LWP 2)]
  0x08157f0f in __asan_memcpy (to=0xfe5d2e30, from=0x808cda0 <__const._ZN12_GLOBAL__N_123setSignalAlternateStackEPv.Action>, size=32) at /vol/llvm/src/llvm-project/dist/compiler-rt/lib/asan/asan_interceptors_memintrinsics.cpp:21
  21	void *__asan_memcpy(void *to, const void *from, uptr size) {
  (gdb) where
  #0  0x08157f0f in __asan_memcpy (to=0xfe5d2e30, from=0x808cda0 <__const._ZN12_GLOBAL__N_123setSignalAlternateStackEPv.Action>, size=32) at /vol/llvm/src/llvm-project/dist/compiler-rt/lib/asan/asan_interceptors_memintrinsics.cpp:21
  #1  0x081b080e in (anonymous namespace)::setSignalAlternateStack(void*) () at /vol/llvm/src/llvm-project/dist/compiler-rt/test/asan/TestCases/Posix/unpoison-alternate-stack.cpp:86
  #2  0x081b02bc in (anonymous namespace)::threadFun(void*) () at /vol/llvm/src/llvm-project/dist/compiler-rt/test/asan/TestCases/Posix/unpoison-alternate-stack.cpp:107
  #3  0x0816dd2a in __asan::AsanThread::ThreadStart (this=0xfe3f3000, os_id=2, signal_thread_is_registered=0xfeffd724) at /vol/llvm/src/llvm-project/dist/compiler-rt/lib/asan/asan_thread.cpp:262
  #4  0x081400d0 in asan_thread_start (arg=0xfeffd720) at /vol/llvm/src/llvm-project/dist/compiler-rt/lib/asan/asan_interceptors.cpp:205
  #5  0xfe281d3b in _thrp_setup () from /lib/libc.so.1
  #6  0xfe282090 in ?? () from /lib/libc.so.1

  in the sequence determing the GOT address at the `calll` insn:

  	subl	$5424, %esp                     # imm = 0x1530
  	calll	.L1$pb
  .L1$pb:

  The problem is that the default Solaris/i386 stack size is only 4 kB, while `__asan_memcpy` tries to allocate either 5436 (32-bit) or 10688 bytes (64-bit) on the stack.  This patch avoids this by requiring at least 16 kB stack size.

- When I later investigated `PlatformUnpoisonStacks` calling `GetThreadStackAndTls`, I was reminded that `GetTls` is a dummy on Solaris.  I do have a (not yet fully portable) implemtation, however that's moot for this testcase.
- Even without `-fsanitize=address` or in a minimal testcase derived from this one, I get an assertion failure:

  Assertion failed: !isOnSignalStack(), file /vol/llvm/src/llvm-project/dist/compiler-rt/test/asan/TestCases/Posix/unpoison-alternate-stack.cpp, line 117

  The fundamental problem with this testcase is that `longjmp` from a signal handler is highly unportable; XPG7 strongly warns against it and it is thus unspecified which stack is used when `longjmp`ing from a signal handler running on an alternative stack.

So I'm `XFAIL`ing this testcases on Solaris.

Tested on `amd64-pc-solaris2.11` and `x86_64-pc-linux-gnu`.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D88501

Files:
  compiler-rt/test/asan/TestCases/Posix/unpoison-alternate-stack.cpp


Index: compiler-rt/test/asan/TestCases/Posix/unpoison-alternate-stack.cpp
===================================================================

--- compiler-rt/test/asan/TestCases/Posix/unpoison-alternate-stack.cpp
+++ compiler-rt/test/asan/TestCases/Posix/unpoison-alternate-stack.cpp
@@ -7,7 +7,10 @@
 // RUN: %run %t
 
 // XFAIL: ios && !iossim
+// longjmp from signal handler is unportable.
+// XFAIL: solaris
 
+#include <algorithm>
 #include <cassert>
 #include <cerrno>
 #include <csetjmp>
@@ -83,10 +86,9 @@
 void setSignalAlternateStack(void *AltStack) {
   sigaltstack((stack_t const *)AltStack, nullptr);
 
-  struct sigaction Action = {
-      .sa_sigaction = signalHandler,
-      .sa_flags = SA_SIGINFO | SA_NODEFER | SA_ONSTACK,
-  };
+  struct sigaction Action;
+  Action.sa_sigaction = signalHandler;
+  Action.sa_flags = SA_SIGINFO | SA_NODEFER | SA_ONSTACK;
   sigemptyset(&Action.sa_mask);
 
   sigaction(SIGUSR1, &Action, nullptr);
@@ -137,9 +139,11 @@
 // reports when the stack is reused.
 int main() {
   size_t const PageSize = sysconf(_SC_PAGESIZE);
+  // The Solaris defaults of 4k (32-bit) and 8k (64-bit) are too small.
+  size_t const MinStackSize = std::max(PTHREAD_STACK_MIN, 16 * 1024);
   // To align the alternate stack, we round this up to page_size.
   size_t const DefaultStackSize =
-      (PTHREAD_STACK_MIN - 1 + PageSize) & ~(PageSize - 1);
+      (MinStackSize - 1 + PageSize) & ~(PageSize - 1);
   // The alternate stack needs a certain size, or the signal handler segfaults.
   size_t const AltStackSize = 10 * PageSize;
   size_t const MappingSize = DefaultStackSize + AltStackSize;
@@ -149,11 +153,10 @@
                              MAP_PRIVATE | MAP_ANONYMOUS,
                              -1, 0);
 
-  stack_t const AltStack = {
-      .ss_sp = (char *)Mapping + DefaultStackSize,
-      .ss_flags = 0,
-      .ss_size = AltStackSize,
-  };
+  stack_t AltStack;
+  AltStack.ss_sp = (char *)Mapping + DefaultStackSize;
+  AltStack.ss_flags = 0;
+  AltStack.ss_size = AltStackSize;
 
   pthread_t Thread;
   pthread_attr_t ThreadAttr;


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D88501.294946.patch
Type: text/x-patch
Size: 2082 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200929/0a2a3b65/attachment.bin>
