Re: Fixing posix_spawn_file_actions_addopen with O_CLOEXEC

On Tue, 8 Jul 2025, Corinna Vinschen wrote:

> On Jul  8 10:38, Jeremy Drake wrote:
> > (continuation from cygwin-patches thread)
> >
> > On Tue, 8 Jul 2025, Corinna Vinschen wrote:
> >
> > > On Jul  7 14:43, Jeremy Drake via Cygwin-patches wrote:
> > > > I noticed a bit that you quoted that I hadn't noticed before
> > > > additionally, when the new process image is executed, any
> > > >   file descriptor (from this new set) which has its FD_CLOEXEC flag set
> > > >   shall be closed (see posix_spawn()).
> > > >
> > > > The "from this new set" is not handled properly in my implementation, and
> > > > I'm pretty sure not from the existing newlib implementation: I copied what
> > > > it was doing, which was to clear the FD_SETFD flag after open.  That
> > > > would be wrong, if faced with an addopen with the O_CLOEXEC flag set.
> > > > This is obviously a stupid thing for a caller to do...
> > >
> > > the POSIX posix_spawn man page has a 4 steps list how to process
> > > the file actions with step 4: close all FD_CLOEXEC descriptors.
> > >
> > > I wonder if that doesn't make some sense, e.g.
> > >
> > >   addopen (&fact, 42, "somedir", O_CLOEXEC);
> > >   addfchdir (42);
> > >
> > > but then again, you could just add an addclose(42) and you would have the
> > > same effect.
> > >
> > > Well, *shrug*, as long as we can do it right.
> > >
> > > But yeah, I don't see anywhere in the POSIX docs that the addopen
> > > descriptors have the FD_CLOEXEC flag removed automatically.  They
> > > are supposed to be closed instead, FWIW.
> > >
> > > A fix to newlib's posix_spawn might make sense.
> >
> > OK.  I'm not sure about what functions are allowed to  be used there,
> > though (fcntl is in a preprocessor check, so I guess not all posix funcs
> > can be assumed to be present).  Can I use dup3 or do I need to fcntl
> > F_SETFD after the dup2?  Should I use the O_CLOEXEC flag to determine this
> > or use F_GETFD on the open-ed fd to F_SETFD on the dup2-ed one?
>
> I'm not sure what you're trying to accomplish.  Given that we execve()
> from here, step 4 from the POSIX.1-2024 man page is basically implied.
>
> So afaics, the only necessary patch is dropping the _fcntl call from
> the FAE_OPEN case, no?

No, because in order to get the result of the open into the specified file
descriptor, chances are that the subsequent dup2 call will also be
required.  dup2 is defined to clear the CLOEXEC flag on the new
file descriptor (as long as it is not equal to the old one), dup3 allows
setting the flag during the dup.

https://pubs.opengroup.org/onlinepubs/9799919799/functions/dup.html

> > Also, I noticed a new spawnattr flag for calling setsid () in the child, I
> > could do a patch to add that, but is that function allowed to be used or
> > does it need a preprocessor check?
>
> I don't think a check is necessary.  Just add setsid to the list
> of "Supporting OS subroutines required" in the file header.

OK