[libc-commits] [PATCH] D76676: [libc] Unblock SIGABRT before raising in abort

Fri Mar 27 11:28:35 PDT 2020

abrachet marked 7 inline comments as done.
abrachet added inline comments.

================
Comment at: libc/src/stdlib/abort.cpp:23
+
 void LLVM_LIBC_ENTRYPOINT(abort)() {
+  sigset_t set;
----------------
PaulkaToast wrote:
> sivachandra wrote:
> > abrachet wrote:
> > > 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.
> > 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?
> 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?
I've added a comment. Not sure if its sufficient.

================
Comment at: libc/src/stdlib/abort.cpp:27
+  __llvm_libc::sigaddset(&set, SIGABRT);
+  __llvm_libc::sigprocmask(SIG_UNBLOCK, &set, nullptr);
+
----------------
PaulkaToast wrote:
> sivachandra wrote:
> > 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.
> 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?
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.

================
Comment at: libc/src/stdlib/abort.cpp:35
+    // Unblock again incase the handler caught then blocked it.
+    __llvm_libc::sigprocmask(SIG_UNBLOCK, &set, nullptr);
+  }
----------------
sivachandra wrote:
> Can this unblock be moved out of this `if` statement, and the above unblock be removed?
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).

================
Comment at: libc/src/stdlib/abort.cpp:38
+
+  __llvm_libc::signal(SIGABRT, SIG_DFL);
   __llvm_libc::raise(SIGABRT);
----------------
PaulkaToast wrote:
> sivachandra wrote:
> > Why not use `sigaction`?
> 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.
> 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.

================
Comment at: libc/src/stdlib/abort.cpp:39
+  __llvm_libc::signal(SIGABRT, SIG_DFL);
   __llvm_libc::raise(SIGABRT);
+
----------------
sivachandra wrote:
> PaulkaToast wrote:
> > 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?  
> That's a good point. We can get creative with some locking may be to test this?
>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 :)

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76676/new/

https://reviews.llvm.org/D76676