[PATCH v2] Cygwin: sigtimedwait: Fix segfault when timeout is used

[Takashi Yano]

On Tue, 19 Nov 2024 11:51:17 +0100
Corinna Vinschen wrote:
> On Nov 19 17:40, Takashi Yano wrote:
> > diff --git a/winsup/cygwin/signal.cc b/winsup/cygwin/signal.cc
> > index 77152910b..eca536e90 100644
> > --- a/winsup/cygwin/signal.cc
> > +++ b/winsup/cygwin/signal.cc
> > @@ -618,6 +618,20 @@ sigwait_common (const sigset_t *set, siginfo_t *info, PLARGE_INTEGER waittime)
> >        switch (cygwait (NULL, waittime,
> >  		       cw_sig_eintr | cw_cancel | cw_cancel_self))
> >  	{
> > +	case WAIT_TIMEOUT:
> > +	  _my_tls.lock ();
> > +	  if (_my_tls.sigwait_mask != 0)
> > +	    {
> > +	      /* Normal timeout */
> > +	      _my_tls.sigwait_mask = 0;
> > +	      _my_tls.unlock ();
> > +	      set_errno (EAGAIN);
> > +	      break;
> > +	    }
> > +	  /* sigpacket::process() already started.
> > +	     Go through to WAIT_SIGNALED case. */
> > +	  _my_tls.unlock ();
> > +	  fallthrough;
> >  	case WAIT_SIGNALED:
> >  	  if (!sigismember (set, _my_tls.infodata.si_signo))
> 
> I don't think this is safe.  infodata is only set in
> _cygtls::interrupt_setup(), called from sigpacket::setup_handler(),
> called from sigpacket::process() *after* _my_tls.sigwait_mask has
> been set to 0 and outside the tls lock.

Yeah, indeed. I didn't have enough thought...

> Unfortunately sigwait_mask only signals that processing the signal has
> started, but we have to make sure that signal processing for this signal
> has finished, so infodata is set up correctly.
> 
> Maybe we can utilize WaitOnAddress, kind of like this?
> 
> sigwait_common, just the fallthrough snippet:
> 
>   +       /* sigpacket::process() already started.
>   +          Go through to WAIT_SIGNALED case. */
>   +       _my_tls.unlock ();
>   +       sigset_t compare = 0;
>   +       WaitOnAddress (&_my_tls.sigwait_mask, &compare,
>   +                      sizeof (sigset_t), INFINITE);
>   +       _my_tls.sigwait_mask = 0;
>   +       fallthrough;
> 
> sigpacket::process():
> 
> @@ -1457,6 +1457,7 @@ sigpacket::process ()
>    bool issig_wait = false;
>    struct sigaction& thissig = global_sigs[si.si_signo];
>    void *handler = have_execed ? NULL : (void *) thissig.sa_handler;
> +  sigset_t orig_wait_mask = 0;
>  
>    threadlist_t *tl_entry = NULL;
>    _cygtls *tls = NULL;
> @@ -1527,11 +1528,15 @@ sigpacket::process ()
>    if ((HANDLE) *tls)
>      tls->signal_debugger (si);
>  
> -  if (issig_wait)
> +  tls->lock ();
> +  if (issig_wait && tls->sigwait_mask != 0)
>      {
> +      orig_wait_mask = tls->sigwait_mask;
>        tls->sigwait_mask = 0;
> +      tls->unlock ();
>        goto dosig;
>      }
> +  tls->unlock ();
>  
>    if (handler == SIG_IGN)
>      {
> @@ -1606,6 +1611,11 @@ dosig:
>    /* Dispatch to the appropriate function. */
>    sigproc_printf ("signal %d, signal handler %p", si.si_signo, handler);
>    rc = setup_handler (handler, thissig, tls);
> +  if (orig_wait_mask)
> +    {
> +      tls->sigwait_mask = orig_wait_mask;
> +      WakeByAddressAll (&tls->sigwait_mask);
> +    }
>  
>  done:
>    cygheap->unlock_tls (tl_entry);
> 
> Mind, that's just an idea.  There may be a simpler way to do this.
> 
> Alternatively we can just fallback to your version 1.

Using WaitOnAddress() may be nice idea, however, I prefer my v1 patch.
It's simpler and the intent of the code is clearer, isn't it?

-- 
Takashi Yano <takashi.yano@nifty.ne.jp>