On Sat, 16 Aug 2025 00:45:22 -0700 (PDT) Jeremy Drake wrote: > 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 <[email protected]> > > > Reviewed-by: Jeremy Drake <[email protected]> > > > Signed-off-by: Takashi Yano <[email protected]> > > > --- > > > 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).
Ok. Thanks. So what about v3 patch? -- Takashi Yano <[email protected]>
