[libc] Unblock SIGABRT before raising in abort

[abrachet]

Summary

Now that signal support is more flushed out it is possible to unblock SIGABRT before raising it and setting its action to default before raising a second time.

[abrachet]

libc/src/stdlib/abort.cpp

23–24

The previous comment mentioned locks, it's a little more complicated than I originally imagined. We would also need locks in sigaction to ensure if one thread is aborting no others can change SIGABRT's action. Even with the recent mutex patch they still cannot be used because as previously explained in the comment we need recursive mutexes. I could add a comment as a TODO here (and in sigaction) but I wonder how much we care about this behavior, so I have waited before adding such a comment.

[sivachandra]

libc/src/stdlib/abort.cpp

23–24

I agree. May be you can add a comment saying we will not worry about MT-safe behavior for now? And also explain how it should all work when we have the recursive mutex available?

27

Is an unblock required before the first raise? If the application blocked it, the first raise would just return, no? May be this is related to my other comment below.

35

Can this unblock be moved out of this if statement, and the above unblock be removed?

38

Why not use sigaction?

libc/test/src/stdlib/CMakeLists.txt

26

I think I have an idea on how to do this. But, what you have is also OK in the meanwhile.

[PaulkaToast]

libc/src/stdlib/abort.cpp

23–24

I agree that the recursive case is much more likely to occur. I don't feel there is a need to complicate with the multi-thread cases. The only difference I see it would exit with a SIGKILL instead of a SIGABRT, right?

27

The standard doesn't specify if we should unblock, so it might make our code simpler to remove this line. Then maybe the recursive situation is handled without the need for the if (!called) because the raise function blocks all incoming signals?

38

I think it would be nice to have a comment here explaining the reason why we install the default sigabrt handler after raising sigabrt above.

Maybe even quote the relevant part of the standard?

The abort function causes abnormal program termination to occur, unless the signal SIGABRT is being caught and the signal handler does not return

ie this is handling the case where the SIGABRT is caught but the handler does return, so we must terminate the program.

44–47

Aside from the multi-thread case, I think there is a single thread re-entry issue here. A signal can be raised in between the execution of this line and the previous line (e.g. from an external program or alarm) that installs the default handler. A possible solution might be to block all signals for this segment?

48

I think add a comment to explain this is our last ditch effort to kill the program if all other attempts have failed.

libc/test/src/stdlib/abort_test.cpp

54

I'd like to see a test-case for the single thread re-entry issue, however I'm not entirely sure if that can be reliably tested. Mind looking into it? If its not possible that's fine. (:

[sivachandra]

libc/src/stdlib/abort.cpp

44–47

That's a good point. We can get creative with some locking may be to test this?

[abrachet]

• Block all signals except for SIGABRT
• Add more comments

[abrachet]

libc/src/stdlib/abort.cpp

23–24

I've added a comment. Not sure if its sufficient.

27

I think that the first unblock is useful in case it is currently blocked and there is a user handler installed. Otherwise we would make the first raise, it does nothing, we unblock then set its handler to the default and we die to that and skip over the user handler.

I've taken @PaulkaToast's suggestion about async signals into account and I just block all except SIGABRT here so this block now both blocks all and unblocks SIGABRT.

35

I think this is useful here in the case that the user handler changes the signal mask and also returns. I could also move it out if it seems more simple, but I think this is only meaningful when we go into this if statement because thats the only time the signal disposition could change (except for other threads changing it).

38

Why not use sigaction?

Our internal use of sigaction is pretty complicated we have the internal sigaction macro which needs to be defined and I think its less complicated to just use signal here. I could use sigaction here if you think it is preferable though.

44–47

Aside from the multi-thread case, I think there is a single thread re-entry issue here. A signal can be raised in between the execution of this line and the previous line (e.g. from an external program or alarm) that installs the default handler. A possible solution might be to block all signals for this segment?

Interesting, I hadn't thought of that :).

