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

[Jon Turney]

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!