On Tue, 3 Dec 2024 21:17:47 +0900 Takashi Yano wrote: > 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.fra...@t-online.de> > > > Reviewed-by: Corinna Vinschen <cori...@vinschen.de> > > > Signed-off-by: Takashi Yano <takashi.y...@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.
I agree we need lock for signal queue handling. Hoever, IIUC, threadlist_t mutex lock woks for only the target thread. So, it cannot be used as a lock for the signal queue that has entries for different threads. So, I introduced a lock for this purpose. Could you please have a look? -- Takashi Yano <takashi.y...@nifty.ne.jp>
