[PATCH] Cygwin: signal: Introduce a lock for the signal queue

[Takashi Yano]

On Wed, 4 Dec 2024 13:53:14 +0100
Corinna Vinschen wrote:
> On Dec  4 20:41, Takashi Yano wrote:
> > Currently, the signal queue is touched by the thread sig as well as
> > other threads that call sigaction_worker(). This potentially has
> > a possibility to destroy the signal queue chain. A possible worst
> > result may be a self-loop chain which causes infinite loop. With
> > this patch, lock()/unlock() are introduce to avoid such a situation.
> 
> I was hoping for a lockfree solution, but never mind that for now.

I cannot imagine how the lockfree can be realized.
Consider the queue chain:
A->B->C->D
A<-B<-C<-D
where "->" shows next link and "<-" shows prev link.
Assume a thread tries to clear B, and another thread tries to clear C.
Two processes, i.e.
clear(B)
B->prev->next = B->next
B->next->prev = B->prev
and
clear(C)
C->prev->next = C->next
C->next->prev = C->prev
are running at the same time. Even if each line is executed atomically,
the next case is not avoidable.

clear(B)			clear(C)
B->prev->next = B->next						A->next = C
				C->prev->next = C->next		B->next = D
B->next->prev = B->prev						D->prev = A
				C->next->prev = C->prev		D->prev = B

Then, the result is broken as:
A->C->D
A<-B<-D
   B->D
   B<-C
while expected result is:
A->D
A<-D

Is there any way to execute atomically two lines:
B->prev->next = B->next
B->next->prev = B->prev
using Interlocked* functions?

> > diff --git a/winsup/cygwin/sigproc.cc b/winsup/cygwin/sigproc.cc
> > index 7e02e61f7..cc3113b88 100644
> > --- a/winsup/cygwin/sigproc.cc
> > +++ b/winsup/cygwin/sigproc.cc
> > @@ -106,12 +106,27 @@ class pending_signals
> >  {
> >    sigpacket sigs[_NSIG + 1];
> >    sigpacket start;
> > +  volatile unsigned locked;
> >    bool retry;
> > +  void lock ()
> > +  {
> > +    while (InterlockedExchange (&locked, 1))
> > +      {
> > +#ifdef __x86_64__
> > +	__asm__ ("pause");
> > +#else
> > +#error unimplemented for this target
> > +#endif
> > +	yield ();
> > +      }
> > +    }
> > +  void unlock () { locked = 0; }
> >  
> >  public:
> > +  pending_signals (): locked(0) {}
> >    void add (sigpacket&);
> >    bool pending () {retry = !!start.next; return retry;}
> > -  void clear (int sig);
> > +  void clear (int sig, bool need_lock);
> >    void clear (_cygtls *tls);
> >    friend void sig_dispatch_pending (bool);
> >    friend void wait_sig (VOID *arg);
> 
> Given this is used in C-code only, what about just using SRWLocks
> in Exclusive-only mode, rather than a spinlock?

I submitted v2 patch. Please check.

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