Signals are very poorly designed in my opinion (Fuchsia even omits them). We just cant guard against this easily because they are process wide. I've changed from unblocking SIGABRT at the beginning to blocking all except SIGABRT to try and not get interrupted by async signals. But another thread could change this unfortunately. We could (but I think maybe we should not) use locks here which sigaction must acquire before changing the signal mask to make sure once we enter abort no other thread can touch signals. At this point though, so many things must come together for this to happen that someone would have to be trying, and they could just make the sigaction syscall without the libc wrapper and go past our locks :P.

Like you mentioned above, worst case here is that the program dies to SIGKILL not SIGABRT. Of course I'll implement whatever we collectively think is necessary, though.

That's a good point. We can get creative with some locking may be to test this?

I might not be creative enough but the only way I could think to deterministically test this is with lldb in a lit style test :)

[abrachet]

Don't block before first raise call, only once.

[sivachandra]

Looking at this now.

[sivachandra]

libc/src/stdlib/abort.cpp

44–47

I think the truly correct way would be to use a recursive lock. Until then, this seems fine.

libc/test/src/stdlib/CMakeLists.txt

26

Can you make this TODO (sivachandra): ... so that I it would be easy for me to look it up?

I started working on fixing this, but then I found one bug (https://reviews.llvm.org/D77256), and then also realized I should check if we want to do this: https://reviews.llvm.org/D77340. If we decide to keep the latter, I prefer to put in first before I make any new changes to tracking deps.

[MaskRay]

libc/src/stdlib/abort.cpp

26

The function-local variable is not needed. If the signal handler of SIGABRT is set to abort, the following blocking logic should work.

[MaskRay]

libc/src/stdlib/abort.cpp

34

Another thread may concurrently install a signal handler for SIGABRT or unblock SIGABRT. There needs to be a lock.

[abrachet]

libc/src/stdlib/abort.cpp

26

On MacOS

::signal(SIGABRT, +[] (int) { ::abort(); });
::abort();

This dies to SIGSEGV, presumably from a stack overflow. This is not ideal.

34

Yes this was discussed, we need a recursive lock here but also in sigaction, we don't have those yet. I don't personally think the benefits are worth that complexity anyway though.

[MaskRay]

libc/src/stdlib/abort.cpp

34

This suggests that the patches should be applied in a better way. If a mutex is implemented before improving abort, then we don't need to care about this.

I think a non-recursive mutex suffices here.

[sivachandra]

libc/src/stdlib/abort.cpp

34

Perfect is the enemy of the good: https://en.wikipedia.org/wiki/Perfect_is_the_enemy_of_good
FWIW, this albeit incomplete implementation allowed us to make progress with other pieces. Not to claim other pieces are complete or perfect. As I said in another post, the quest for perfection hinders progress, and more importantly, hinders our own learning.

[MaskRay]

libc/src/stdlib/abort.cpp

34

I am not sure this is the excuse to check in the imperfect code at this point. Actually the simply abort() is sufficient for 99.9% use cases. Nobody will sigaction SIGABRT to abort itself. When the other infrastructure is not ready, it is premature to over-optimize abort. You can disregard it but that will eventually clutter up the history of llvm-libc.

A messy history will just hinder learning for posterity, which may be a lot of people.

[sivachandra]

libc/src/stdlib/abort.cpp

34

You seem to argue for perfection and imperfection at the same time (because this patch is trying to reduce the existing imperfection). It is probably getting into subjective opinions at this point so I am going to let @alexbrachet decide if they want to submit it as is, or if they want to wait for a recursive mutex implementation. I am OK with either approach as I don't think "clutter up history" is an issue.

[abrachet]

libc/src/stdlib/abort.cpp

34

I could add a lock here and make sigaction and sigprocmask acquire it. But I really wonder if it is worth the added complexity to handle this case. Also, what if the user handler longjmps? Then the next call to abort will be a deadlock too. I don't think there is 0 tradeoff here, it isn't obvious as you make it seem that we must handle this case. The standard says very little about what is required here. @MaskRay if you feel strongly then I will add it, its a trivial change on my end.

libc/src/assert/CMakeLists.txt

 add_entrypoint_object(
   __assert_fail
   SRCS
     __assert_fail.cpp
   HDRS
     assert.h
   DEPENDS
     abort
     # These two dependencies are temporary and should be replaced by fprintf
     # later.
     sys_syscall_h
     linux_syscall_h
+    # TOOD: Remove these as they are dependencies of abort
+    sigaction
+    sigfillset
+    sigdelset
+    sigprocmask
+    signal
+    __errno_location
 )

libc/src/signal/linux/CMakeLists.txt

add_entrypoint_object(
   SRCS
     signal.cpp
   HDRS
     signal.h
     ../signal.h
   DEPENDS
     sigaction
     signal_h
+    # TODO: Remove this it is a dependency of sigaction
+    __errno_location
 )
 
 add_entrypoint_object(
   sigfillset
   SRCS
     sigfillset.cpp
   HDRS
     signal.h

libc/src/stdlib/CMakeLists.txt

 if(EXISTS ${CMAKE_CURRENT_SOURCE_DIR}/${LIBC_TARGET_OS})
   add_subdirectory(${LIBC_TARGET_OS})
 endif()
 
 add_entrypoint_object(
   abort
   SRCS
     abort.cpp
   HDRS
     abort.h
   DEPENDS
     raise
     _Exit
     stdlib_h
+    sigfillset
+    sigdelset
+    sigprocmask
+    signal
+    # TODO: Remove this, dependency of signal via sigaction
+    __errno_location
 )

libc/src/stdlib/abort.cpp

 //===---------------------- Implementation of abort -----------------------===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 
 #include "src/__support/common.h"
 #include "src/signal/raise.h"
+#include "src/signal/sigdelset.h"
+#include "src/signal/sigfillset.h"
+#include "src/signal/signal.h"
+#include "src/signal/sigprocmask.h"
 #include "src/stdlib/_Exit.h"
 
 #include "src/stdlib/abort.h"
 
 namespace __llvm_libc {
 
+// Currently the signal implementation doesn't guarantee that SIGABRT's handler
+// or the signal mask as a whole is not modified from other threads.
 void LLVM_LIBC_ENTRYPOINT(abort)() {
-  // TODO: When sigprocmask and sigaction land:
-  // Unblock SIGABRT, raise it, if it was ignored or the handler returned,
-  // change its action to SIG_DFL, raise it again.
-  // TODO: When C11 mutexes land:
-  // Aquire recursive mutex (in case the current signal handler for SIGABRT
-  // itself calls abort we don't want to deadlock on the same thread trying
-  // to aquire it's own mutex.)
+  // Block all blockable signals except SIGABRT, so this function doesn't get
+  // interrupted by any async signals. This also unblocks SIGABRT.
+  sigset_t set;
+  __llvm_libc::sigfillset(&set);
+  __llvm_libc::sigdelset(&set, SIGABRT);
+  __llvm_libc::sigprocmask(SIG_SETMASK, &set, nullptr);
+
+  // If the current SIGABRT handler calls abort(), we should make sure it
+  // it does not infinitely call itself.
+  static bool called;
+  if (!called) {
+    called = true;
     __llvm_libc::raise(SIGABRT);
+    // Reset the signal mask in case the user handler changed it.
+    __llvm_libc::sigprocmask(SIG_SETMASK, &set, nullptr);
+  }
+
+  // If the user handler returned, we re-raise SIGABRT with the default handler
+  // to attempt to die with SIGABRT.
+  __llvm_libc::signal(SIGABRT, SIG_DFL);
+  __llvm_libc::raise(SIGABRT);
+
+  // If the program still hasn't been killed by SIGABRT we must guarantee that
+  // this function never returns.
   __llvm_libc::raise(SIGKILL);
   __llvm_libc::_Exit(127);
 }
 
 } // namespace __llvm_libc

libc/test/src/assert/CMakeLists.txt

 add_libc_testsuite(libc_assert_unittests)
 
 add_libc_unittest(
   assert_test
   SUITE
     libc_assert_unittests
   SRCS
     assert_test.cpp
   DEPENDS
     __assert_fail
     # These are necessary for now because dependencies are not properly added.
     abort
     raise
     _Exit
+    sigaction
+    sigfillset
+    sigdelset
+    sigprocmask
+    signal
+    __errno_location
 )

libc/test/src/assert/assert_test.cpp

 //===---------------------- Unittests for assert --------------------------===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 
 #undef NDEBUG
+#include "include/signal.h"
 #include "src/assert/assert.h"
 #include "utils/UnitTest/Test.h"
 
 extern "C" int close(int);
 
 TEST(Assert, Enabled) {
-  // -1 matches against any signal, which is necessary for now until
-  // __llvm_libc::abort() unblocks SIGABRT. Close standard error for the
-  // child process so we don't print the assertion failure message.
+  // Close standard error for the child process so we don't print the assertion
+  // failure message.
   EXPECT_DEATH(
       [] {
         close(2);
         assert(0);
       },
-      -1);
+      SIGABRT);
 }
 
 #define NDEBUG
 #include "src/assert/assert.h"
 
 TEST(Assert, Disabled) {
   EXPECT_EXITS([] { assert(0); }, 0);
 }

libc/test/src/stdlib/CMakeLists.txt

add_libc_unittest(
   SRCS
     abort_test.cpp
   DEPENDS
     stdlib_h
     signal_h
     abort
     _Exit
     raise
+    # TODO: Remove depencies of abort
+    sigaction
+    sigemptyset
+    sigaddset
+    sigfillset
+    sigdelset
+    sigprocmask
+    signal
+    __errno_location
 )

libc/test/src/stdlib/abort_test.cpp

 //===----------------------- Unittests for abort --------------------------===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 
 #include "include/signal.h"
 #include "include/stdlib.h"
+#include "src/signal/sigaddset.h"
+#include "src/signal/sigemptyset.h"
+#include "src/signal/signal.h"
+#include "src/signal/sigprocmask.h"
 #include "src/stdlib/abort.h"
 #include "utils/UnitTest/Test.h"
 
-TEST(Stdlib, abort) {
-  // -1 matches against any signal, which is necessary for now until
-  // __llvm_libc::abort() unblocks SIGABRT.
-  EXPECT_DEATH([] { __llvm_libc::abort(); }, -1);
+TEST(Abort, Basic) {
+  EXPECT_DEATH([] { __llvm_libc::abort(); }, SIGABRT);
+}
+
+TEST(Abort, Blocked) {
+  __llvm_libc::signal(SIGABRT, SIG_IGN);
+  EXPECT_DEATH([] { __llvm_libc::abort(); }, SIGABRT);
+}
+
+TEST(Abort, RecursiveHandler) {
+  __llvm_libc::signal(
+      SIGABRT, +[](int) { __llvm_libc::abort(); });
+  EXPECT_DEATH([] { __llvm_libc::abort(); }, SIGABRT);
+}
+
+TEST(Abort, BlockFromHandler) {
+  __llvm_libc::signal(
+      SIGABRT, +[](int) {
+        sigset_t set;
+        __llvm_libc::sigemptyset(&set);
+        __llvm_libc::sigaddset(&set, SIGABRT);
+        __llvm_libc::sigprocmask(SIG_BLOCK, &set, nullptr);
+      });
+  EXPECT_DEATH([] { __llvm_libc::abort(); }, SIGABRT);
+}
+
+TEST(Abort, BlockFromHandlerRecursive) {
+  __llvm_libc::signal(
+      SIGABRT, +[](int) {
+        sigset_t set;
+        __llvm_libc::sigemptyset(&set);
+        __llvm_libc::sigaddset(&set, SIGABRT);
+        __llvm_libc::sigprocmask(SIG_BLOCK, &set, nullptr);
+        __llvm_libc::abort();
+      });
+  EXPECT_DEATH([] { __llvm_libc::abort(); }, SIGABRT);
 }