Re: [PATCH] Make abort() AS-safe (Bug 26275).

[Rich Felker <dalias@libc.org>]

On Sun, Sep 27, 2020 at 10:04:02PM +0200, Florian Weimer wrote:
> * Carlos O'Donell via Libc-alpha:
> 
> > diff --git a/stdlib/Makefile b/stdlib/Makefile
> > index 4615f6dfe7..cd470e53ac 100644
> > --- a/stdlib/Makefile
> > +++ b/stdlib/Makefile
> > @@ -87,7 +87,7 @@ tests		:= tst-strtol tst-strtod testmb testrand testsort testdiv   \
> >  		   tst-makecontext-align test-bz22786 tst-strtod-nan-sign \
> >  		   tst-swapcontext1 tst-setcontext4 tst-setcontext5 \
> >  		   tst-setcontext6 tst-setcontext7 tst-setcontext8 \
> > -		   tst-setcontext9 tst-bz20544
> > +		   tst-setcontext9 tst-bz20544 tst-threaded-fork-abort \
> >  
> >  tests-internal	:= tst-strtod1i tst-strtod3 tst-strtod4 tst-strtod5i \
> >  		   tst-tls-atexit tst-tls-atexit-nodelete
> > @@ -243,3 +243,5 @@ $(objpfx)tst-setcontext3.out: tst-setcontext3.sh $(objpfx)tst-setcontext3
> >  	$(evaluate-test)
> >  
> >  $(objpfx)tst-makecontext: $(libdl)
> > +
> > +LDLIBS-tst-threaded-fork-abort = $(shared-thread-library)
> 
> This should be a dependency, so that the test gets relinked if the
> thread library chagnes.
> 
> > --- /dev/null
> > +++ b/stdlib/tst-threaded-fork-abort.c
> 
> > +   On a x86_64 VM (4 vCPU @ 3.5GHz) the test fails once every 100-600
> > +   seconds when run continuosly in a loop.  We want this test to fail
> > +   with a probability of say 10% so develpers see it at least once in
> > +   a one or two week development period, or at least once during the
> > +   release window. To achieve that we run for at least 20 seconds,
> > +   and set the timeout at 30 seconds (gives time for a last iteration
> > +   to complete).  */
> 
> Please make it an xtest.  I don't think the test is valuable enough to
> run continuously during development, given its overhead.
> 
> > diff --git a/support/timespec.h b/support/timespec.h
> > index 1a6775a882..280844f2c0 100644
> > --- a/support/timespec.h
> > +++ b/support/timespec.h
> > @@ -55,6 +55,11 @@ struct timespec support_timespec_normalize (struct timespec time);
> >  int support_timespec_check_in_range (struct timespec expected, struct timespec observed,
> >  				  double lower_bound, double upper_bound);
> >  
> > +/* True if left is before right, false otherwise.  */
> > +#define TIMESPEC_BEFORE(left, right) \
> > +  ((left.tv_sec < right.tv_sec)					\
> > +   || (left.tv_sec == right.tv_sec				\
> > +       && left.tv_nsec < right.tv_nsec))
> 
> Should this be an inline function?
> 
> >  /* Check that the timespec on the left represents a time before the
> >     time on the right. */
> > diff --git a/sysdeps/nptl/fork.c b/sysdeps/nptl/fork.c
> > index 5091a000e3..8fdc6589e5 100644
> > --- a/sysdeps/nptl/fork.c
> > +++ b/sysdeps/nptl/fork.c
> > @@ -66,6 +66,11 @@ __libc_fork (void)
> >      {
> >        _IO_list_lock ();
> >  
> > +      /* Acquire the abort lock.  We need to prevent other threads
> > +	 from aborting while we fork.  We must keep abort AS-safe
> > +	 and to do that we need to own the lock in the child.  */
> > +      __abort_fork_lock ();
> > +
> >        /* Acquire malloc locks.  This needs to come last because fork
> >  	 handlers may use malloc, and the libio list lock has an
> >  	 indirect malloc dependency as well (via the getdelim
> 
> Is this ordering really correct?  I think this will result in
> deadlocks if another thread detects heap corruption and calls abort:
> the forking thead acquires the abort lock and then the malloc locks,
> while the other thread acquires one malloc lock and then the abort
> lock.

Is there a reason to take the lock across fork rather than just
resetting it in the child? After seeing this I'm working on fixing the
same issue in musl and was about to take the lock, but realized ours
isn't actually protecting any userspace data state, just excluding
sigaction on SIGABRT during abort.

Rich