posix_spawn facility

posix_spawn facility

[Bruno Haible]

Hi,

AFAIU, Cygwin has a working posix_spawn[p] implementation since 2020
(commit 3fbfcd11fb09d5f47af3043ee47ec5c7d863d872, 2020-08-03, Cygwin 3.1.7).

Additionally, Gnulib has a posix_spawn[p] implementation since 2022,
that works on all platforms, including native Windows. Based on it,
I recommend posix_spawn[p] over fork+exec, see
https://savannah.gnu.org/news/?id=10219 . It allows to have a single
application code for spawning subprocesses.

The GNU groff maintainer asks about the performance of posix_spawn[p]
on Cygwin. And here's the problem: While Cygwin has an implementation
that avoids the slow fork(), by calling child_info_spawn::worker more
or less directly, Gnulib prefers its own implementation over the Cygwin
one, and the Gnulib implementation uses slow fork()+exec().

The reason is that we consider posix_spawn[p] unsecure if it will
readily execute plain text files without a #! marker as if they were
shell scripts, usually leading to plenty of syntax errors, but also
exhibiting undefined behaviour.

This reasoning follows what was done in GNU libc:
https://sourceware.org/bugzilla/show_bug.cgi?id=13134
https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=d96de9634a334af16c0ac711074c15ac1762b23c
https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=13adfa34aff03fd9f1c1612b537a0d736ddb6c2b

These are the two configure tests that Gnulib uses:

======================= test secure posix_spawn ==========================
Preparation:
  echo ':' > conftest.scr
  chmod a+x conftest.scr
C program:
       #include <errno.h>
       #include <spawn.h>
       #include <stddef.h>
       #include <sys/types.h>
       #include <sys/wait.h>
       int
       main ()
       {
         const char *prog_path = "./conftest.scr";
         const char *prog_argv[2] = { prog_path, NULL };
         const char *environment[2] = { "PATH=.", NULL };
         pid_t child;
         int status;
         int err = posix_spawn (&child, prog_path, NULL, NULL,
                                (char **) prog_argv, (char **) environment);
         if (err == ENOEXEC)
           return 0;
         if (err != 0)
           return 1;
         status = 0;
         while (waitpid (child, &status, 0) != child)
           ;
         if (!WIFEXITED (status))
           return 2;
         if (WEXITSTATUS (status) != 127)
           return 3;
         return 0;
       }
======================= test secure posix_spawnp =========================
Preparation:
  echo ':' > conftest.scr
  chmod a+x conftest.scr
C program:
       #include <errno.h>
       #include <spawn.h>
       #include <stddef.h>
       #include <sys/types.h>
       #include <sys/wait.h>
       int
       main ()
       {
         const char *prog_path = "./conftest.scr";
         const char *prog_argv[2] = { prog_path, NULL };
         const char *environment[2] = { "PATH=.", NULL };
         pid_t child;
         int status;
         int err = posix_spawnp (&child, prog_path, NULL, NULL,
                                 (char **) prog_argv, (char **) environment);
         if (err == ENOEXEC)
           return 0;
         if (err != 0)
           return 1;
         status = 0;
         while (waitpid (child, &status, 0) != child)
           ;
         if (!WIFEXITED (status))
           return 2;
         if (WEXITSTATUS (status) != 127)
           return 3;
         return 0;
       }
==========================================================================

In Cygwin, the "test secure posix_spawn" recipe succeeds, whereas the
"test secure posix_spawnp" fails; the latter is the obstacle that
prevents Gnulib from using Cygwin's implementation.

Would it be possible to change Cygwin's posix_spawnp implementation,
so that both tests succeed?

Disclaimer: I have done my tests with Cygwin 2.9.0; so, if things have
improved since then, the better!

Bruno

Re: posix_spawn facility

[Corinna Vinschen]

Hi Bruno,

On Apr 16 13:46, Bruno Haible via Cygwin wrote:
> Hi,
> 
> AFAIU, Cygwin has a working posix_spawn[p] implementation since 2020
> (commit 3fbfcd11fb09d5f47af3043ee47ec5c7d863d872, 2020-08-03, Cygwin 3.1.7).
                                                    ^^^^^^^^^^^^^^^^^^^^^^^^

> [...]
> Would it be possible to change Cygwin's posix_spawnp implementation,
> so that both tests succeed?

Basically, yes, but...

> Disclaimer: I have done my tests with Cygwin 2.9.0; so, if things have
                                        ^^^^^^^^^^^^
					2017-09-07

I'm a bit puzzled. You quote that only Cygwin 3.1.7 has the fixed
posix_spawn, but then you test this with a version three years older?

Sorry, but... wouldn't it at least make sense to test this with a more
recent version of Cygwin first?


Thanks,
Corinna

Re: posix_spawn facility

[Bruno Haible]

Hi Corinna,

> > Would it be possible to change Cygwin's posix_spawnp implementation,
> > so that both tests succeed?
> 
> Basically, yes, but...

Thanks!

> > Disclaimer: I have done my tests with Cygwin 2.9.0; so, if things have
>                                         ^^^^^^^^^^^^
> 					2017-09-07
> 
> I'm a bit puzzled. You quote that only Cygwin 3.1.7 has the fixed
> posix_spawn, but then you test this with a version three years older?

I have now verified that the findings with Cygwin 3.4.6 are the same as
with Cygwin 2.9.0. I had expected that, based on browsing through the
Cygwin git history; I confirm it now.

Btw, there are two more functions in the posix_spawn family meanwhile:
  * posix_spawn_file_actions_addchdir_np
    implemented by glibc [1], musl libc, macOS, FreeBSD [2], Solaris ≥ 11.3
    used by a few packages (Firefox, Chromium, Rust)
  * posix_spawn_file_actions_addfchdir_np
    implemented in glibc, musl libc
    but not used by any package so far [3].

The next POSIX will contain these functions (without the _np suffix).[4]

Bruno

[1] https://sourceware.org/bugzilla/show_bug.cgi?id=17405
[2] https://man.freebsd.org/cgi/man.cgi?query=posix_spawn_file_actions_adddup2&apropos=0&sektion=3&manpath=FreeBSD+11-current&format=html
[3] https://codesearch.debian.net/
[4] https://www.austingroupbugs.net/view.php?id=1208

Re: posix_spawn facility

[Corinna Vinschen]

Hi Bruno,

On Apr 17 20:44, Bruno Haible via Cygwin wrote:
> Hi Corinna,
> 
> > > Would it be possible to change Cygwin's posix_spawnp implementation,
> > > so that both tests succeed?
> > 
> > Basically, yes, but...
> 
> Thanks!
> 
> > > Disclaimer: I have done my tests with Cygwin 2.9.0; so, if things have
> >                                         ^^^^^^^^^^^^
> > 					2017-09-07
> > 
> > I'm a bit puzzled. You quote that only Cygwin 3.1.7 has the fixed
> > posix_spawn, but then you test this with a version three years older?
> 
> I have now verified that the findings with Cygwin 3.4.6 are the same as
> with Cygwin 2.9.0. I had expected that, based on browsing through the
> Cygwin git history; I confirm it now.

Thanks a lot!  The patch is actually simple.

But I'm not *that* happy with the change yet, for two reasons.

First, the security risk outlined in
https://sourceware.org/bugzilla/show_bug.cgi?id=13134#c0 doesn't
actually exist on Cygwin, because we don't implement setuid/setgid
executables.  You can set the mode bits, but they are not acted upon.

Second, the rational section in POSIX explains posix_spawn and
posix_spawnp, but it does *not* actually provide an example
implementation of posix_spawnp, only of posix_spawn.

From the above bugzilla entry I take it that on glibc, both
functions tried to run the shell if the executable isn't recognized
(up to commits d96de9634a33 / 13adfa34aff).

However, on Cygwin, only posix_spawnp does that,but not posix_spawn.

In fact, I read the POSIX descriptions in terms of these functions quite
thoroughly, and at no point I see it mentioned that posix_spawnp shall
*not* work like exevlp/execvp.  The crucial difference between posix_spawn
and posixc_spawnp is described in an interestingly vague way:

  posix_spawnp() interprets the second parameter more elaborately than
  posix_spawn().

If I missed the point in the POSIX docs, please tell me.

