From 18bdb28ef603012a5516d6fbf997859c2dec5323 Mon Sep 17 00:00:00 2001 From: Vyacheslav Egorov Date: Mon, 23 Mar 2020 21:49:37 +0000 Subject: [PATCH] [vm/profiler] On Android use alternative stack for handling SIGPROF. Bionic implementation of setjmp mangles[1] stack pointer - which means it is unsafe to handle signals on the thread stack (see b/152210274). Thread interrupter is constantly sending SIGPROF to the Dart thread - which means with a small probability it might hit the case when we are inside setjmp. If SP is mangled it might point to random writable memory or to non-writable region. In the first case we will get a very obscure memory corruption, and in the second case kernel would send us SIGSEGV because it fails to deliver original signal. This bug is the source of the numerous mysterious crashes reported for Flutter, looking like this: F/libc (11547): Fatal signal 11 (SIGSEGV), code 128, fault addr 0x0 in tid 11572 (1.ui), pid 11547 (ectivity_change) ... signal 11 (SIGSEGV), code 128 (SI_KERNEL), fault addr 0x0 ... backtrace: #00 pc 00018abc /system/lib/libc.so (sigsetjmp+120) Note the following key points: SIGSEGV has code SI_KERNEL (meaning it was triggered by kernel - rather than by a hardware fault) and the first and only frame is inside sigsetjmp (unwinding is obviously also broken because SP is mangled). Fixes https://github.com/flutter/flutter/issues/27077 [1] https://android.googlesource.com/platform/bionic/+/refs/heads/master/libc/arch-x86/bionic/setjmp.S#132 Change-Id: I91afa42dbf6575db0cce8e223368b857a49b39b8 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/140643 Reviewed-by: Ryan Macnak Commit-Queue: Vyacheslav Egorov --- runtime/vm/signal_handler_android.cc | 27 +++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-) diff --git a/runtime/vm/signal_handler_android.cc b/runtime/vm/signal_handler_android.cc index f9f6944cd3d..cdef00fc363 100644 --- a/runtime/vm/signal_handler_android.cc +++ b/runtime/vm/signal_handler_android.cc @@ -95,11 +95,27 @@ uintptr_t SignalHandler::GetLinkRegister(const mcontext_t& mcontext) { } void SignalHandler::InstallImpl(SignalAction action) { + // Bionic implementation of setjmp temporary mangles SP register + // in place which breaks signal delivery on the thread stack - when + // kernel tries to deliver SIGPROF and we are in the middle of + // setjmp SP value is invalid - might be pointing to random memory + // or outside of writable space at all. In the first case we + // get memory corruption and in the second case kernel would send + // SIGSEGV to the process. See b/152210274 for details. + // To work around this issue we are using alternative signal stack + // to handle SIGPROF signals. + stack_t ss; + ss.ss_size = SIGSTKSZ; + ss.ss_sp = malloc(ss.ss_size); + ss.ss_flags = 0; + int r = sigaltstack(&ss, NULL); + ASSERT(r == 0); + struct sigaction act = {}; act.sa_sigaction = action; sigemptyset(&act.sa_mask); - act.sa_flags = SA_RESTART | SA_SIGINFO; - int r = sigaction(SIGPROF, &act, NULL); + act.sa_flags = SA_RESTART | SA_SIGINFO | SA_ONSTACK; + r = sigaction(SIGPROF, &act, NULL); ASSERT(r == 0); } @@ -111,6 +127,13 @@ void SignalHandler::Remove() { sigemptyset(&act.sa_mask); int r = sigaction(SIGPROF, &act, NULL); ASSERT(r == 0); + + // Disable and delete alternative signal stack. + stack_t ss, old_ss; + ss.ss_flags = SS_DISABLE; + r = sigaltstack(&ss, &old_ss); + ASSERT(r == 0); + free(old_ss.ss_sp); } } // namespace dart