Hi Jeremy, Thanks for reviewing.
On Fri, 15 Aug 2025 14:46:31 -0700 (PDT) Jeremy Drake 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 all > > member variables 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: > > Signed-off-by: Takashi Yano <takashi.y...@nifty.ne.jp> > > --- > > winsup/cygwin/local_includes/child_info.h | 4 +++- > > winsup/cygwin/spawn.cc | 2 +- > > winsup/cygwin/syscalls.cc | 2 +- > > 3 files changed, 5 insertions(+), 3 deletions(-) > > > > diff --git a/winsup/cygwin/local_includes/child_info.h > > b/winsup/cygwin/local_includes/child_info.h > > index 2da62ffaa..e359d3645 100644 > > --- a/winsup/cygwin/local_includes/child_info.h > > +++ b/winsup/cygwin/local_includes/child_info.h > > @@ -148,7 +148,9 @@ public: > > char filler[4]; > > > > void cleanup (); > > - child_info_spawn () {}; > > + child_info_spawn () : child_info (sizeof *this, _CH_NADA, false), > > + hExeced (NULL), ev (NULL), sem (NULL), cygpid (0), > > + moreinfo (NULL), __stdin (0), __stdout (0) {}; > > child_info_spawn (child_info_types, bool); > > void record_children (); > > void reattach_children (); > > Making a change to the child_info* classes will require updating the > "magic" define in that header. Done. > Do we really need to initialize all of the members? It seems like most of > them would be written during child_info_spawn::worker. I expect sem to be > an exception. I guess it doesn't hurt. Yeah, indeed. 'sem' is used only for _P_OVERLAY, so spawnve() and popen() are not be affected. Only 'ev' is referred in child_info_spawn::cleanup() without initialization. So, we need to initialize only ev, I think. I'll submit v2 patch. -- Takashi Yano <takashi.y...@nifty.ne.jp>
