[asan] Unpoison signal alternate stack.

[robot]

Summary

Before unwinding the stack, __asan_handle_no_return is supposed to
unpoison the entire stack - that is, remove the entries in the shadow
memory corresponding to stack (e.g. redzone markers around variables).
This does not work correctly if __asan_handle_no_return is called from
the alternate stack used in signal handlers, because the stack top is
read from a cache, which yields the default stack top instead of the
signal alternate stack top.

It is also possible to jump between the default stack and the signal
alternate stack. Therefore, __asan_handle_no_return needs to unpoison
both.

[robot]

Rebased, slightly better error reporting in test.

[robot]

Fix clang-tidy issues. This cause inconsistency in code style in asan (snake case vs camel case), because I did not want to touch all the asan code in this patch.

[eugenis]

compiler-rt/lib/asan/asan_rtl.cpp

620 ↗

(On Diff #255325)

I do not think this is true - false positive reports are only possible if we fail the kMaxExpectedCleanupSize check below.
Remove this statement?

compiler-rt/lib/sanitizer_common/sanitizer_common.h

331 ↗

(On Diff #255325)

I want to say that this will break compilation on fuchsia and rtems because you only have definitions for posix + win, but then we have the same situation with GetThreadStackAndTls on fuchsia, and things are somehow fine. Any idea why? Consider adding an inline implementation in the header under the complementary condition (i.e. not-posix).

[robot]

Refactored the way stack bounds are acquired in __asan_handle_no_return. This should fix compilation on Fuchsia and RTEMS.

compiler-rt/lib/asan/asan_rtl.cpp

620 ↗

(On Diff #255325)

You sent me down a rabbit hole there :)
My initial reasoning was indeed flawed, but I still think a false positive is possible in the context. Please see the comment I've added to the source.

compiler-rt/lib/sanitizer_common/sanitizer_common.h

331 ↗

(On Diff #255325)

From a bit of experimentation, I think the dead branches are dropped even at -O0. Therefore there's never a symbol reference for the linker to resolve.

Regarding Fuchsia and RTEMS, I've pondered a bit on where to put things, and came up with another approach. It's more intrusive but avoids noop-functions. Please let me know what you think.

[robot]

Improve portability by supplying fd=-1 to mmap.

[robot]

Fix Windows and MacOS builds:

• #ifdef SANITIZER_POSIX -> #if SANITIZER_POSIX (it's always defined)
• don't address sigaction::sa_restorer member, just let it be implicitly zero-initialized
• use sigemptyset to clear the signal mask

[robot]

Would be great if you could have another look.

[eugenis]

Sorry for the delay.

I find this very hard to read. You add 150 lines to asan_rtl.cpp that do basically nothing. There is maybe 10 lines of the actual logic. I wonder if the same could be expressed in less code, and especially in fewer functions?

[kcc]

also, please avoid #ifdefs.
OS-specific code should go to an OS-specific file.

[vitalybuka]

compiler-rt/lib/asan/asan_rtl.cpp

696 ↗

(On Diff #258685)

Please, extract into a single NFC patch the code
you move existing code for stack calculation into a new function with
common for all platforms. e.g. like the getCurrentStackBounds
and move the current relevant code there. Please put implementation into relevant platform specific files. This will help us avoid ifdef

Then you can rebase D76986 so it will be about behavior changes only without any refactoring.

743 ↗

(On Diff #258685)

historically code in sanitizer_common and other sanitizers follow Google C++ style, not LLVM
please keep it consistent.

compiler-rt/lib/sanitizer_common/sanitizer_posix.h

123 ↗

(On Diff #258685)

Code should try to match style around. These comments looks very different from what is on the rest of sanitizer_common

compiler-rt/lib/sanitizer_common/sanitizer_posix_libcdep.cpp

212 ↗

(On Diff #258685)

this easily can be compiled into libc memset interceptors, that's why we use
internal_memset(&sigact, 0, sizeof(sigact)); around

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

56

I guess all above is irrelevant to the test?
In most case here just ignoring the return code is appropriate solution.

89

it could be more readable if you inline both these functions

[robot]

Split refactoring into separate commit.

[robot]

I'm trying to split this into two changes, please bear with my while I'm trying to figure out Phabricator & Arcanist.

compiler-rt/lib/asan/asan_rtl.cpp

696 ↗

(On Diff #258685)

That's a great idea, thanks! I've reduced the refactoring to a small amount, unfortunately I didn't find a better solution than to introduce customization points for RTEMS, Fuchsia, POSIX and Windows. Logically speaking, those all behave differently, but only slightly. I've struggled with making this difference explicit without duplicating code.

743 ↗

(On Diff #258685)

I see. I was trying to satisfy Harbormaster's clang-format checks, which complained about my original code and suggested LLVM style. I've tried to adapt.

compiler-rt/lib/sanitizer_common/sanitizer_posix.h

123 ↗

(On Diff #258685)

I've seen some doxygen in compiler-rt/lib (e.g. sanitizer_common), so I was trying to use it for the new code.
Reverted to plain C comments, hope that's what you meant?

compiler-rt/lib/sanitizer_common/sanitizer_posix_libcdep.cpp

212 ↗

(On Diff #258685)

That's good to know, thanks! I'm not sure though which patterns I should replace with a memset. The code now looks very different anyway, please have a look.

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

56

In other projects, I encountered intermittent faults in CI / push barriers / continuous testing when using system calls - most likely caused by massive parallelism on the test machine. A test would fail or time out, which is hard to reproduce locally. Any error from functions like mmap or pthread_create are not test failures due to a bug in asan, but they are infrastructure issues.

So yes, I can certainly remove the error checks without affecting the test itself, but I fear that this decreases the confidence that test failures indicate a bug in asan.

89

I'm not sure if I can inline prepareStackShadow, from a logical point of view:
I want to test that the redzones created by the local variable Blob in prepareStackShadow are gone.
But if I place the variable into testHandeNoReturn, then its redzones should logically still be there in the catch clause, because the variable is still accessible.
In practice, the redzones should have been deleted because the throw calls __asan_handle_no_return which removes all redzones on the stack. However, I'm not sure if it's good if the test would rely on this behaviour.

I can inline assertNoRedzones, no objections.

[robot]

Trying to split change into two commits.

[robot]

@eugenis I've tried to minimize impact. Hope it's better now.
@kcc I've left most of the logic now as it was originally, not in an OS-specific file but using plain ifs (not ifdefs). Extracted a customization point for POSIX, however I could only come up with a solution that defines GetStackBounds in four different files.

[vitalybuka]

compiler-rt/lib/asan/asan_posix.cpp

45

most of this comment is more relevant to GetStackBounds than to this function
also it's explains what I already see from the code.
I'd refer you remove it. Maybe move some into patch description.

79

This function looks inconsistent. Sometimes it returns current stack, sometimes the given stack from curr_thread.
Please remove argument from here and move GetCurrentThread(); into GetStackBoundsFromCacheOrOs

compiler-rt/lib/asan/asan_rtems.cpp

67 ↗

(On Diff #268878)

GetStackBounds -> GetCurrentStackBounds

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

105

most comments here and in the lib are quite redundant

112

the test does not cover the case when we throw on default stack but sigaltstack is set.

@eugenis WDTY?
I'd like to ask you to always unpoison both: default and altstack from __asan_handle_no_return.
Currently asan_handle_no_return is just a bailout for exceptions. I don't remember that we tried to avoid false negatives after them. Only that it's not not going to repot false positives. So it should be fine.
Also in case of fake_stack it may unpoison both already.

155

Not sure that all compilers will accepts designated initializers in C++ mode. Runtime is compiled and tested with gcc as well

[eugenis]

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

112

I don't see why we would do that. It's true that we are more concerned with correctness than completeness, but we should not unpoison stuff that we definitely do not need to.

FakeStack is different - it is logically interleaved with the main stack.

[eugenis]

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

112

Hmm, wait, it's possible to longjmp from altstack to the main stack, is it not?
Does it mean we need to unpoison both, at least when the current stack == altstack?

[robot]

Unpoison both stacks always.

To support jumps between stacks, always unpoison both the signal
alternate stack and the default stack.

• [asan] Refactor fetching of stack boundaries
• [asan] Unpoison signal alternate stack.

[robot]

Now unpoisoning both stacks, always.

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

105

lib meaning the asan library (compiler-rt/lib)?
I've removed the last big comment there on your request.
Here in the test I'm trying to explain why the test setup does its steps.

112

Ah, yes, I've even seen this in production code. It's probably possible to jump from main stack to altstack as well, so I've tried to use the simplest solution and unpoison both stacks always.

155

Ah, I was trying to use the C++20 feature but didn't realize the flag is not set in the test command line. I've chosen to set the flag explicitly, but I can also replace the designated initializers of course.

[robot]

Formatting.

• [asan] Refactor fetching of stack boundaries.
• [asan] Unpoison signal alternate stack.

[vitalybuka]

In D76986#2082243, @robot wrote:

Formatting.

• [asan] Refactor fetching of stack boundaries.

can you please move this into a separate review? It's just another NFC patch, no new tests will be needed.

compiler-rt/lib/asan/asan_internal.h

86 ↗

(On Diff #269521)

I'd avoid the list of platforms here as it's may because outdated with time.

compiler-rt/lib/asan/asan_rtl.cpp

568 ↗

(On Diff #269521)

please don't mix refactoring with functional changes in the same patch.
if you want to rename something create another patch.
However, here new naming conversion does not match code in this file, so it should not be renamed like this at all.

578 ↗

(On Diff #269521)

if (bounds.top == bounds.bottom)
  return;

and you can remove have_bounds

613 ↗

(On Diff #269521)

// everywhere
void PlatformUnpoisonStacks() {
  return false; 
}

// linux
void PlatformUnpoisonStacks() {
  return true or false; 
}

void NOINLINE __asan_handle_no_return() {
  if (asan_init_is_running)
    return;

  if (!PlatformUnpoisonStacks())
     UnpoisonStacks();
  UnpoisonCurrentFakeStack();
}

This way only PlatformUnpoisonStacks needs to be in headers and you can drop StackBounds at all
I would not bother to try to get "WARNING: ASan is ignoring requested" from PlatformUnpoisonStacks

Still request about separate refactoring patch is relevant.

if you like to avoid the new patch then we can do the following:

void NOINLINE __asan_handle_no_return() {
  if (asan_init_is_running)
      return;

  if (!PlatformUnpoisonStacks()) {
   // entire block of existing code
   int local_stack;
   ...
   PoisonShadow(bottom, top - bottom, 0);
  }

  if (curr_thread && curr_thread->has_fake_stack())
    curr_thread->fake_stack()->HandleNoReturn();
}

743 ↗

(On Diff #258685)

please use clang-format with --style=file

compiler-rt/lib/asan/asan_win.cpp

273 ↗

(On Diff #269521)

I would prefer we do not let platform to choose not to call UnpoisonCurrentFakeStack();

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

56

it's applies to many other tests, but we don't overload them error handling logic

[robot]

Extracted refactoring into D81577

compiler-rt/lib/asan/asan_rtl.cpp

568 ↗

(On Diff #269521)

Sorry, the renaming was not intentional. I'm running part of the CI linter script locally, and this performed a git-clang-format - essentially what Harbormaster is doing. Will undo.

I misunderstood the request for an NFC change and split the refactoring into its own commit in the same phabricator diff. Will create a new diff out of the refactoring.

613 ↗

(On Diff #269521)

There are two ways to unpoison the default stack in POSIX case:

• If we're on the sigalt stack, try cache first, fall back to asking OS.
• If we're not on the sigalt stack, never use cache.

I've chosen to put the first into UnpoisonStack and inline the second one into PlatformUnpoisonStacks. However, this also suppresses any warning for the default stack bounds in case we're on the sigalt stack.
It is also questionable if we should ever use the cache on Linux: I've left the problem with SS_AUTODISARM unsolved, so in case we're on the sigalt stack but with SS_AUTODISARM, we'll attempt to use the cache and might produce a warning. If we wouldn't use the cache, we'd properly unpoison the default stack.

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

56

Not sure which error handling is relevant, I've removed all error handling now.

[robot]

compiler-rt/lib/asan/asan_posix.cpp

51

In order to properly solve the case "on sigalt stack but SS_AUTODISARM was used", we could intercept sigaltstack and register the signal alternate stack with the thread object in asan. Maybe in another revision.

[vitalybuka]

LGTM with few changes

compiler-rt/lib/asan/asan_posix.cpp

43–67

don't need on_sig_alt_stack

just

if (signal_stack.ss_flags != SS_ONSTACK)
  return false;

66

Please in the first patch extract warning block and call it here (and above)

[robot]

Simplify logic, warn about unlikely stack bounds in PlatformUnpoisonStacks.

[vitalybuka]

Do you need me to land these patches?

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

1

This test should be under compiler-rt/test/asan/TestCases/Posix or even compiler-rt/test/asan/TestCases/Linux

[robot]

In D76986#2091249, @vitalybuka wrote:

Do you need me to land these patches?

Yes, please. I don't have commit rights.

[robot]

Move test to posix subfolder, since it requires posix features.

compiler-rt/lib/asan/asan_posix.cpp

 //===-- asan_posix.cpp ----------------------------------------------------===//
 //
 // 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 is a part of AddressSanitizer, an address sanity checker.
 //
 // Posix-specific details.
 //===----------------------------------------------------------------------===//
 
 #include "sanitizer_common/sanitizer_platform.h"
 #if SANITIZER_POSIX
 
 #include "asan_internal.h"
 #include "asan_interceptors.h"
 #include "asan_mapping.h"
+#include "asan_poisoning.h"
 #include "asan_report.h"
 #include "asan_stack.h"
 #include "sanitizer_common/sanitizer_libc.h"
 #include "sanitizer_common/sanitizer_posix.h"
 #include "sanitizer_common/sanitizer_procmaps.h"
 
 #include <pthread.h>
+#include <signal.h>
 #include <stdlib.h>
 #include <sys/time.h>
 #include <sys/resource.h>
 #include <unistd.h>
 
 namespace __asan {
 
 void AsanOnDeadlySignal(int signo, void *siginfo, void *context) {
   StartReportDeadlySignal();
   SignalContext sig(siginfo, context);
   ReportDeadlySignal(sig);
 }
 
 bool PlatformUnpoisonStacks() {
+  uptr sigalt_top, sigalt_bottom;
+  stack_t signal_stack;
+  CHECK(0 == sigaltstack(nullptr, &signal_stack));
+  sigalt_bottom = (uptr)signal_stack.ss_sp;
+  sigalt_top = (uptr)((char *)signal_stack.ss_sp + signal_stack.ss_size);
+  // If we're executing on the signal alternate stack AND the Linux flag
+  // SS_AUTODISARM was used, then we cannot get the signal alternate stack
+  // bounds from sigaltstack -- sigaltstack's output looks just as if no
+  // alternate stack has ever been set up.
+  // We're always unpoisoning the signal alternate stack to support jumping
+  // between the default stack and signal alternate stack.
+  if (signal_stack.ss_flags != SS_DISABLE)
+    UnpoisonStack(sigalt_bottom, sigalt_top, "sigalt");
+
+  if (signal_stack.ss_flags != SS_ONSTACK)
     return false;
+
+  // Since we're on the signal altnerate stack, we cannot find the DEFAULT
+  // stack bottom using a local variable.
+  uptr default_top, default_bottom, tls_addr, tls_size, stack_size;
+  GetThreadStackAndTls(/*main=*/false, &default_bottom, &stack_size, &tls_addr,
+                       &tls_size);
+  default_top = default_bottom + stack_size;
+  UnpoisonStack(default_bottom, default_top, "default");
+  return true;
 }
 
 // ---------------------- TSD ---------------- {{{1
 
 #if SANITIZER_NETBSD && !ASAN_DYNAMIC
 // Thread Static Data cannot be used in early static ASan init on NetBSD.
 // Reuse the Asan TSD API for compatibility with existing code
 // with an alternative implementation.
 
 static void (*tsd_destructor)(void *tsd) = nullptr;
 
 struct tsd_key {
   tsd_key() : key(nullptr) {}
   ~tsd_key() {
     CHECK(tsd_destructor);
     if (key)
       (*tsd_destructor)(key);
   }
   void *key;
 };

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

+// Tests that __asan_handle_no_return properly unpoisons the signal alternate
+// stack.
+
+// Don't optimize, otherwise the variables which create redzones might be
+// dropped.
+// RUN: %clangxx_asan -std=c++20 -fexceptions -O0 %s -o %t -pthread
+// RUN: %run %t
+
+#include <cassert>
+#include <cerrno>
+#include <csetjmp>
+#include <cstdint>
+#include <cstdio>
+#include <cstdlib>
+#include <cstring>
+
+#include <limits.h>
+#include <pthread.h>
+#include <signal.h>
+#include <sys/mman.h>
+#include <unistd.h>
+
+#include <sanitizer/asan_interface.h>
+
+
+namespace {
+
+struct TestContext {
+  char *LeftRedzone;
+  char *RightRedzone;
+  std::jmp_buf JmpBuf;
+};
+
+TestContext defaultStack;
+TestContext signalStack;
+
+// Create a new stack frame to ensure that logically, the stack frame should be
+// unpoisoned when the function exits. Exit is performed via jump, not return,
+// such that we trigger __asan_handle_no_return and not ordinary unpoisoning.
+template <class Jump>
+void __attribute__((noinline)) poisonStackAndJump(TestContext &c, Jump jump) {
+  char Blob[100]; // This variable must not be optimized out, because we use it
+                  // to create redzones.
+
+  c.LeftRedzone = Blob - 1;
+  c.RightRedzone = Blob + sizeof(Blob);
+
+  assert(__asan_address_is_poisoned(c.LeftRedzone));
+  assert(__asan_address_is_poisoned(c.RightRedzone));
+
+  // Jump to avoid normal cleanup of redzone markers. Instead,
+  // __asan_handle_no_return is called which unpoisons the stacks.
+  jump();
+}
+
+void testOnCurrentStack() {
+  TestContext c;
+
+  if (0 == setjmp(c.JmpBuf))
+    poisonStackAndJump(c, [&] { longjmp(c.JmpBuf, 1); });
+
+  assert(0 == __asan_region_is_poisoned(c.LeftRedzone,
+                                        c.RightRedzone - c.LeftRedzone));
+}
+
+void signalHandler(int, siginfo_t *, void *) {
+  {
+    stack_t Stack;
+    sigaltstack(nullptr, &Stack);
+    assert(Stack.ss_flags == SS_ONSTACK);
+  }
+
+  // test on signal alternate stack
+  testOnCurrentStack();
+
+  // test unpoisoning when jumping between stacks
+  poisonStackAndJump(signalStack, [] { longjmp(defaultStack.JmpBuf, 1); });
+}
+
+void setSignalAlternateStack(void *AltStack) {
+  sigaltstack((stack_t const *)AltStack, nullptr);
+
+  struct sigaction Action = {
+      .sa_sigaction = signalHandler,
+      .sa_flags = SA_SIGINFO | SA_NODEFER | SA_ONSTACK,
+  };
+  sigemptyset(&Action.sa_mask);
+
+  sigaction(SIGUSR1, &Action, nullptr);
+}
+
+// Main test function.
+// Must be run on another thread to be able to control memory placement between
+// default stack and alternate signal stack.
+// If the alternate signal stack is placed in close proximity before the
+// default stack, __asan_handle_no_return might unpoison both, even without
+// being aware of the signal alternate stack.
+// We want to test reliably that __asan_handle_no_return can properly unpoison
+// the signal alternate stack.
+void *threadFun(void *AltStack) {
+  // first test on default stack (sanity check), no signal alternate stack set
+  testOnCurrentStack();
+
+  setSignalAlternateStack(AltStack);
+
+  // test on default stack again, but now the signal alternate stack is set
+  testOnCurrentStack();
+
+  // set up jump to test unpoisoning when jumping between stacks
+  if (0 == setjmp(defaultStack.JmpBuf))
+    // Test on signal alternate stack, via signalHandler
+    poisonStackAndJump(defaultStack, [] { raise(SIGUSR1); });
+
+  assert(0 == __asan_region_is_poisoned(
+                  defaultStack.LeftRedzone,
+                  defaultStack.RightRedzone - defaultStack.LeftRedzone));
+
+  assert(0 == __asan_region_is_poisoned(
+                  signalStack.LeftRedzone,
+                  signalStack.RightRedzone - signalStack.LeftRedzone));
+
+  return nullptr;
+}
+
+} // namespace
+
+// Check that __asan_handle_no_return properly unpoisons a signal alternate
+// stack.
+// __asan_handle_no_return tries to determine the stack boundaries and
+// unpoisons all memory inside those. If this is not done properly, redzones for
+// variables on can remain in shadow memory which might lead to false positive
+// reports when the stack is reused.
+int main() {
+  size_t const PageSize = sysconf(_SC_PAGESIZE);
+  // To align the alternate stack, we round this up to page_size.
+  size_t const DefaultStackSize =
+      (PTHREAD_STACK_MIN - 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;
+  // Using mmap guarantees proper alignment.
+  void *const Mapping = mmap(nullptr, MappingSize,
+                             PROT_READ | PROT_WRITE,
+                             MAP_PRIVATE | MAP_ANONYMOUS,
+                             -1, 0);
+
+  stack_t const AltStack = {
+      .ss_sp = (char *)Mapping + DefaultStackSize,
+      .ss_flags = 0,
+      .ss_size = AltStackSize,
+  };
+
+  pthread_t Thread;
+  pthread_attr_t ThreadAttr;
+  pthread_attr_init(&ThreadAttr);
+  pthread_attr_setstack(&ThreadAttr, Mapping, DefaultStackSize);
+  pthread_create(&Thread, &ThreadAttr, &threadFun, (void *)&AltStack);
+
+  pthread_join(Thread, nullptr);
+
+  munmap(Mapping, MappingSize);
+}