So, again, the patch is simple.  But it's kind of a pity that the change
in glibc has been made without a bigger discussion.  Right now, it looks
like the glibc change to posix_spawn was correct, but the change to
posix_spawnp was arbitrary.

Has anybody attempted to ask the Austin group to define this behaviour
in posix_spawnp more concise?  Is there a protocel from the Austin
group?  If not, wouldn't it be time to ask the Austin group?

> Btw, there are two more functions in the posix_spawn family meanwhile:
>   * posix_spawn_file_actions_addchdir_np
>     implemented by glibc [1], musl libc, macOS, FreeBSD [2], Solaris ≥ 11.3
>     used by a few packages (Firefox, Chromium, Rust)
>   * posix_spawn_file_actions_addfchdir_np
>     implemented in glibc, musl libc
>     but not used by any package so far [3].
> 
> The next POSIX will contain these functions (without the _np suffix).[4]

Thanks for the pointers.  I'm not sure I'll have the time to implement
them soon, but I put them on my list for 3.5.0.  Patches welcome!


Thanks,
Corinna

Re: posix_spawn facility

[Eric Blake]

On Tue, Apr 18, 2023 at 11:25:11AM +0200, Corinna Vinschen via Cygwin wrote:
> Hi Bruno,
> 
> On Apr 17 20:44, Bruno Haible via Cygwin wrote:
> > Hi Corinna,

Jumping in to this conversation a bit belatedly, but as someone on the
Austin Group that can try to get an answer upstream...

> But I'm not *that* happy with the change yet, for two reasons.
> 
> First, the security risk outlined in
> https://sourceware.org/bugzilla/show_bug.cgi?id=13134#c0 doesn't
> actually exist on Cygwin, because we don't implement setuid/setgid
> executables.  You can set the mode bits, but they are not acted upon.

The glibc bug points to the sample posix_spawn() implementation in
POSIX XRAT - but that example implementation is non-normative and
known buggy, so it is not safe to rely on it.

Clarifying the wording in XRAT to explicitly mention that the example
is NOT bullet-proof (and that implementations should do better) is
probably worthwhile; I'll tackle that bug report.

> 
> Second, the rational section in POSIX explains posix_spawn and
> posix_spawnp, but it does *not* actually provide an example
> implementation of posix_spawnp, only of posix_spawn.

POSIX is silent as to whether posix_spawnp() has to fall back to 'sh'
on ENOEXEC failure.  The p suffix is indeed similar to execvp() (which
DOES require a fallback to sh), but it could also just mean a
PATH-search, and not the PATH-search-and-sh-fallback of execvp().  As
we now have implementations in the wild that differ in behavior, and
use security as a reason for the divergence, it is worth getting that
clarified in POSIX.  I'll file a bug against POSIX shortly, and reply
again once it is up.

My personal preference: sh fallback on ENOEXEC is useful in execvp(),
but a bear to get right (see
https://www.austingroupbugs.net/view.php?id=1645 where POSIX has a bug
in requiring argv[0] to be the script's filename, which breaks busybox
sh and is NOT what glibc does; meanwhile, musl intentionally does NOT
do the sh fallback), so NOT doing it in posix_spawnp() would be
reasonable; but we'll have to see what the rest of the Austin Group
says.

> 
> From the above bugzilla entry I take it that on glibc, both
> functions tried to run the shell if the executable isn't recognized
> (up to commits d96de9634a33 / 13adfa34aff).
> 
> However, on Cygwin, only posix_spawnp does that,but not posix_spawn.
> 
> In fact, I read the POSIX descriptions in terms of these functions quite
> thoroughly, and at no point I see it mentioned that posix_spawnp shall
> *not* work like exevlp/execvp.  The crucial difference between posix_spawn
> and posixc_spawnp is described in an interestingly vague way:
> 
>   posix_spawnp() interprets the second parameter more elaborately than
>   posix_spawn().
> 
> If I missed the point in the POSIX docs, please tell me.

Yeah, it appears that POSIX is (accidentally) silent on whether
posix_spawnp() has to do the sh fallback on ENOEXEC; but it seems
quite reasonable that posix_spawn() being more like execle() must NOT
do a sh fallback.

> 
> So, again, the patch is simple.  But it's kind of a pity that the change
> in glibc has been made without a bigger discussion.  Right now, it looks
> like the glibc change to posix_spawn was correct, but the change to
> posix_spawnp was arbitrary.
> 
> Has anybody attempted to ask the Austin group to define this behaviour
> in posix_spawnp more concise?  Is there a protocel from the Austin
> group?  If not, wouldn't it be time to ask the Austin group?

Doing that now ;)

> 
> > Btw, there are two more functions in the posix_spawn family meanwhile:
> >   * posix_spawn_file_actions_addchdir_np
> >     implemented by glibc [1], musl libc, macOS, FreeBSD [2], Solaris ≥ 11.3
> >     used by a few packages (Firefox, Chromium, Rust)
> >   * posix_spawn_file_actions_addfchdir_np
> >     implemented in glibc, musl libc
> >     but not used by any package so far [3].
> > 
> > The next POSIX will contain these functions (without the _np suffix).[4]
> 
> Thanks for the pointers.  I'm not sure I'll have the time to implement
> them soon, but I put them on my list for 3.5.0.  Patches welcome!
> 
> 
> Thanks,
> Corinna

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: posix_spawn facility

[Corinna Vinschen]

Hi Eric,

On Apr 18 15:49, Eric Blake via Cygwin wrote:
> On Tue, Apr 18, 2023 at 11:25:11AM +0200, Corinna Vinschen via Cygwin wrote:
> Jumping in to this conversation a bit belatedly, but as someone on the
> Austin Group that can try to get an answer upstream...

Many thanks for your input, it's highly appreciated.

> > Second, the rational section in POSIX explains posix_spawn and
> > posix_spawnp, but it does *not* actually provide an example
> > implementation of posix_spawnp, only of posix_spawn.
> 
> POSIX is silent as to whether posix_spawnp() has to fall back to 'sh'
> on ENOEXEC failure.  The p suffix is indeed similar to execvp() (which
> DOES require a fallback to sh), but it could also just mean a
> PATH-search, and not the PATH-search-and-sh-fallback of execvp().  As
> we now have implementations in the wild that differ in behavior, and
> use security as a reason for the divergence, it is worth getting that
> clarified in POSIX.  I'll file a bug against POSIX shortly, and reply
> again once it is up.
> 
> My personal preference: sh fallback on ENOEXEC is useful in execvp(),
> but a bear to get right (see
> https://www.austingroupbugs.net/view.php?id=1645 where POSIX has a bug
> in requiring argv[0] to be the script's filename, which breaks busybox
> sh and is NOT what glibc does; meanwhile, musl intentionally does NOT
> do the sh fallback), so NOT doing it in posix_spawnp() would be
> reasonable; but we'll have to see what the rest of the Austin Group
> says.

My point here is mostly directed to this gnulib testcase.  It tests
posix_spawnp in terms of an undefined behaviour, and if it doesn't
behave in a certain way, it's deemed insecure.

I strongly doubt that this is the right thing to do.

That doesn't mean I'm refusing to change Cygwin to be aligned to
the behaviour of glibc or, even more important, to POSIX.

But the testcase *is* questionable.


> > Has anybody attempted to ask the Austin group to define this behaviour
> > in posix_spawnp more concise?  Is there a protocel from the Austin
> > group?  If not, wouldn't it be time to ask the Austin group?
> 
> Doing that now ;)

Thanks a lot!


Corinna

Re: posix_spawn facility

[Bruno Haible]

Eric Blake wrote:
> we now have implementations in the wild that differ in behavior, and
> use security as a reason for the divergence, it is worth getting that
> clarified in POSIX.  I'll file a bug against POSIX shortly

For the reference, the systems that return ENOEXEC for posix_spawnp
attempting to execute a script without #! marker are:
  - glibc/Linux ≥ 2.15
  - glibc/Hurd ≥ 2.33 (commit 13adfa34aff03fd9f1c1612b537a0d736ddb6c2b)
  - musl libc

Bruno

Re: posix_spawn facility

[Eric Blake]

