From mboxrd@z Thu Jan 1 00:00:00 1970 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=-3.3 required=5.0 tests=MAILING_LIST_MULTI, RCVD_IN_DNSWL_MED,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL autolearn=ham autolearn_force=no version=3.4.4 Received: (qmail 23371 invoked from network); 27 Oct 2020 21:17:53 -0000 Received: from mother.openwall.net (195.42.179.200) by inbox.vuxu.org with ESMTPUTF8; 27 Oct 2020 21:17:53 -0000 Received: (qmail 9974 invoked by uid 550); 27 Oct 2020 21:17:48 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Reply-To: musl@lists.openwall.com Received: (qmail 9956 invoked from network); 27 Oct 2020 21:17:48 -0000 Date: Tue, 27 Oct 2020 17:17:35 -0400 From: Rich Felker To: musl@lists.openwall.com Message-ID: <20201027211735.GV534@brightrain.aerifal.cx> References: <20201026005028.GI534@brightrain.aerifal.cx> <20201026005912.GJ534@brightrain.aerifal.cx> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20201026005912.GJ534@brightrain.aerifal.cx> User-Agent: Mutt/1.5.21 (2010-09-15) Subject: Re: [musl] Status report and MT fork On Sun, Oct 25, 2020 at 08:59:20PM -0400, Rich Felker wrote: > On Sun, Oct 25, 2020 at 08:50:29PM -0400, Rich Felker wrote: > > I just pushed a series of changes (up through 0b87551bdf) I've had > > queued for a while now, some of which had minor issues that I think > > have all been resolved now. They cover a range of bugs found in the > > process of reviewing the possibility of making fork provide a > > consistent execution environment for the child of a multithreaded > > parent, and a couple unrelated fixes. > > > > Based on distro experience with musl 1.2.1, I'm working on getting the > > improved fork into 1.2.2. Despite the fact that 1.2.1 did not break > > anything that wasn't already broken (apps invoking UB in MT-forked > > children), prior to it most of the active breakage was hit with very > > low probability, so there were a lot of packages people *thought* were > > working, that weren't, and feedback from distros seems to be that > > getting everything working as reliably as before (even if it was > > imperfect and dangerous before) is not tractable in any reasonable > > time frame. And in particular, I'm concerned about language runtimes > > like Ruby that seem to have a contract with applications they host to > > support MT-forked children. Fixing these is not a matter of fixing a > > finite set of bugs but fixing a contract, which is likely not > > tractable. > > > > Assuming it goes through, the change here will be far more complete > > than glibc's handling of MT-forked children, where most things other > > than malloc don't actually work, but fail sufficiently infrequently > > that they seem to work. While there are a lot of things I dislike > > about this path, one major thing I do like is that it really makes > > internal use of threads by library code (including third party libs) > > transparent to the application, rather than "transparent, until you > > use fork". > > > > Will follow up with draft patch for testing. > > Patch attached. It should suffice for testing and review of whether > there are any locks/state I overlooked. It could possibly be made less > ugly too. > > Note that this does not strictly conform to past and current POSIX > that specify fork as AS-safe. POSIX-future has removed fork from the > AS-safe list, and seems to have considered its original inclusion > erroneous due to pthread_atfork and existing implementation practice. > The patch as written takes care to skip all locking in single-threaded > parents, so it does not break AS-safety property in single-threaded > programs that may have made use of it. -Dfork=_Fork can also be used > to get an AS-safe fork, but it's not equivalent to old semantics; > _Fork does not run atfork handlers. It's also possible to static-link > an alternate fork implementation that provides its own pthread_atfork > and calls _Fork, if really needed for a particular application. > > Feedback from distro folks would be very helpful -- does this fix all > the packages that 1.2.1 "broke"? Another bug: > diff --git a/src/process/fork.c b/src/process/fork.c > index a12da01a..ecf7f376 100644 > --- a/src/process/fork.c > +++ b/src/process/fork.c > @@ -1,13 +1,81 @@ > #include > #include "libc.h" > +#include "lock.h" > +#include "pthread_impl.h" > +#include "fork_impl.h" > + > +static volatile int *const dummy_lockptr = 0; > + > +weak_alias(dummy_lockptr, __at_quick_exit_lockptr); > +weak_alias(dummy_lockptr, __atexit_lockptr); > +weak_alias(dummy_lockptr, __dlerror_lockptr); > +weak_alias(dummy_lockptr, __gettext_lockptr); > +weak_alias(dummy_lockptr, __random_lockptr); > +weak_alias(dummy_lockptr, __sem_open_lockptr); > +weak_alias(dummy_lockptr, __stdio_ofl_lockptr); > +weak_alias(dummy_lockptr, __syslog_lockptr); > +weak_alias(dummy_lockptr, __timezone_lockptr); > +weak_alias(dummy_lockptr, __bump_lockptr); > + > +weak_alias(dummy_lockptr, __vmlock_lockptr); > + > +static volatile int *const *const atfork_locks[] = { > + &__at_quick_exit_lockptr, > + &__atexit_lockptr, > + &__dlerror_lockptr, > + &__gettext_lockptr, > + &__random_lockptr, > + &__sem_open_lockptr, > + &__stdio_ofl_lockptr, > + &__syslog_lockptr, > + &__timezone_lockptr, > + &__bump_lockptr, > +}; > > static void dummy(int x) { } > weak_alias(dummy, __fork_handler); > +weak_alias(dummy, __malloc_atfork); > +weak_alias(dummy, __ldso_atfork); > + > +static void dummy_0(void) { } > +weak_alias(dummy_0, __tl_lock); > +weak_alias(dummy_0, __tl_unlock); > > pid_t fork(void) > { > + sigset_t set; > __fork_handler(-1); > + __block_app_sigs(&set); > + int need_locks = libc.need_locks > 0; > + if (need_locks) { > + __ldso_atfork(-1); > + __inhibit_ptc(); > + for (int i=0; i + if (atfork_locks[i]) LOCK(*atfork_locks[i]); ^^^^^^^^^^^^^^^ Always non-null because it's missing a level of indirection; causes static linked program to crash. Should be if (*atfork_locks[i]). > + __malloc_atfork(-1); > + __tl_lock(); > + } > + pthread_t self=__pthread_self(), next=self->next; > pid_t ret = _Fork(); > + if (need_locks) { > + if (!ret) { > + for (pthread_t td=next; td!=self; td=td->next) > + td->tid = -1; > + if (__vmlock_lockptr) { > + __vmlock_lockptr[0] = 0; > + __vmlock_lockptr[1] = 0; > + } > + } > + __tl_unlock(); > + __malloc_atfork(!ret); > + for (int i=0; i + if (atfork_locks[i]) ^^^^^^^^^^^^^^^ And same here. > + if (ret) UNLOCK(*atfork_locks[i]); > + else **atfork_locks[i] = 0; > + __release_ptc(); > + __ldso_atfork(!ret); > + } > + __restore_sigs(&set); > __fork_handler(!ret); > return ret; > }