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

[Siva Chandra via Phabricator via libc-commits]

sivachandra accepted this revision.
sivachandra marked an inline comment as done.
sivachandra added inline comments.
This revision is now accepted and ready to land.


================
Comment at: libc/src/stdlib/abort.cpp:39
+  __llvm_libc::signal(SIGABRT, SIG_DFL);
   __llvm_libc::raise(SIGABRT);
+
----------------
abrachet wrote:
> 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 :)
I think the truly correct way would be to use a recursive lock. Until then, this seems fine.


================
Comment at: libc/test/src/stdlib/CMakeLists.txt:26
     raise
+    # TODO: Remove depencies of abort
+    sigaction
----------------
sivachandra wrote:
> I think I have an idea on how to do this. But, what you have is also OK in the meanwhile.
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.


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

https://reviews.llvm.org/D76676