Re: [PATCH v3 2/3] Cygwin: signal: Fix a race issue on modifying _pinfo::process_state

[Corinna Vinschen]

On Mar  3 21:24, Takashi Yano wrote:
> On Mon, 3 Mar 2025 10:51:30 +0100
> Corinna Vinschen wrote:
> > On Mar  1 08:33, Takashi Yano wrote:
> > > The PID_STOPPED flag in _ponfo::process_state is sometimes accidentally
> > > cleared due to a race condition when modifying it with the "|=" or "&="
> > > operators. This patch uses InterlockedOr/And() instead to avoid the
> > > race condition.
> > 
> > Is this really sufficent?  I'm asking because of...
> > 
> > > @@ -678,8 +678,9 @@ dofork (void **proc, bool *with_forkables)
> > >  
> > >    if (ischild)
> > >      {
> > > -      myself->process_state |= PID_ACTIVE;
> > > -      myself->process_state &= ~(PID_INITIALIZING | PID_EXITED | 
> > > PID_REAPED);
> > > +      InterlockedOr ((LONG *) &myself->process_state, PID_ACTIVE);
> > > +      InterlockedAnd ((LONG *) &myself->process_state,
> > > +               ~(PID_INITIALIZING | PID_EXITED | PID_REAPED));
> > >      }
> > >    else if (res < 0)
> > >      {
> > 
> > ...places like these.  Every single Interlocked call is safe in itself,
> > but what if somebody else changes something between the two interlocked
> > calls?  Maybe this should be done with InterlockedCompareExchange.
> 
> Thanks for reviewing.
> 
> How can we guard that situation by using InterlockedCompareExchange()?
> Could you please give me some more instruction?

The InterlockedCompareExchange can be used to check for a parallel
change, kind of like this:

  DWORD old_state, new_state;
  do
    {
      old_state = myself->process_state;
      new_state = old_state | PID_ACTIVE;
      new_state &= ~(PID_INITIALIZING | PID_EXITED | PID_REAPED);
    }
  while (InterlockedCompareExchange (&myself->process_state,
                                     new_state,
                                     old_state) != old_state);

but now that I'm writing it I'm even more unsure this is necessary.
The only two places doing an And and an Or are doing it with the
exact same flags.  And the combination of PID_ACTIVE and the other
three flags is never tested together.

What do you think?

Either way, I would add a volatile to _pinfo::process_state.


Thanks,
Corinna