Re: [musl] [PATCH] Split fork and abort locks

[Alexey Izbyshev <izbyshev@ispras.ru>]

On 2024-01-20 11:12, Markus Wichmann wrote:
> Hi all,
> 
> a while ago I had noticed that __abort_lock was being taken in some
> functions that have nothing to do with SIGABRT. Namely in the forking
> functions. Investigating this a bit, I noticed that __abort_lock had
> become dual purpose. But this is a code smell.
> 
> Actually, there are several locks that have expanded in scope a bit
> since their introduction. At least the ptc lock (__inhibit_ptc() et 
> al.)
> deserves a closer look later on as well. Seems to me like in case of 
> the
> default stack size, that lock is used simply because an rwlock was
> needed and this one was around.
> 
> The corruption in this case probably came from posix_spawn(). That
> function takes the abort lock, because, as a baffling comment above the
> lock statement tells us, this guards against SIGABRT disposition
> changing. This is because abort() changes the disposition without
> calling sigaction(), so a handler would not be noted in the handler 
> set.
> 
> However, actually posix_spawn() does not need to care about this. The
> handler set is strictly additive, so all the signals it contains /may/
> have a handler. And abort() removes a handler. In the worst case, the
> handler set will spuriously contain SIGABRT, which is a condition the
> child needs to check for anyway.
> 
> And a concurrent sigaction() call from the application establishing a
> handler is no different for SIGABRT than for any other signal. That is
> handled by retrieving the handler set in the child, when everything is
> fixed since the child is single-threaded. For the same reason, there
> cannot be a concurrent call to abort() in the child.
> 
The problem that __abort_lock solves is that a child process created by 
fork/_Fork/clone/posix_spawn should not observe SIGABRT disposition 
reset  if abort is called by the parent concurrently with the child 
creation. For example, if the initial SIGABRT disposition is SIG_IGN, 
and one thread of a program calls posix_spawn while another thread calls 
abort, without __abort_lock the child could inherit SIG_DFL disposition. 
This is not related to maintaining handler_set used for posix_spawn 
optimization.

A separate reason for why removing __abort_lock LOCK/UNLOCK from clone.c 
and _Fork.c (as done in your patch) is wrong is because they take part 
in creation of a consistent execution state in the child (the child 
should be able to call abort and not deadlock).

Alexey