[PATCH v3 3/9] Cygwin: signal: Remove queue entry from the queue chain when cleared

[Takashi Yano]

On Mon, 2 Dec 2024 16:14:59 +0100
Corinna Vinschen wrote:
> On Nov 29 20:48, Takashi Yano wrote:
> > The queue is cleaned up by removing the entries having si_signo == 0
> > while processing the queued signals, however, sipacket::process() may
> > set si_signo in the queue to 0 of the entry already processed but not
> > succeed by calling sig_clear(). This patch ensures the sig_clear()
> > to remove the entry from the queue chain.
> 
> Thanks for this patch.  I just have two questions.
> 
> > Addresses: https://cygwin.com/pipermail/cygwin/2024-November/256744.html
> > Fixes: 9d2155089e87 ("(wait_sig): Define variable q to be the start of the signal queue.  Just iterate through sigq queue, deleting processed or zeroed signals")
> > Reported-by: Christian Franke <Christian.Franke@t-online.de>
> > Reviewed-by: Corinna Vinschen <corinna@vinschen.de>
> > Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp>
> > ---
> >  winsup/cygwin/local_includes/sigproc.h |  3 +-
> >  winsup/cygwin/sigproc.cc               | 48 +++++++++++++++++---------
> >  2 files changed, 34 insertions(+), 17 deletions(-)
> > 
> > diff --git a/winsup/cygwin/local_includes/sigproc.h b/winsup/cygwin/local_includes/sigproc.h
> > index 46e26db19..8b7062aae 100644
> > --- a/winsup/cygwin/local_includes/sigproc.h
> > +++ b/winsup/cygwin/local_includes/sigproc.h
> > @@ -50,8 +50,9 @@ struct sigpacket
> >    {
> >      HANDLE wakeup;
> >      HANDLE thread_handle;
> > -    struct sigpacket *next;
> >    };
> > +  struct sigpacket *next;
> > +  struct sigpacket *prev;
> 
> The former method using q and qnext ptr didn't need prev.  The question
> is, why did you add prev?  If you think this has an advantage, even if
> just better readability, it would be nice to document this in the commit
> message.

Consider the queued signal chain is like:
A->B->C->D
Assume that now 'q' and 'qnext' ptr points to C and process() is
processing C. If B is cleared in process(), A->next should be set
to C. Then, if process() for C succeeds, C should be removed from
the queue, so A->next should be set to D. However, now we cannot
access to A because we do not have the pointer to A.

To resolve this problem, I introduced prev member.

v2 patch didn't need prev because it rescans again after all
process() finished.

> >    int process ();
> >    int setup_handler (void *, struct sigaction&, _cygtls *);
> >  };
> > diff --git a/winsup/cygwin/sigproc.cc b/winsup/cygwin/sigproc.cc
> > index 4d50a5865..8ffb90a2c 100644
> > --- a/winsup/cygwin/sigproc.cc
> > +++ b/winsup/cygwin/sigproc.cc
> > @@ -111,7 +111,7 @@ class pending_signals
> >  public:
> >    void add (sigpacket&);
> >    bool pending () {retry = !!start.next; return retry;}
> > -  void clear (int sig) {sigs[sig].si.si_signo = 0;}
> > +  void clear (int sig);
> >    void clear (_cygtls *tls);
> >    friend void sig_dispatch_pending (bool);
> >    friend void wait_sig (VOID *arg);
> > @@ -432,21 +432,35 @@ sig_clear (int sig)
> >    sigq.clear (sig);
> >  }
> >  
> > +/* Clear pending signals of specific si_signo.
> > +   Called from sigpacket::process(). */
> > +void
> > +pending_signals::clear (int sig)
> > +{
> > +  sigpacket *q = sigs + sig;
> > +  if (!sig || !q->si.si_signo)
> > +    return;
> > +  q->si.si_signo = 0;
> > +  q->prev->next = q->next;
> > +  if (q->next)
> > +    q->next->prev = q->prev;
> > +}
> 
> This is called from sigpacket::process() as well as from wait_sig(),
> _cygtls::handle_SIGCONT() and sigaction_worker().
> 
> The below clear method is called from _cygtls::remove_pending_sigs().
> 
> The calls from sigpacket::process() and _cygtls::handle_SIGCONT() are
> under protection of the threadlist_t mutex (which actually isn't meant
> to protect the sig queue), but the calls from wait_sig() and
> sigaction_worker() are not.  wait_sig() also modifies the queue by itself.
> 
> Given that the signal queue is working on predefined memory, there's
> fortunately not much chance of memory corruption, but without locking
> or, better, lockfree add/clear using Interlocked functions, aren't
> we potentially losing signals?

Hmmm, maybe. Let me consider a bit.

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