[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240120212410.GX4163@brightrain.aerifal.cx>
Date: Sat, 20 Jan 2024 16:24:11 -0500
From: Rich Felker <dalias@...c.org>
To: musl@...ts.openwall.com
Subject: Re: [PATCH] Split fork and abort locks
On Sat, Jan 20, 2024 at 02:29:28PM +0300, Alexey Izbyshev wrote:
> 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).
Yes. In order to separate these locks, most places that take either
lock would have to take both locks. This is more costly and
error-prone, and has no advantages I can see.
Rich
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
