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.y...@nifty.ne.jp>
