On Fri, 15 Aug 2025, Jeremy Drake via Cygwin-patches wrote:
> On Sat, 16 Aug 2025, Takashi Yano wrote:
>
> > The class child_info_spawn has two constructors: one without arguments
> > and one with two arguments. The former does not initialize any members.
> > Commit 1f836c5f7394 used the latter to ensure that the local ch_spawn
> > (i.e., ch_spawn_local) would be properly initialized. However, this was
> > insufficient - it initialized only the base child_info members, not the
> > fields specific to child_info_spawn. This led to the issue reported in
> > https://cygwin.com/pipermail/cygwin/2025-August/258660.html.
> >
> > This patch updates the former constructor to properly initialize member
> > variable 'ev' which was referred without initialization, and switches
> > ch_spawn_local to use it.
> >
> > Addresses: https://cygwin.com/pipermail/cygwin/2025-August/258660.html
> > Fixes: 1f836c5f7394 ("Cygwin: spawn: Make system() thread-safe")
> > Reported-by: Denis Excoffier <cyg...@denis-excoffier.org>
> > Reviewed-by: Jeremy Drake <cyg...@jdrake.com>
> > Signed-off-by: Takashi Yano <takashi.y...@nifty.ne.jp>
> > ---
> > winsup/cygwin/local_includes/child_info.h | 5 +++--
> > winsup/cygwin/spawn.cc | 2 +-
> > winsup/cygwin/syscalls.cc | 2 +-
> > 3 files changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/winsup/cygwin/local_includes/child_info.h
> > b/winsup/cygwin/local_includes/child_info.h
> > index 2da62ffaa..b8707b9ec 100644
> > --- a/winsup/cygwin/local_includes/child_info.h
> > +++ b/winsup/cygwin/local_includes/child_info.h
> > @@ -33,7 +33,7 @@ enum child_status
> > #define EXEC_MAGIC_SIZE sizeof(child_info)
> >
> > /* Change this value if you get a message indicating that it is
> > out-of-sync. */
> > -#define CURR_CHILD_INFO_MAGIC 0xacbf4682U
> > +#define CURR_CHILD_INFO_MAGIC 0x39f766b5U
> >
> > #include "pinfo.h"
> > struct cchildren
> > @@ -148,7 +148,8 @@ public:
> > char filler[4];
> >
> > void cleanup ();
> > - child_info_spawn () {};
> > + child_info_spawn () :
> > + child_info (sizeof *this, _CH_NADA, false), ev (NULL) {};
>
> I noticed that moreinfo is checked/used in cleanup too, but it is set in
> worker. It'd probably be safer to initialize it too though. Looking at
> child_info, subproc_ready seems to not be initialized if
> need_subproc_ready is false. It'd probably be subject to the same issue
> as ev.
More thoughts as I'm trying to sleep. "Complicating" the default
constructor may now require actually running code to construct the global
ch_spawn instance. Perhaps a new constructor for this purpose?
I'd put the constructor with initializers in a .cpp file rather than
inlining it in the header, due to the CHILD_INFO_MAGIC hashing going on
with the header. Then I think it'd be cleaner initializing more members
too (all the pointers/handles would make sense).
>
> > child_info_spawn (child_info_types, bool);
> > void record_children ();
> > void reattach_children ();
> > diff --git a/winsup/cygwin/spawn.cc b/winsup/cygwin/spawn.cc
> > index 680f0fefd..6cd97ec17 100644
> > --- a/winsup/cygwin/spawn.cc
> > +++ b/winsup/cygwin/spawn.cc
> > @@ -950,7 +950,7 @@ spawnve (int mode, const char *path, const char *const
> > *argv,
> > if (!envp)
> > envp = empty_env;
> >
> > - child_info_spawn ch_spawn_local (_CH_NADA, false);
> > + child_info_spawn ch_spawn_local;
> > switch (_P_MODE (mode))
> > {
> > case _P_OVERLAY:
> > diff --git a/winsup/cygwin/syscalls.cc b/winsup/cygwin/syscalls.cc
> > index 863f8f23c..83a54ca05 100644
> > --- a/winsup/cygwin/syscalls.cc
> > +++ b/winsup/cygwin/syscalls.cc
> > @@ -4535,7 +4535,7 @@ popen (const char *command, const char *in_type)
> > fcntl (stdchild, F_SETFD, stdchild_state | FD_CLOEXEC);
> >
> > /* Start a shell process to run the given command without forking. */
> > - child_info_spawn ch_spawn_local (_CH_NADA, false);
> > + child_info_spawn ch_spawn_local;
> > pid_t pid = ch_spawn_local.worker ("/bin/sh", argv, environ,
> > _P_NOWAIT,
> > __std[0], __std[1]);
> >
> >
--
I've given up reading books; I find it takes my mind off myself.
