On Tue, 22 Jul 2025 10:40:50 +0200
Corinna Vinschen wrote:
> Hi Takashi,
>
> On Jul 22 09:31, Takashi Yano wrote:
> > ...completion in child process because the cygheap should not be
> > changed to avoid mismatch between child_info::cygheap_max and
> > ::cygheap_max. Otherwise, child_copy() might copy cygheap being
> > modified by other process. However, do not lock cygheap if the
> > child process is non-cygwin process, because child_copy() will
> > not be called in it. Not only it is unnecessary, it can also fall
> > into deadlock in close_all_files() while cygheap is already locked.
> >
> > Fixes: 977ad5434cc0 ("* spawn.cc (spawn_guts): Call refresh_cygheap before
> > creating a new process to ensure that cygheap_max is up-to-date.")
> > Reviewed-by: Corinna Vinschen <cori...@vinschen.de>
> > Signed-off-by: Takashi Yano <takashi.y...@nifty.ne.jp>
> > ---
>
> When you create a new patch version, it would be nice if you could
> add version info after the three dashes, kind of like
>
> v3: add cygheap_init::lock method
> v4: inline cygheap_init::lock method
> v5: don't lock cygheap for non-cygwin child
> v6: add spawn_cygheap_lock
>
> Otherwise it's a bit tricky to review because one has to first find
> out why a new version exists at all.
>
> > winsup/cygwin/spawn.cc | 12 +++++++++++-
> > 1 file changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/winsup/cygwin/spawn.cc b/winsup/cygwin/spawn.cc
> > index cb58b6eed..cf344d382 100644
> > --- a/winsup/cygwin/spawn.cc
> > +++ b/winsup/cygwin/spawn.cc
> > @@ -542,7 +542,6 @@ child_info_spawn::worker (const char *prog_arg, const
> > char *const *argv,
> > ::cygheap->ctty ? ::cygheap->ctty->tc_getpgid () : 0;
> > if (!iscygwin () && ctty_pgid && ctty_pgid != myself->pgid)
> > c_flags |= CREATE_NEW_PROCESS_GROUP;
> > - refresh_cygheap ();
> >
> > if (mode == _P_DETACH)
> > /* all set */;
> > @@ -611,6 +610,9 @@ child_info_spawn::worker (const char *prog_arg, const
> > char *const *argv,
> >
> > cygpid = (mode != _P_OVERLAY) ? create_cygwin_pid () : myself->pid;
> >
> > + if (iscygwin ())
> > + cygheap->lock ();
> > + refresh_cygheap ();
>
> I compared v5 and v6, and I think we should not introduce the
> spawn_cygheap_lock() method. It hides a crucial problem.
>
> Assuming no further change, I'd prefer v5, BUT with a comment preceeding
> the `if (iscygwin ())' condition, along the lines of
>
> /* Lock the cygheap here to make sure the child doesn't copy a
> cygheap while it's being modified in parallel. Don't lock if
> the child is a non-Cygwin child to avoid a deadlock in
> close_all_files(). */
>
> However, IIUC this situation only occurs if a non-Cygwin child is
> execve'd, and we're talking about the close_all_files() call in line 768
> in origin/main, which potentially occurs while the cygheap would be
> locked by your patch, right?
>
> I see two different ways out:
>
> - Either convert the SRWLOCK to a muto to allow a recursive cygheap lock.
>
> - Or move the close_all_files() call.
>
> The latter seems to me like the way to go here.
>
> The close_all_files() call was introduced by commit 2f415d5efae5a
> ("Cygwin: pty: Disable FreeConsole() on close for non cygwin process.")
>
> Why not move it out of the locked region?
Thanks for the advice. I'll apply your idea and submit v7 patch.
--
Takashi Yano <takashi.y...@nifty.ne.jp>