On Wed, Apr 19, 2023 at 12:10:34AM +0200, Bruno Haible wrote:
> Eric Blake wrote:
> > we now have implementations in the wild that differ in behavior, and
> > use security as a reason for the divergence, it is worth getting that
> > clarified in POSIX.  I'll file a bug against POSIX shortly
> 
> For the reference, the systems that return ENOEXEC for posix_spawnp
> attempting to execute a script without #! marker are:
>   - glibc/Linux ≥ 2.15
>   - glibc/Hurd ≥ 2.33 (commit 13adfa34aff03fd9f1c1612b537a0d736ddb6c2b)
>   - musl libc

POSIX issue now filed as
https://www.austingroupbugs.net/view.php?id=1674; although we'll have
to see if my wording is acceptable or if it settles on something a bit
looser (such as implementation-defined as to whether an sh fallback is
attempted, rather than outright forbidden).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: posix_spawn facility

[Corinna Vinschen]

On Apr 18 21:39, Eric Blake via Cygwin wrote:
> On Wed, Apr 19, 2023 at 12:10:34AM +0200, Bruno Haible wrote:
> > Eric Blake wrote:
> > > we now have implementations in the wild that differ in behavior, and
> > > use security as a reason for the divergence, it is worth getting that
> > > clarified in POSIX.  I'll file a bug against POSIX shortly
> > 
> > For the reference, the systems that return ENOEXEC for posix_spawnp
> > attempting to execute a script without #! marker are:
> >   - glibc/Linux ≥ 2.15
> >   - glibc/Hurd ≥ 2.33 (commit 13adfa34aff03fd9f1c1612b537a0d736ddb6c2b)
> >   - musl libc
> 
> POSIX issue now filed as
> https://www.austingroupbugs.net/view.php?id=1674; although we'll have
> to see if my wording is acceptable or if it settles on something a bit
> looser (such as implementation-defined as to whether an sh fallback is
> attempted, rather than outright forbidden).

Thank you!


Corinna

Re: posix_spawn facility

[Corinna Vinschen]

Hi Bruno,

On Apr 17 20:44, Bruno Haible via Cygwin wrote:
> Btw, there are two more functions in the posix_spawn family meanwhile:
>   * posix_spawn_file_actions_addchdir_np
>     implemented by glibc [1], musl libc, macOS, FreeBSD [2], Solaris ≥ 11.3
>     used by a few packages (Firefox, Chromium, Rust)
>   * posix_spawn_file_actions_addfchdir_np
>     implemented in glibc, musl libc
>     but not used by any package so far [3].
> 
> The next POSIX will contain these functions (without the _np suffix).[4]

Actually I had some spare time yesterday so I came up with an
implementation of posix_spawn_file_actions_addchdir_np and
posix_spawn_file_actions_addfchdir_np.  It's pretty straightforward:

https://cygwin.com/cgit/newlib-cygwin/commit/?id=7e03fc35f528
https://cygwin.com/cgit/newlib-cygwin/commit/?id=c743751aafa8

You can install the just building test release
cygwin-3.5.0-0.287.g53f7fb20a064 via our installer, if you'd
like to test it.

Actually... do you have a testcase readily available to share with us?


Thanks,
Corinna

Re: posix_spawn facility

[Bruno Haible]

I wrote:
>   * posix_spawn_file_actions_addfchdir_np
>     implemented in glibc, musl libc

Correction: posix_spawn_file_actions_addfchdir_np is implemented in
glibc, musl libc, FreeBSD ≥ 13.1, macOS ≥ 10.15.

Bruno

Re: posix_spawn_file_actions_add[f]chdir_np

[Bruno Haible]

Hi Corinna,

> Actually I had some spare time yesterday so I came up with an
> implementation of posix_spawn_file_actions_addchdir_np and
> posix_spawn_file_actions_addfchdir_np.  It's pretty straightforward:
> 
> https://cygwin.com/cgit/newlib-cygwin/commit/?id=7e03fc35f528

Yes, it's pretty straightforward on Unix-like platforms.

Note that there is a small inconsistency between implementations:
The manual page
https://man.freebsd.org/cgi/man.cgi?query=posix_spawn_file_actions_addfchdir_np&apropos=0&sektion=3&manpath=FreeBSD+13.2-RELEASE+and+Ports&arch=default&format=html
mentions that when the dirfd argument is negative, the
posix_spawn_file_actions_addfchdir function should fail with error EBADF.
musl libc, FreeBSD, macOS do this; glibc doesn't. Test program attached below.
Confirmed by looking at the source code:
- musl libc:
        if (fd < 0) return EBADF;
- FreeBSD:
        if (fildes < 0)
                return (EBADF);

> You can install the just building test release
> cygwin-3.5.0-0.287.g53f7fb20a064 via our installer, if you'd
> like to test it.
> 
> Actually... do you have a testcase readily available to share with us?

Indeed, I'm not likely to install test releases. (Due to the way I work
with my Windows VM, it's a bit clumsy to create a snapshot.) It's easier
for me to provide you a test case, with the unit tests from Gnulib.

I ran the command
  ./gnulib-tool --create-testdir --dir=../testdir-posix-spawn --single-configure \
     posix_spawn \
     posix_spawnattr_destroy \
     posix_spawnattr_getflags \
     posix_spawnattr_getpgroup \
     posix_spawnattr_getschedparam \
     posix_spawnattr_getschedpolicy \
     posix_spawnattr_getsigdefault \
     posix_spawnattr_getsigmask \
     posix_spawnattr_init \
     posix_spawnattr_setflags \
     posix_spawnattr_setpgroup \
     posix_spawnattr_setschedparam \
     posix_spawnattr_setschedpolicy \
     posix_spawnattr_setsigdefault \
     posix_spawnattr_setsigmask \
     posix_spawn_file_actions_addclose \
     posix_spawn_file_actions_adddup2 \
     posix_spawn_file_actions_addopen \
     posix_spawn_file_actions_addchdir \
     posix_spawn_file_actions_addfchdir \
     posix_spawn_file_actions_destroy \
     posix_spawn_file_actions_init \
     posix_spawnp

You can download the resulting tarball from
https://haible.de/bruno/gnu/testdir-posix-spawn.tar.gz .

When configuring it, set the environment variable
gl_cv_func_posix_spawnp_secure_exec=yes . This will prevent the
autoconfiguration from attempting to override the entire posix_spawn facility:
  $ env gl_cv_func_posix_spawnp_secure_exec=yes ./configure
Verify through
  $ grep REPLACE_POSIX_SPAWN config.status
that the values of REPLACE_POSIX_SPAWN and REPLACE_POSIX_SPAWNP are 0;
if they are 1, the output of 'configure' should reveal why.

Then build it:
  $ make
