[perl #126474] malloc deadlock in unsafe signal handler

On Wed, 10 Jul 2019 03:38:53 -0700, demerphq wrote:
> On Wed, 10 Jul 2019 at 03:48, Tony Cook via RT
> <perlbug-followup@perl.org> wrote:
> >
> > On Sun, 08 Nov 2015 19:43:06 -0800, tonyc wrote:
> > > On Thu Oct 29 19:27:04 2015, kazuhooku@gmail.com wrote:
> > > > For example, you can call POSIX::write in the unsafe signal
> > > > handler
> > > > to
> > > > wake up `select` reliably.  The code will look like:
> > > >
> > > > pipe(my $rfh, $wfh);
> > > > my $wfd = fileno $wfh;
> > > > POSIX::sigaction(SIGTERM, POSIX::SigAction->new(sub {
> > > >     POSIX::write $wfd, "1", 1;
> > >
> > > POSIX::write() calls sv_newmortal() for its return value, so
> > > there's a
> > > small change of a call to malloc() there.
> >
> > Both for allocating the SV itself and for expanding the tmps stack.
> >
> > >
> > > > }));
> > > >
> > > > my $rbits = '';
> > > > vec($rbits, fileno $rfh, 1) = 1;
> > > > ...
> > > > select($rbits, ...);
> > > >
> > > > This is essentially same as using signaled provided by Linux, but
> > > > is
> > > > more portable.
> > > >
> > > > >> However (as I believe) in Perl 5.17, the implementation of the
> > > > >> signal
> > > > >> handler in the perl core has been altered to _always_ call
> > > > >> malloc
> > > > >> (via
> > > > >> newSVsv).
> > > > >>
> > > > >> In other words, starting from 5.17, it has become _impossible_
> > > > >> to
> > > > >> write an unsafe signal handler without fear of deadlock.  And
> > > > >> that
> > > > >> is
> > > > >> what I see as a regression.
> > > > >>
> > > > >
> > > > > I think you're referring to 100c03aa. Possibly an alternative
> > > > > $@ is
> > > > > the
> > > > > solution here too.
> > > >
> > > > Agreed.
> > >
> > > Possibly $@ should be set to a sv_newmortal() instead of a copy of
> > > its
> > > current value.
> >
> > That doesn't help, the CLEAR_ERRSV() in Perl_call_sv() will still
> > allocate a new PV (via SvPVCLEAR() which is a wrapper around
> > Perl_sv_setpv_bufsize()).
> >
> > Several other macros can end up allocating memory too, depending on
> > whether the stack involved has enough space or not.
> >
> > We can't use G_KEEPERR to skip the CLEAR_ERRSV() since the signal
> > handler uses the value of ERRSV to check if the sub died.
> >
> > The only way I can see to make it work would be to pre-create a SV
> > that we keep in an interpreter global and substitute that into
> > GvSV(PL_errgv) when we call_sv() the signal handler.
> 
> Surely this is a reasonable thing to do? Do we need one per thread for
> threaded builds?

Well, it would fix one point on non-safety, for it to actually be safe we'd also need to switch in at least:

- a pool of SVs to allocate from
- preallocated temp, save and argument stacks

since all of these are used either by the signal -> perl sub delivery code, or by the C<< POSIX::write $wfd, "1", 1; >> in the suggested signal handler:

- POSIX::write() allocates a SV and makes it mortal for its return value (temp stack)
- the call to the signal handler sub and to POSIX::write both use the argument stack
- the signal handling code uses the save stack
- the signal handling code pushes a new argument stack (possibly using an existing one, but creating one if needed). This code may race if the non-signal handler code is also pushing a stack

Tony

---
via perlbug:  queue: perl5 status: open
https://rt.perl.org/Ticket/Display.html?id=126474

• [perl #126474] malloc deadlock in unsafe signal handler by Tony Cook via RT