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

[Corinna Vinschen]

On Mar  6 14:55, Jon Turney wrote:
> On 03/03/2025 18:53, Corinna Vinschen wrote:
> > On Mar  3 23:39, Takashi Yano wrote:
> > > On Mon, 3 Mar 2025 14:01:08 +0100
> > > Corinna Vinschen wrote:
> > > > 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?
> > > 
> > > No other code touches these flags except for:
> > > 
> > > diff --git a/winsup/cygwin/sigproc.cc b/winsup/cygwin/sigproc.cc
> > > index 1ffe00a94..8739f18f5 100644
> > > --- a/winsup/cygwin/sigproc.cc
> > > +++ b/winsup/cygwin/sigproc.cc
> > > @@ -252,7 +252,7 @@ proc_subproc (DWORD what, uintptr_t val)
> > >   	  vchild->sid = myself->sid;
> > >   	  vchild->ctty = myself->ctty;
> > >   	  vchild->cygstarted = true;
> > > -	  vchild->process_state |= PID_INITIALIZING;
> > > +	  InterlockedOr ((LONG *)&vchild->process_state, PID_INITIALIZING);
> > >   	  vchild->ppid = myself->pid;	/* always set last */
> > >   	}
> > >         break;
> > > 
> > > Moreover, using InterlockedOr()/InterlockedAnd() can ensure that
> > > the other flags than the current code is modifying will be kept
> > > during modification. So using InterlockedCompareExchange() might
> > > be over the top.
> > 
> > Okidoki!
> > 
> > > > Either way, I would add a volatile to _pinfo::process_state.
> > > 
> > > Thanks. I will.
> > 
> > Great.  Feel free to push the patch without sending another patch
> > submission to cygwin-patches.
> 
> I think Takashi-san must be about due another gold star, as he's been doing
> some sterling work recently, fixing complex and difficult to reproduce bugs!

Absolutely!  Yesterday I was even mulling over a pink plush hippo, but
Takashi already has one over his fireplace and I wasn't sure if a second
one isn't taking too much space.


Corinna