Verify through
  $ ls -l gllib/*.o
that no posix_spawn_*.o object file was built.

Then run
  $ make check
and it will run the unit tests against the Cygwin libc. It's likely
that you will see 1 test failure (test-posix_spawnp-script); but
other than that, there ought to be no test failures.

Bruno


================ Test program for EBADF check ================
#include <spawn.h>

int
main (void)
{
  posix_spawn_file_actions_t actions;

  if (posix_spawn_file_actions_init (&actions) != 0)
    return 1;

  if (posix_spawn_file_actions_addfchdir_np (&actions, -3) != 0)
    return 2;
  else
    return 3;
}

Re: posix_spawn facility

[Bruno Haible]

Eric Blake wrote:
> POSIX issue now filed as
> https://www.austingroupbugs.net/view.php?id=1674

Thank you very much!

Note a typo: line 126541 is on page 3694 not on page 3594.

Bruno

Re: posix_spawn_file_actions_add[f]chdir_np

[Corinna Vinschen]

Hi Bruno,

On Apr 19 13:24, Bruno Haible via Cygwin wrote:
> Hi Corinna,
> 
> > Actually I had some spare time yesterday so I came up with an
> > implementation of posix_spawn_file_actions_addchdir_np and
> > posix_spawn_file_actions_addfchdir_np.  It's pretty straightforward:
> > 
> > https://cygwin.com/cgit/newlib-cygwin/commit/?id=7e03fc35f528
> 
> Yes, it's pretty straightforward on Unix-like platforms.
> 
> Note that there is a small inconsistency between implementations:
> The manual page
> https://man.freebsd.org/cgi/man.cgi?query=posix_spawn_file_actions_addfchdir_np&apropos=0&sektion=3&manpath=FreeBSD+13.2-RELEASE+and+Ports&arch=default&format=html
> mentions that when the dirfd argument is negative, the
> posix_spawn_file_actions_addfchdir function should fail with error EBADF.
> musl libc, FreeBSD, macOS do this; glibc doesn't. Test program attached below.
> Confirmed by looking at the source code:
> - musl libc:
>         if (fd < 0) return EBADF;
> - FreeBSD:
>         if (fildes < 0)
>                 return (EBADF);

Ok, good to know.  I implemented it like glibc, so fd isn't tested in
posix_spawn_file_actions_addfchdir_np, but Austin group issue 1208 at
https://www.austingroupbugs.net/view.php?id=1208 documents it like it's
implemented in musl and FreeBSD:

  ERRORS
  The posix_spawn_file_actions_addfchdir( ) function shall fail if:
  [EBADF] The value specified by fildes is negative.
  [...]

I will fix this in Cygwin.

> > You can install the just building test release
> > cygwin-3.5.0-0.287.g53f7fb20a064 via our installer, if you'd
> > like to test it.
> > 
> > Actually... do you have a testcase readily available to share with us?
> 
> Indeed, I'm not likely to install test releases. (Due to the way I work
> with my Windows VM, it's a bit clumsy to create a snapshot.)

I don't quite understand the snapshot thingy.  Can't you just run
setup in your Windows VM?

> It's easier
> for me to provide you a test case, with the unit tests from Gnulib.
> [...]
> You can download the resulting tarball from
> https://haible.de/bruno/gnu/testdir-posix-spawn.tar.gz .
> 
> When configuring it, set the environment variable
> gl_cv_func_posix_spawnp_secure_exec=yes . This will prevent the
> autoconfiguration from attempting to override the entire posix_spawn facility:
>   $ env gl_cv_func_posix_spawnp_secure_exec=yes ./configure
> Verify through
>   $ grep REPLACE_POSIX_SPAWN config.status
> that the values of REPLACE_POSIX_SPAWN and REPLACE_POSIX_SPAWNP are 0;
> if they are 1, the output of 'configure' should reveal why.
> 
> Then build it:
>   $ make
> Verify through
>   $ ls -l gllib/*.o
> that no posix_spawn_*.o object file was built.
> 
> Then run
>   $ make check
> and it will run the unit tests against the Cygwin libc. It's likely
> that you will see 1 test failure (test-posix_spawnp-script); but
> other than that, there ought to be no test failures.

Indeed, thanks a lot!  I checked all the above and ran the testsuite.
All tests pass except the test-posix_spawnp-script.exe testcase.
Great!


Thanks again,
Corinna

Re: posix_spawn_file_actions_add[f]chdir_np

[Corinna Vinschen]

Hi Bruno,

On Apr 19 17:05, Corinna Vinschen via Cygwin wrote:
> On Apr 19 13:24, Bruno Haible via Cygwin wrote:
> > Note that there is a small inconsistency between implementations:
> > [...]
> > Confirmed by looking at the source code:
> > - musl libc:
> >         if (fd < 0) return EBADF;
> > - FreeBSD:
> >         if (fildes < 0)
> >                 return (EBADF);
> 
> Ok, good to know.  I implemented it like glibc, so fd isn't tested in
> posix_spawn_file_actions_addfchdir_np, but Austin group issue 1208 at
> https://www.austingroupbugs.net/view.php?id=1208 documents it like it's
> implemented in musl and FreeBSD:
> 
>   ERRORS
>   The posix_spawn_file_actions_addfchdir( ) function shall fail if:
>   [EBADF] The value specified by fildes is negative.
>   [...]
> 
> I will fix this in Cygwin.

Done: https://cygwin.com/cgit/newlib-cygwin/commit/?id=3124d8b436a8 

> > [...]
> > Then run
> >   $ make check
> > and it will run the unit tests against the Cygwin libc. It's likely
> > that you will see 1 test failure (test-posix_spawnp-script); but
> > other than that, there ought to be no test failures.
> 
> Indeed, thanks a lot!  I checked all the above and ran the testsuite.
> All tests pass except the test-posix_spawnp-script.exe testcase.
> Great!

I pushed the posix_spawnp patch for now, so all these tests PASS.

See https://cygwin.com/cgit/newlib-cygwin/commit/?id=da40bd6eaf40


Thanks,
Corinna

Re: posix_spawn_file_actions_add[f]chdir_np

[Eliot Moss]

A wondering in all this ...

Does having more spawn support imply that bash (for example)
may end up doing faster process spawning, skipping some of
high overhead we've lived with for a long time because of
the Windows process spawning model?

Regards - Eliot Moss

Re: posix_spawn facility

[Csaba Raduly]

On Wed, 19 Apr 2023 at 13:56, Bruno Haible via Cygwin <cygwin@cygwin.com> wrote:
> Note a typo: line 126541 is on page 3694 not on page 3594.
>

That sentence is scary in and of itself :)

Csaba
-- 
You can get very substantial performance improvements
by not doing the right thing. - Scott Meyers, An Effective C++11/14 Sampler
So if you're looking for a completely portable, 100% standards-conformant way
to get the wrong information: this is what you want. - Scott Meyers (C++TDaWYK)

Re: posix_spawn facility

[gs-cygwin.com]

On Mon, Apr 17, 2023 at 08:44:51PM +0200, Bruno Haible via Cygwin wrote:
> Btw, there are two more functions in the posix_spawn family meanwhile:
>   * posix_spawn_file_actions_addchdir_np
>     implemented by glibc [1], musl libc, macOS, FreeBSD [2], Solaris ≥ 11.3
>     used by a few packages (Firefox, Chromium, Rust)
>   * posix_spawn_file_actions_addfchdir_np
>     implemented in glibc, musl libc
>     but not used by any package so far [3].
> 
> The next POSIX will contain these functions (without the _np suffix).[4]
> 
> Bruno
> 
> [1] https://sourceware.org/bugzilla/show_bug.cgi?id=17405
> [2] https://man.freebsd.org/cgi/man.cgi?query=posix_spawn_file_actions_adddup2&apropos=0&sektion=3&manpath=FreeBSD+11-current&format=html
> [3] https://codesearch.debian.net/
> [4] https://www.austingroupbugs.net/view.php?id=1208

With regards to [3] above, the next lighttpd release (lighttpd 1.4.70)
will use posix_spawn_file_actions_addfchdir_np(), where available, for
spawning CGI programs.

I have not yet tested under cygwin, but under Linux the overhead of
initial CGI process creation can be greatly reduced by using
posix_spawn() instead of fork(),execve().  The speedup is inversely
proportional to how much work the target script performs (compared to
the overhead of initial process creation).  On x86_64 Linux, running a
minimal C program for CGI can be ~60% faster in lighttpd when using
posix_spawn()!  When running a minimal /bin/sh program for CGI, the
speedup is "only" ~20%!  (Those numbers were obtained from running
h2load and weighttp microbenchmarks with the minimal CGI programs.)

minimal C program for CGI (compiled gcc -O3):
  #include <unistd.h>
  static const char resp[] = "Status: 200\n\n";
  int main(void) { write(STDOUT_FILENO, resp, sizeof(resp)-1); return 0; }

minimal /bin/sh program for CGI:
  #!/bin/sh
  printf 'Status: 200\n\n'

Cheers, Glenn

Re: posix_spawn facility

[Corinna Vinschen]

On Apr 20 03:14, gs-cygwin.com--- via Cygwin wrote:
> On Mon, Apr 17, 2023 at 08:44:51PM +0200, Bruno Haible via Cygwin wrote:
> > Btw, there are two more functions in the posix_spawn family meanwhile:
> >   * posix_spawn_file_actions_addchdir_np
> >     implemented by glibc [1], musl libc, macOS, FreeBSD [2], Solaris ≥ 11.3
> >     used by a few packages (Firefox, Chromium, Rust)
> >   * posix_spawn_file_actions_addfchdir_np
> >     implemented in glibc, musl libc
> >     but not used by any package so far [3].
> > 
> > The next POSIX will contain these functions (without the _np suffix).[4]
> > 
> > Bruno
> > 
> > [1] https://sourceware.org/bugzilla/show_bug.cgi?id=17405
> > [2] https://man.freebsd.org/cgi/man.cgi?query=posix_spawn_file_actions_adddup2&apropos=0&sektion=3&manpath=FreeBSD+11-current&format=html
> > [3] https://codesearch.debian.net/
> > [4] https://www.austingroupbugs.net/view.php?id=1208
> 
> With regards to [3] above, the next lighttpd release (lighttpd 1.4.70)
> will use posix_spawn_file_actions_addfchdir_np(), where available, for
> spawning CGI programs.
> 
> I have not yet tested under cygwin, but under Linux the overhead of
> initial CGI process creation can be greatly reduced by using
> posix_spawn() instead of fork(),execve().  The speedup is inversely
> proportional to how much work the target script performs (compared to
> the overhead of initial process creation).

Unfortunately you can't expect any noticable difference on Cygwin by
using posix_spawn.  While Cygwin has a spawn() family of functions, we
don't (and can't... yet) use them.

The problem is that we don't have a safe way to perform the spawn
attributes and file actions which are supposed to be performed between
the fork() and the exec() step when using the spawn() functions.  This
would have to be implemented first.

So the difference between Linux and Cygwin is that Linux has a vfork()
call, which performs a quick and shallow kind of fork() with the sole
intention to exec() asap, and this is (via __clone or __clone3) used in
glibc to implement posix_spawn().

We tried to implement a shallow vfork() on Cygwin as well, years ago,
but it never really worked correctly, so we gave it up.  vfork() on
Cygwin is the same as a real fork().

Bottom line is, I will keep this problem in mind, and maybe we can come
up with a new, quicker solution, using the spawn() functions.

Our major problem is that we are just too few people actually
hacking on Cygwin, and all of us are doing it mostly in spare time.
Patches are, as always, gratefully received.


Corinna

Re: posix_spawn facility

[Bruno Haible]

Corinna Vinschen wrote:
> Unfortunately you can't expect any noticable difference on Cygwin by
> using posix_spawn.  While Cygwin has a spawn() family of functions, we
> don't (and can't... yet) use them.
> 
> The problem is that we don't have a safe way to perform the spawn
> attributes and file actions which are supposed to be performed between
> the fork() and the exec() step when using the spawn() functions.  This
> would have to be implemented first.

So, you are saying that on Cygwin, spawnvpe() and friends are fast,
because child_info_spawn::worker ends up calling CreateProcessW rather
immediately? But posix_spawn is slow, because it goes another path,
through fork()?

If that's the case, it might help to look at how I created Gnulib's
posix_spawn for native Windows:

  1) Extend the spawnvpe function to a spawnvpech function. It takes
     additional arguments
       - const char *currdir
       - int currdirfd
       - HANDLE stdin_handle, HANDLE stdout_handle, HANDLE stderr_handle
     (The latter is because the first 3 file descriptors are passed
     specially on Windows.)

  2) Refactor the guts of spawnvpech, moving as much as possible into
     helper functions. In my case, spawnvpech shrunk to only 150 lines
     of code.

     The helper functions, in the native Windows case, were:
       - prepare_spawn
       - compose_command
       - compose_envblock
       - init_inheritable_handles, compose_handles_block,
         free_inheritable_handles
       - convert_CreateProcess_error
     See https://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=blob;f=lib/windows-spawn.h .

     I would expect that similar helper functions can be created in
     Cygwin.

     The "inheritable handles" is a data structure that allows for the
     arbitrary reshuffling of file descriptors required by posix_spawn
     (the addopen, adddup2, addclose actions), i.e. which does the book-
     keeping which HANDLE must in the end be open in the parent and which
     must in the end be open in the child, and at which position. While
     at the same time optimizing the number of DuplicateHandle calls.
     Without this optimization, the code is simpler, but there were many
     redundant DuplicateHandle calls.
     This data structure is essential for posix_spawn[p] to be multithread-
     safe. posix_spawn[p] must not modify the file descriptors of the
     parent; instead it has to create the planned file descriptors for the
     child in memory.

     During this refactoring, it is essential to have a good test suite.
     Because when you make a mistake and leave a fd / HANDLE accidentally
     open (in the parent or in the child), applications that invoke a pipe
     start to hang.

  3) With these helper functions, write a new core for posix_spawn[p].
     In my case, it was less than 250 lines of code, which is not
     straightforward, but also not really hard either.
     See https://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=blob;f=lib/spawni.c
     lines 610..852.

     At this step, you can use Gnulib's test suite for verifying that the new
     code works properly.

Hope this helps.

Bruno

Re: posix_spawn_file_actions_add[f]chdir_np

[Bruno Haible]

Corinna Vinschen wrote:
> I pushed the posix_spawnp patch for now, so all these tests PASS.
> 
> See https://cygwin.com/cgit/newlib-cygwin/commit/?id=da40bd6eaf40

Cool!

Re: posix_spawn facility

[Corinna Vinschen]

On Apr 20 12:18, Bruno Haible via Cygwin wrote:
> Corinna Vinschen wrote:
> > Unfortunately you can't expect any noticable difference on Cygwin by
> > using posix_spawn.  While Cygwin has a spawn() family of functions, we
> > don't (and can't... yet) use them.
> > 
> > The problem is that we don't have a safe way to perform the spawn
> > attributes and file actions which are supposed to be performed between
> > the fork() and the exec() step when using the spawn() functions.  This
> > would have to be implemented first.
> 
> So, you are saying that on Cygwin, spawnvpe() and friends are fast,
> because child_info_spawn::worker ends up calling CreateProcessW rather
> immediately? But posix_spawn is slow, because it goes another path,
> through fork()?

Basically, it's still fork/exec, just with some additional code to
handle process synchronization so we can propagate the error from a
failing execve to the parent process.

> If that's the case, it might help to look at how I created Gnulib's
> posix_spawn for native Windows:

Great, thanks for that.  I'm a bit busy ATM, but I'll take a look,
this might be very helpful.

>   1) Extend the spawnvpe function to a spawnvpech function. It takes
>      additional arguments
>        - const char *currdir
>        - int currdirfd
>        - HANDLE stdin_handle, HANDLE stdout_handle, HANDLE stderr_handle
>      (The latter is because the first 3 file descriptors are passed
>      specially on Windows.)
> 
>   2) Refactor the guts of spawnvpech, moving as much as possible into
>      helper functions. In my case, spawnvpech shrunk to only 150 lines
>      of code.
> 
>      The helper functions, in the native Windows case, were:
>        - prepare_spawn
>        - compose_command
>        - compose_envblock
>        - init_inheritable_handles, compose_handles_block,
>          free_inheritable_handles
>        - convert_CreateProcess_error
>      See https://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=blob;f=lib/windows-spawn.h .
> 
>      I would expect that similar helper functions can be created in
>      Cygwin.

The additional problem in Cygwin is that we also have to care for the
POSIX stuff, like signals, effective vs. real UID/GID, etc.

>      The "inheritable handles" is a data structure that allows for the
>      arbitrary reshuffling of file descriptors required by posix_spawn
>      (the addopen, adddup2, addclose actions), i.e. which does the book-
>      keeping which HANDLE must in the end be open in the parent and which
>      must in the end be open in the child, and at which position.

Hmm.  Your code uses lpReserved2 for that, but the functionality is
one implemented in MSVCRT.  For obvious reasons, Cygwin executables
are not linked against msvcrt.dll and we're using lpReserved2 for our
own purposes.

While we probably could implement the same mechanism, we would have to
implement the target process side as well, and the result would be
incompatible with MSVCRT.  We also have virtual files with handles
pointing to \Device\Null (just so an inheritable handle is available)
Additionally, we already propagate complete fhandlers (the datastructures
the descriptors in Cygwin are pointing to) via the Cygwin heap.

The annoying thing here is that we probably need tow different
mechanisms, one for Cygwin POSIX child processes, and one for native
Windows child processes.

>      During this refactoring, it is essential to have a good test suite.
>      Because when you make a mistake and leave a fd / HANDLE accidentally
>      open (in the parent or in the child), applications that invoke a pipe
>      start to hang.
> 
>   3) With these helper functions, write a new core for posix_spawn[p].
>      In my case, it was less than 250 lines of code, which is not
>      straightforward, but also not really hard either.
>      See https://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=blob;f=lib/spawni.c
>      lines 610..852.
> 
>      At this step, you can use Gnulib's test suite for verifying that the new
>      code works properly.
> 
> Hope this helps.

I think so, yes, thanks a lot!

Corinna

Re: posix_spawn facility

[Corinna Vinschen]

On Apr 20 16:21, Corinna Vinschen via Cygwin wrote:
> On Apr 20 12:18, Bruno Haible via Cygwin wrote:
> >      The "inheritable handles" is a data structure that allows for the
> >      arbitrary reshuffling of file descriptors required by posix_spawn
> >      (the addopen, adddup2, addclose actions), i.e. which does the book-
> >      keeping which HANDLE must in the end be open in the parent and which
> >      must in the end be open in the child, and at which position.
> 
> Hmm.  Your code uses lpReserved2 for that, but the functionality is
> one implemented in MSVCRT.  For obvious reasons, Cygwin executables
> are not linked against msvcrt.dll and we're using lpReserved2 for our
> own purposes.

Oh, btw., did you know that there's a newer mechanism for defining
specific inheritable handles to CreateProcess, which is implemented
in kernel32.dll, so it does not depend on MSVCRT?

There's a STARTUPINFOEX structure which allows to specify the 
additional handles.  See

https://learn.microsoft.com/en-us/windows/win32/api/winbase/ns-winbase-startupinfoexa

and the PROC_THREAD_ATTRIBUTE_HANDLE_LIST argument described in

https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-updateprocthreadattribute


Corinna

Re: posix_spawn facility

[Bruno Haible]

Corinna Vinschen wrote:
> > Hmm.  Your code uses lpReserved2 for that, but the functionality is
> > one implemented in MSVCRT.  For obvious reasons, Cygwin executables
> > are not linked against msvcrt.dll and we're using lpReserved2 for our
> > own purposes.
> 
> Oh, btw., did you know that there's a newer mechanism for defining
> specific inheritable handles to CreateProcess, which is implemented
> in kernel32.dll, so it does not depend on MSVCRT?
> 
> There's a STARTUPINFOEX structure which allows to specify the 
> additional handles.  See
> 
> https://learn.microsoft.com/en-us/windows/win32/api/winbase/ns-winbase-startupinfoexa
> 
> and the PROC_THREAD_ATTRIBUTE_HANDLE_LIST argument described in
> 
> https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-updateprocthreadattribute

Indeed, this appears to be a more "official" way to pass handles for fd ≥ 3,
instead of lpReserved2 — albeit without associated 'flags'. Not sure how
O_APPEND is handled then... (Note: O_APPEND behaviour is tested by
gnulib/tests/test-posix_spawn-open2.c.)

I had seen this doc page, but thought it was irrelevant because the
title is about "thread attributes", not "process attributes"...

Bruno

Re: posix_spawn facility

[Corinna Vinschen]

On Apr 20 16:58, Bruno Haible via Cygwin wrote:
> Corinna Vinschen wrote:
> > > Hmm.  Your code uses lpReserved2 for that, but the functionality is
> > > one implemented in MSVCRT.  For obvious reasons, Cygwin executables
> > > are not linked against msvcrt.dll and we're using lpReserved2 for our
> > > own purposes.
> > 
> > Oh, btw., did you know that there's a newer mechanism for defining
> > specific inheritable handles to CreateProcess, which is implemented
> > in kernel32.dll, so it does not depend on MSVCRT?
> > 
> > There's a STARTUPINFOEX structure which allows to specify the 
> > additional handles.  See
> > 
> > https://learn.microsoft.com/en-us/windows/win32/api/winbase/ns-winbase-startupinfoexa
> > 
> > and the PROC_THREAD_ATTRIBUTE_HANDLE_LIST argument described in
> > 
> > https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-updateprocthreadattribute
> 
> Indeed, this appears to be a more "official" way to pass handles for fd ≥ 3,
> instead of lpReserved2 — albeit without associated 'flags'. Not sure how
> O_APPEND is handled then...

Yeah, theoretically, that should be handled by CreateFile opening the
file with FILE_APPEND_DATA attribute, and in the child MSVCRT should
test with NtQueryInformationFile(FILE_ACCESS_INFORMATION) if the
FILE_APPEND_DATA flag is set.

But then again, if MSVCRT implements fcntl (F_SETFL) to allow
manipulating the O_APPEND flag... unfortunately there's no such
operation via Win32 or native calls.  That would require to reopen the
file with different access mask and replace the HANDLE under the hood of
the descriptor.  I'm not aware if and how MSVCRT performs this action.


Corinna

Re: posix_spawn facility

[gs-cygwin.com]

On Thu, Apr 20, 2023 at 04:40:52PM +0200, Corinna Vinschen via Cygwin wrote:
> On Apr 20 16:21, Corinna Vinschen via Cygwin wrote:
> > On Apr 20 12:18, Bruno Haible via Cygwin wrote:
> > >      The "inheritable handles" is a data structure that allows for the
> > >      arbitrary reshuffling of file descriptors required by posix_spawn
> > >      (the addopen, adddup2, addclose actions), i.e. which does the book-
> > >      keeping which HANDLE must in the end be open in the parent and which
> > >      must in the end be open in the child, and at which position.
> > 
> > Hmm.  Your code uses lpReserved2 for that, but the functionality is
> > one implemented in MSVCRT.  For obvious reasons, Cygwin executables
> > are not linked against msvcrt.dll and we're using lpReserved2 for our
> > own purposes.
> 
> Oh, btw., did you know that there's a newer mechanism for defining
> specific inheritable handles to CreateProcess, which is implemented
> in kernel32.dll, so it does not depend on MSVCRT?
> 
> There's a STARTUPINFOEX structure which allows to specify the 
> additional handles.  See
> 
> https://learn.microsoft.com/en-us/windows/win32/api/winbase/ns-winbase-startupinfoexa
> 
> and the PROC_THREAD_ATTRIBUTE_HANDLE_LIST argument described in
> 
> https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-updateprocthreadattribute

An example using these to run CGI programs in lighttpd on Windows
using native _WIN32 code can be seen on my dev branch (intended for
lighttpd 1.4.70).

https://git.lighttpd.net/lighttpd/lighttpd1.4/src/branch/personal/gstrauss/master/src/fdevent_win32.c#L539
(lighttpd is BSD-3-Clause)

The aim is to securely manage which handles are inherited by child CGI
processes.  Elsewhere, lighttpd clears HANDLE_FLAG_INHERIT on handles
lighttpd creates, and uses WSA_FLAG_NO_HANDLE_INHERIT in calls to
WSASocket() func family.  Also in that file, I have an (expensive)
socketpair() implementation, since lighttpd uses sockets instead of
pipes to connect to CGI programs, so that events can be received using
WSAPoll() or select().

To get things working in native _WIN32 lighttpd, which has a test
framework that runs under Cygwin, I had to detect and perform some path
translation between native Windows and Cygwin paths.  While my solution
is specific to lighttpd's use, I hope that this may give you some ideas.

Cheers, Glenn

Re: posix_spawn facility

[gs-cygwin.com]

On Thu, Apr 20, 2023 at 04:58:20PM +0200, Bruno Haible via Cygwin wrote:
> Corinna Vinschen wrote:
> > > Hmm.  Your code uses lpReserved2 for that, but the functionality is
> > > one implemented in MSVCRT.  For obvious reasons, Cygwin executables
> > > are not linked against msvcrt.dll and we're using lpReserved2 for our
> > > own purposes.
> > 
> > Oh, btw., did you know that there's a newer mechanism for defining
> > specific inheritable handles to CreateProcess, which is implemented
> > in kernel32.dll, so it does not depend on MSVCRT?
> > 
> > There's a STARTUPINFOEX structure which allows to specify the 
> > additional handles.  See
> > 
> > https://learn.microsoft.com/en-us/windows/win32/api/winbase/ns-winbase-startupinfoexa
> > 
> > and the PROC_THREAD_ATTRIBUTE_HANDLE_LIST argument described in
> > 
> > https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-updateprocthreadattribute
> 
> Indeed, this appears to be a more "official" way to pass handles for fd ≥ 3,
> instead of lpReserved2 — albeit without associated 'flags'. Not sure how
> O_APPEND is handled then... (Note: O_APPEND behaviour is tested by
> gnulib/tests/test-posix_spawn-open2.c.)
> 
> I had seen this doc page, but thought it was irrelevant because the
> title is about "thread attributes", not "process attributes"...

Excellent (very technical) article on the subject:

Programmatically controlling which handles are inherited by new processes in Win32
https://devblogs.microsoft.com/oldnewthing/20111216-00/?p=8873

Cheers, Glenn

Re: posix_spawn facility

[gs-cygwin.com]

On Thu, Apr 20, 2023 at 05:40:52PM +0200, Corinna Vinschen via Cygwin wrote:
> On Apr 20 16:58, Bruno Haible via Cygwin wrote:
> > Corinna Vinschen wrote:
> > > > Hmm.  Your code uses lpReserved2 for that, but the functionality is
> > > > one implemented in MSVCRT.  For obvious reasons, Cygwin executables
> > > > are not linked against msvcrt.dll and we're using lpReserved2 for our
> > > > own purposes.
> > > 
> > > Oh, btw., did you know that there's a newer mechanism for defining
> > > specific inheritable handles to CreateProcess, which is implemented
> > > in kernel32.dll, so it does not depend on MSVCRT?
> > > 
> > > There's a STARTUPINFOEX structure which allows to specify the 
> > > additional handles.  See
> > > 
> > > https://learn.microsoft.com/en-us/windows/win32/api/winbase/ns-winbase-startupinfoexa
> > > 
> > > and the PROC_THREAD_ATTRIBUTE_HANDLE_LIST argument described in
> > > 
> > > https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-updateprocthreadattribute
> > 
> > Indeed, this appears to be a more "official" way to pass handles for fd ≥ 3,
> > instead of lpReserved2 — albeit without associated 'flags'. Not sure how
> > O_APPEND is handled then...
> 
> Yeah, theoretically, that should be handled by CreateFile opening the
> file with FILE_APPEND_DATA attribute, and in the child MSVCRT should
> test with NtQueryInformationFile(FILE_ACCESS_INFORMATION) if the
> FILE_APPEND_DATA flag is set.
> 
> But then again, if MSVCRT implements fcntl (F_SETFL) to allow
> manipulating the O_APPEND flag... unfortunately there's no such
> operation via Win32 or native calls.  That would require to reopen the
> file with different access mask and replace the HANDLE under the hood of
> the descriptor.  I'm not aware if and how MSVCRT performs this action.

If you are carefully controlling and allowing an explicit set of file
handles to be inherited, and the entire program uses this interface to
create new processes, then you can safely _sopen_s() or otherwise to
create new handles, pass them to CreateProcess() using STARTUPINFOEX,
and then close any new handles created solely for inheritance in child.

Cheers, Glenn

Re: posix_spawn facility

[Bruno Haible]

Glenn wrote:
> > > https://learn.microsoft.com/en-us/windows/win32/api/winbase/ns-winbase-startupinfoexa
> > > 
> > > and the PROC_THREAD_ATTRIBUTE_HANDLE_LIST argument described in
> > > 
> > > https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-updateprocthreadattribute
> > ...
> Excellent (very technical) article on the subject:
> 
> Programmatically controlling which handles are inherited by new processes in Win32
> https://devblogs.microsoft.com/oldnewthing/20111216-00/?p=8873

It's nice to see an example for PROC_THREAD_ATTRIBUTE_HANDLE_LIST.

But the article exaggerates a problem:
  "But all this inheritability fiddling still had a fatal flaw: What
   if two threads within the same process both call Create­Process but
   disagree on which handles they want to be inherited?"
The answer, overlooked in the article, is to use DuplicateHandle
and set the inheritability of the duplicate to true. Concurrently
running posix_spawn invocations in other threads will not see the
duplicates, since they only see HANDLEs that are assigned to file
descriptors, not HANDLEs that merely reside in some data structure
in memory.

Bruno

Re: posix_spawn facility

[gs-cygwin.com]

On Thu, Apr 20, 2023 at 09:31:38PM +0200, Bruno Haible wrote:
> Glenn wrote:
> > > > https://learn.microsoft.com/en-us/windows/win32/api/winbase/ns-winbase-startupinfoexa
> > > > 
> > > > and the PROC_THREAD_ATTRIBUTE_HANDLE_LIST argument described in
> > > > 
> > > > https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-updateprocthreadattribute
> > > ...
> > Excellent (very technical) article on the subject:
> > 
> > Programmatically controlling which handles are inherited by new processes in Win32
> > https://devblogs.microsoft.com/oldnewthing/20111216-00/?p=8873
> 
> It's nice to see an example for PROC_THREAD_ATTRIBUTE_HANDLE_LIST.
> 
> But the article exaggerates a problem:
>   "But all this inheritability fiddling still had a fatal flaw: What
>    if two threads within the same process both call Create­Process but
>    disagree on which handles they want to be inherited?"
> The answer, overlooked in the article, is to use DuplicateHandle
> and set the inheritability of the duplicate to true. Concurrently
> running posix_spawn invocations in other threads will not see the
> duplicates, since they only see HANDLEs that are assigned to file
> descriptors, not HANDLEs that merely reside in some data structure
> in memory.

It might not be an issue if everything -- and I mean everything -- goes
through posix_spawn() to create processes.

The article is from 2011 and about Windows.  If a third-party dll
running in another thread calls CreateProcess() and does not explicitly
restrict the inherited handles using the techiques in the article, then
there is still that race that might leak additional handles into the
other process.

In the case of cygwin, the cygwin layer could/should be able to
centralize and control process creation, avoiding the race.
Even if there were any steps that need to be protected, wrapping
in a CriticalSection (or mutex) would probably be sufficient.

Cheers, Glenn

Re: posix_spawn facility

[gs-cygwin.com]

On Thu, Apr 20, 2023 at 03:14:59AM -0400, gs-cygwin.com@gluelogic.com wrote:
> On Mon, Apr 17, 2023 at 08:44:51PM +0200, Bruno Haible via Cygwin wrote:
> > Btw, there are two more functions in the posix_spawn family meanwhile:
> >   * posix_spawn_file_actions_addchdir_np
> >     implemented by glibc [1], musl libc, macOS, FreeBSD [2], Solaris ≥ 11.3
> >     used by a few packages (Firefox, Chromium, Rust)
> >   * posix_spawn_file_actions_addfchdir_np
> >     implemented in glibc, musl libc
> >     but not used by any package so far [3].
> > 
> > The next POSIX will contain these functions (without the _np suffix).[4]
> > 
> > Bruno
> > 
> > [1] https://sourceware.org/bugzilla/show_bug.cgi?id=17405
> > [2] https://man.freebsd.org/cgi/man.cgi?query=posix_spawn_file_actions_adddup2&apropos=0&sektion=3&manpath=FreeBSD+11-current&format=html
> > [3] https://codesearch.debian.net/
> > [4] https://www.austingroupbugs.net/view.php?id=1208
> 
> With regards to [3] above, the next lighttpd release (lighttpd 1.4.70)
> will use posix_spawn_file_actions_addfchdir_np(), where available, for
> spawning CGI programs.
> 
> I have not yet tested under cygwin, but under Linux the overhead of
> initial CGI process creation can be greatly reduced by using
> posix_spawn() instead of fork(),execve().  The speedup is inversely
> proportional to how much work the target script performs (compared to
> the overhead of initial process creation).  On x86_64 Linux, running a
> minimal C program for CGI can be ~60% faster in lighttpd when using
> posix_spawn()!  When running a minimal /bin/sh program for CGI, the
> speedup is "only" ~20%!  (Those numbers were obtained from running
> h2load and weighttp microbenchmarks with the minimal CGI programs.)
> 
> minimal C program for CGI (compiled gcc -O3):
>   #include <unistd.h>
>   static const char resp[] = "Status: 200\n\n";
>   int main(void) { write(STDOUT_FILENO, resp, sizeof(resp)-1); return 0; }
> 
> minimal /bin/sh program for CGI:
>   #!/bin/sh
>   printf 'Status: 200\n\n'
> 
> Cheers, Glenn

lighttpd 1.4.70 has been released for cygwin and uses posix_spawn() when
running CGI programs.

Under cygwin 3.4.0, posix_spawn_file_actions_addfchdir_np() is not
detected and so not used, but once cygwin 3.5.0 is out, I'll rebuild the
lighttpd package on cygwin to take advantage of it.

Cheers, Glenn

Re: posix_spawn facility

[gs-cygwin.com]

On Thu, Apr 20, 2023 at 04:00:15PM -0400, gs-cygwin.com@gluelogic.com wrote:
> On Thu, Apr 20, 2023 at 09:31:38PM +0200, Bruno Haible wrote:
> > Glenn wrote:
> > > > > https://learn.microsoft.com/en-us/windows/win32/api/winbase/ns-winbase-startupinfoexa
> > > > > 
> > > > > and the PROC_THREAD_ATTRIBUTE_HANDLE_LIST argument described in
> > > > > 
> > > > > https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-updateprocthreadattribute
> > > > ...
> > > Excellent (very technical) article on the subject:
> > > 
> > > Programmatically controlling which handles are inherited by new processes in Win32
> > > https://devblogs.microsoft.com/oldnewthing/20111216-00/?p=8873
> > 
> > It's nice to see an example for PROC_THREAD_ATTRIBUTE_HANDLE_LIST.
> > 
> > But the article exaggerates a problem:
> >   "But all this inheritability fiddling still had a fatal flaw: What
> >    if two threads within the same process both call Create­Process but
> >    disagree on which handles they want to be inherited?"
> > The answer, overlooked in the article, is to use DuplicateHandle
> > and set the inheritability of the duplicate to true. Concurrently
> > running posix_spawn invocations in other threads will not see the
> > duplicates, since they only see HANDLEs that are assigned to file
> > descriptors, not HANDLEs that merely reside in some data structure
> > in memory.
> 
> It might not be an issue if everything -- and I mean everything -- goes
> through posix_spawn() to create processes.
> 
> The article is from 2011 and about Windows.  If a third-party dll
> running in another thread calls CreateProcess() and does not explicitly
> restrict the inherited handles using the techiques in the article, then
> there is still that race that might leak additional handles into the
> other process.
> 
> In the case of cygwin, the cygwin layer could/should be able to
> centralize and control process creation, avoiding the race.
> Even if there were any steps that need to be protected, wrapping
> in a CriticalSection (or mutex) would probably be sufficient.
> 
> Cheers, Glenn

lighttpd 1.4.70 includes support for native _WIN32 (separate build
from cygwin) and includes a working (but slow) _WIN32 socketpair() as
well as fully-functional code using CreateProcess() with the above
techniques to limit filehandles inherited by CGI processes to only the
stdhandles for stdin, stdout, and stderr.

lighttpd 1.4.70 uses sockets (via socketpair()) instead of pipes for
CGI on Windows since select() and WSAPoll() work only on sockets.


Corinna:
With the somewhat recent update to minimum Windows version supported by
cygwin, I believe that using PROC_THREAD_ATTRIBUTE_HANDLE_LIST should be
available on all cygwin-supported versions of Windows.

Cheers, Glenn

P.S. if any Windows developers look at the (BSD-3-Clause) lighttpd code
and notice that I am doing something wrong or suboptimal on _WIN32,
please do let me know how it can be improved.  Thanks!

Re: posix_spawn facility

[Eric Blake]

Following up on an older thread:

On Tue, Apr 18, 2023 at 03:49:20PM -0500, Eric Blake wrote:
> The glibc bug points to the sample posix_spawn() implementation in
> POSIX XRAT - but that example implementation is non-normative and
> known buggy, so it is not safe to rely on it.
> 
> Clarifying the wording in XRAT to explicitly mention that the example
> is NOT bullet-proof (and that implementations should do better) is
> probably worthwhile; I'll tackle that bug report.
> 
> > 
> > Second, the rational section in POSIX explains posix_spawn and
> > posix_spawnp, but it does *not* actually provide an example
> > implementation of posix_spawnp, only of posix_spawn.
> 
> POSIX is silent as to whether posix_spawnp() has to fall back to 'sh'
> on ENOEXEC failure.  The p suffix is indeed similar to execvp() (which
> DOES require a fallback to sh), but it could also just mean a
> PATH-search, and not the PATH-search-and-sh-fallback of execvp().  As
> we now have implementations in the wild that differ in behavior, and
> use security as a reason for the divergence, it is worth getting that
> clarified in POSIX.  I'll file a bug against POSIX shortly, and reply
> again once it is up.
> 
> My personal preference: sh fallback on ENOEXEC is useful in execvp(),
> but a bear to get right (see
> https://www.austingroupbugs.net/view.php?id=1645 where POSIX has a bug
> in requiring argv[0] to be the script's filename, which breaks busybox
> sh and is NOT what glibc does; meanwhile, musl intentionally does NOT
> do the sh fallback), so NOT doing it in posix_spawnp() would be
> reasonable; but we'll have to see what the rest of the Austin Group
> says.

...

> 
> Yeah, it appears that POSIX is (accidentally) silent on whether
> posix_spawnp() has to do the sh fallback on ENOEXEC; but it seems
> quite reasonable that posix_spawn() being more like execle() must NOT
> do a sh fallback.

The Austin Group finally visited the topic today; result is that in
the next version of POSIX, it will be explicit that neither
posix_spawn() nor posix_spawnp() are allowed to attempt sh fallback
(instead, they must fail with ENOEXEC if detected in the parent, or
with status 127 if after creating the child).

https://austingroupbugs.net/view.php?id=1674#c6411

Yes, it's odd that ENOEXEC normally equates to status 126, but does
not do so for posix_spawn().  If you want to add an extention
POSIX_SPAWN flag (for use in posix_spawnattr_setflags()) to further
tweak things as an extension to the standard, that would probably be
reasonable, but without implementations already implementing and
relying on such extension flags, the Austin Group did not want to
visit that topic today.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

Re: posix_spawn facility

[Corinna Vinschen]

Hi Eric,

On Jul 31 13:58, Eric Blake via Cygwin wrote:
> Following up on an older thread:
> 
> On Tue, Apr 18, 2023 at 03:49:20PM -0500, Eric Blake wrote:
> > The glibc bug points to the sample posix_spawn() implementation in
> > POSIX XRAT - but that example implementation is non-normative and
> > known buggy, so it is not safe to rely on it.
> > 
> > Clarifying the wording in XRAT to explicitly mention that the example
> > is NOT bullet-proof (and that implementations should do better) is
> > probably worthwhile; I'll tackle that bug report.
> > 
> > > 
> > > Second, the rational section in POSIX explains posix_spawn and
> > > posix_spawnp, but it does *not* actually provide an example
> > > implementation of posix_spawnp, only of posix_spawn.
> > 
> > POSIX is silent as to whether posix_spawnp() has to fall back to 'sh'
> > on ENOEXEC failure.  The p suffix is indeed similar to execvp() (which
> > DOES require a fallback to sh), but it could also just mean a
> > PATH-search, and not the PATH-search-and-sh-fallback of execvp().  As
> > we now have implementations in the wild that differ in behavior, and
> > use security as a reason for the divergence, it is worth getting that
> > clarified in POSIX.  I'll file a bug against POSIX shortly, and reply
> > again once it is up.
> > 
> > My personal preference: sh fallback on ENOEXEC is useful in execvp(),
> > but a bear to get right (see
> > https://www.austingroupbugs.net/view.php?id=1645 where POSIX has a bug
> > in requiring argv[0] to be the script's filename, which breaks busybox
> > sh and is NOT what glibc does; meanwhile, musl intentionally does NOT
> > do the sh fallback), so NOT doing it in posix_spawnp() would be
> > reasonable; but we'll have to see what the rest of the Austin Group
> > says.
> 
> ...
> 
> > 
> > Yeah, it appears that POSIX is (accidentally) silent on whether
> > posix_spawnp() has to do the sh fallback on ENOEXEC; but it seems
> > quite reasonable that posix_spawn() being more like execle() must NOT
> > do a sh fallback.
> 
> The Austin Group finally visited the topic today; result is that in
> the next version of POSIX, it will be explicit that neither
> posix_spawn() nor posix_spawnp() are allowed to attempt sh fallback
> (instead, they must fail with ENOEXEC if detected in the parent, or
> with status 127 if after creating the child).
> 
> https://austingroupbugs.net/view.php?id=1674#c6411

Ok, clear words.  Fortunately I already pushed that patch a while ago:
https://cygwin.com/cgit/newlib-cygwin/commit/?id=da40bd6eaf40


Thanks for keeping us in the loop!


Corinna