On Mon, 3 Mar 2025 14:01:08 +0100
Corinna Vinschen wrote:
> 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);
Thanks!
> 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.
> Either way, I would add a volatile to _pinfo::process_state.
Thanks. I will.
--
Takashi Yano <[email protected]>
