On Wed, 2 Jul 2025, Corinna Vinschen wrote: > On Jul 1 16:43, Jeremy Drake via Cygwin-patches wrote: > > This will be used by posix_spawn_fileaction_add_(f)chdir. > > > > The int cwdfd is placed such that it fits into space previously unused > > due to alignment in the cygheap_exec_info class. > > > > This uses a file descriptor rather than a path both because it is easier > > to marshal to the child and because this should protect against races > > where the directory might be renamed or removed between addfchdir and > > the actual setting of the cwd in the child. > > > > Signed-off-by: Jeremy Drake <[email protected]> > > --- > > winsup/cygwin/dcrt0.cc | 19 +++- > > winsup/cygwin/local_includes/child_info.h | 4 +- > > winsup/cygwin/local_includes/path.h | 6 +- > > winsup/cygwin/local_includes/winf.h | 2 +- > > winsup/cygwin/spawn.cc | 100 ++++++++++++++++++---- > > winsup/cygwin/syscalls.cc | 4 +- > > 6 files changed, 113 insertions(+), 22 deletions(-) > > > > diff --git a/winsup/cygwin/dcrt0.cc b/winsup/cygwin/dcrt0.cc > > index b0fb5c9c1e..6adc31495a 100644 > > --- a/winsup/cygwin/dcrt0.cc > > +++ b/winsup/cygwin/dcrt0.cc > > @@ -46,6 +46,7 @@ extern "C" void __sinit (_reent *); > > > > static int NO_COPY envc; > > static char NO_COPY **envp; > > +static int NO_COPY cwdfd = AT_FDCWD; > > > > bool NO_COPY jit_debug; > > > > @@ -656,6 +657,7 @@ child_info_spawn::handle_spawn () > > __argv = moreinfo->argv; > > envp = moreinfo->envp; > > envc = moreinfo->envc; > > + cwdfd = moreinfo->cwdfd; > > if (!dynamically_loaded) > > cygheap->fdtab.fixup_after_exec (); > > if (__stdin >= 0) > > @@ -842,7 +844,22 @@ dll_crt0_1 (void *) > > > > ProtectHandle (hMainThread); > > > > - cygheap->cwd.init (); > > + if (cwdfd >= 0) > > + { > > + int res = fchdir (cwdfd); > > + if (res < 0) > > + { > > + /* if the error occurs after the calling process successfully > > + returns, the child process shall exit with exit status 127. */ > > + /* why is this byteswapped? */ > > + set_api_fatal_return (0x7f00); > > + api_fatal ("can't fchdir, %R", res); > > + } > > + close (cwdfd); > > + cwdfd = AT_FDCWD; > > + } > > + else > > + cygheap->cwd.init (); > > Weeeeell, as discussed in the other thread, and on second thought, maybe > this is the right spot to handle all the posix_spawn stuff. > > But then, it should be in it's own function. And you don't need > moreinfo->cwdfd, because the entire set of actions requested by the > posix_spawn caller should run one at a time in that function, so > multiple chdir and fchdir actions may be required. > > I would also suggest to pimp cwdstuff::init() by adding an argument > which allows to say
... ? > Eventually, this code snippet in dll_crt0_1 should probably look like > this: > > cygheap->cwd.init (); > if (posix_spawn_actions_present) > posix_spawn_run_child_actions (...); > > Regardless if posix_spawn chdir/fchdir file actions are present or not, > in the first place the cwd of the child is the parent's cwd. The > posix_spawn chdir/fchdir file actions run afterwards. In https://cygwin.com/pipermail/cygwin-developers/2025-March/012733.html, you said > For posix_spawn without forking, this complicates matters. For > instance, we don't want having to close FD_CLOEXEC handles in the > spawned child because that's a security problem. FD_CLOEXEC sets handles as non-inherited at the Windows level, but for posix_spawn_file_actions_addclose is that still a security problem? Also, it is allowed to posix_spawn_file_actions_adddup2 from a FD_CLOEXEC file descriptor, so the parent would have to go through all the file actions, work the (f)chdirs to know where to look for relative prog_arg, and check adddup2s for FD_CLOEXEC descriptors, set them to not-FD_CLOEXEC and record that they were for the child to know to close them (and put them back to FL_CLOEXEC after the spawn). This is already a good part of the work being done in the parent in my patch.
