bugmake - Bugs: bug #63070, posix_spawn fails to run a child...

 
 

bug #63070: posix_spawn fails to run a child process.

Submitter:  Dmitry Goncharov <dgoncharov>
Submitted:  Sat 17 Sep 2022 01:23:19 AM UTC
   
 
Severity:  3 - Normal Item Group:  Bug
Status:  Fixed Privacy:  Public
Assigned to:  None Open/Closed:  Closed
Component Version:  4.4 Operating System:  POSIX-Based
Fixed Release:  4.4 Triage Status:  Small Effort
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Sun 25 Sep 2022 10:43:48 PM UTC, comment #15: 

OK I fixed the configure.ac checking program.  Hopefully it works properly now.

Paul D. Smith <psmith>
Group administrator
Wed 21 Sep 2022 09:46:55 AM UTC, comment #14: 

comment #10:

> In glibc-2.17 posix_spawn returns 0 whether POSIX_SPAWN_USEVFORK is set or not.
> When POSIX_SPAWN_USEVFORK is not set posix_spawn returns 0 and errno is 0.
> When POSIX_SPAWN_USEVFORK is set posix_spawn returns 0 and errno is ENOEXEC.


Hi Dmitry,

If the function is returning 0, there's no error, and errno shouldn't be read however, posix_spawn(3) is a bit special on that, and it may still fail with a return code of 0; continue reading).

I guess you maybe didn't state it here as obvious, but to me it's not by reading the conversation: Apart from posix_spawn(3) returning 0 and setting errno to ENOEXEC, I guess you're seeing something else, such as a child returning 127?

If so, I think I understand the problem you're trying to explain.

Otherwise, glibc is allowed to set errno without failing, and you shouldn't be reading errno as it's meaningless.

posix_spawn(3):
RETURN VALUE
       Upon    successful    completion,    posix_spawn()    and
       posix_spawnp() place the PID of the child process in pid,
       and  return  0.   If  there is an error during the fork()
       step, then no child is created, the contents of *pid  are
       unspecified,  and  these functions return an error number
       as described below.

       Even when these functions return a  success  status,  the
       child  process  may  still fail for a plethora of reasons
       related to its pre-exec() initialization.   In  addition,
       the  exec(3)  may fail.  In all of these cases, the child
       process will exit with the exit value of 127.



Cheers,
Alex

Alejandro Colomar <alx>
Wed 21 Sep 2022 01:57:11 AM UTC, comment #13: 

There is nothing to be sorry about, Martin. Thank you for your work, keep contributing.

Dmitry Goncharov <dgoncharov>
Wed 21 Sep 2022 01:11:42 AM UTC, comment #12: 

Ah, so the cause was as noted in https://savannah.gnu.org/bugs/?57022#comment5:

> I leaked a copy of the filename because I'm really a C++ programmer, whose string literals are const.


Thanks for sorting it out, Dmitry, and sorry for the considerable trouble.  Who'd have thought we'd have leak detection before const-correctness?  Well, with hindsight, it's obvious that posix_spawn's prototype can't change, while tool support can move as fast as it likes.

Martin Dorey <mdorey>
Wed 21 Sep 2022 12:56:29 AM UTC, comment #11: 

Patch 3 tested on glibc-2.17, glibc-2.32 and sun os.

Dmitry Goncharov <dgoncharov>
Wed 21 Sep 2022 12:55:27 AM UTC, comment #10: 

In glibc-2.17 posix_spawn returns 0 whether POSIX_SPAWN_USEVFORK is set or not.
When POSIX_SPAWN_USEVFORK is not set posix_spawn returns 0 and errno is 0.
When POSIX_SPAWN_USEVFORK is set posix_spawn returns 0 and errno is ENOEXEC.

However, today i realized one aspect that we overlooked. There is this configure check which is supposed to disable posix_spawn.
Why was posix_spawn enabled on this box?

This is the configure check for posix_spawn.


     AC_RUN_IFELSE([AC_LANG_SOURCE([[
       #include <spawn.h>
       #include <string.h>

       extern char **environ;

       int main() {
         char* path = strdup("./non-existent");
         char *argv[[2]];
         argv[[0]] = path;
         argv[[1]] =  0;
         return posix_spawn(0, path, 0, 0, argv, environ);
       }]])],
       [make_cv_synchronous_posix_spawn=no],
       [make_cv_synchronous_posix_spawn=yes],
       [make_cv_synchronous_posix_spawn="no (cross-compiling)"])]))


This check contains a memory leak of string "./non-existent". i specify -fsanitize=leak and this leak causes the test to fail.
when the test fails configure mistakenly treats this failure as a posix_spawn return code being non zero and enables posix_spawn.

We can fix this configure check and keep job.c intact.

Dmitry Goncharov <dgoncharov>
Tue 20 Sep 2022 06:29:39 AM UTC, comment #9: 

Sorry Dmitry I'm still really confused.  Maybe it would help if you made more clear what you discovered.

GNU make always sets POSIX_SPAWN_USEVFORK if it exists, and it does exist in all versions of glibc we are discussing.

You have discovered that in glibc 2.17, posix_spawn() has a bug in its behavior, with POSIX_SPAWN_USEVFORK set (since we've determined it is set everywhere).

In glibc >=2.24, posix_spawn() does not have this bug with POSIX_SPAWN_USEVFORK set (and, presumably, without it set either although I didn't check this).

The question is, does posix_spawn() in glibc <2.24 (e.g., 2.17) and WITHOUT POSIX_SPAWN_USEVFORK set also show this bug, or does it work properly without vfork?

If older glibc works if you don't set POSIX_SPAWN_USEVFORK, then I suggest that the thing to do is to detect whether this old version of glibc is being used and if it is, then we don't use POSIX_SPAWN_USEVFORK regardless of whether it exists or not, and just use normal posix_spawn().

If older glibc doesn't work even if you do use POSIX_SPAWN_USEVFORK, then I'm not sure why we're even testing for this since it doesn't seem to make any difference either way.

Am I missing something?

Paul D. Smith <psmith>
Group administrator
Sun 18 Sep 2022 11:32:54 PM UTC, comment #8: 

However, maybe we should also define _GNU_SOURCE in the configure check before including #include <spawn.h>.
Something like


index ec8b4c13..38418142 100644
--- a/configure.ac
+++ b/configure.ac
@@ -369,6 +369,7 @@ AS_IF([test "$make_cv_posix_spawn" = yes],
     [make_cv_synchronous_posix_spawn],
     [make_cv_synchronous_posix_spawn=no
      AC_RUN_IFELSE([AC_LANG_SOURCE([[
+       #define _GNU_SOURCE 1
        #include <spawn.h>
        #include <string.h>


Dmitry Goncharov <dgoncharov>
Sun 18 Sep 2022 11:23:23 PM UTC, comment #7: 

Let me describe differently.

posix_spawn calls either vfork or clone, depending on arguments being passed. See the man page referenced above.
I observed that when posix_spawn calls vfork, that's is when POSIX_SPAWN_USEVFORK is set in flags, errno is set to ENOEXEC. This allows the patch to work.
When posix_spawn calls clone, that is POSIX_SPAWN_USEVFORK is not set in flags, errno it not set. In this situation the patch does not help.

In other words, make wants to set POSIX_SPAWN_USEVFORK in order to let the patch work.

The change in configure.ac in case there is such a even older version of glibc where posix_spawn returns 0 and there is no POSIX_SPAWN_USEVFORK (i am not sure there is such a version, but if there is, it's definitely not 2.17). In that case the configure check disables posix_spawn.

Dmitry Goncharov <dgoncharov>
Sun 18 Sep 2022 11:05:16 PM UTC, comment #6: 

I'm not sure I understand the change being made.  POSIX_SPAWN_USEVFORK is definitely defined in glibc 2.17: I'm looking at the source for that release right now and see it.

It's set if __USE_GNU is set:

/* Flags to be set in the `posix_spawnattr_t'.  */
#define POSIX_SPAWN_RESETIDS            0x01
#define POSIX_SPAWN_SETPGROUP           0x02
#define POSIX_SPAWN_SETSIGDEF           0x04
#define POSIX_SPAWN_SETSIGMASK          0x08
#define POSIX_SPAWN_SETSCHEDPARAM       0x10
#define POSIX_SPAWN_SETSCHEDULER        0x20
#ifdef __USE_GNU
# define POSIX_SPAWN_USEVFORK           0x40
#endif


This is almost identical to the current version (it adds a new extra flag as well).

I think the only way to make this work is not supply USEVFORK if we are building with glibc less than 2.24...?

Paul D. Smith <psmith>
Group administrator
Sun 18 Sep 2022 07:20:51 PM UTC, comment #5: 

posix_spawn calls either vfork of spawn.
On glibc-2.17 i observed that when posix_spawn calls vfork the return code is zero, but errno is set to ENOEXEC.
When posix_spawn calls spawn, both return code and errno are zero.

This man page
https://man7.org/linux/man-pages/man3/posix_spawn.3.html
describes when vfork is called.
This is a quote


       Before glibc 2.24, the child process is created using vfork(2)
       instead of fork(2) when either of the following is true:

       *  the spawn-flags element of the attributes object pointed to by
          attrp contains the GNU-specific flag POSIX_SPAWN_USEVFORK; or

       *  file_actions is NULL and the spawn-flags element of the
          attributes object pointed to by attrp does not contain
          POSIX_SPAWN_SETSIGMASK, POSIX_SPAWN_SETSIGDEF,
          POSIX_SPAWN_SETSCHEDPARAM, POSIX_SPAWN_SETSCHEDULER,
          POSIX_SPAWN_SETPGROUP, or POSIX_SPAWN_RESETIDS.


Make specifies POSIX_SPAWN_USEVFORK, when avaiable, and this allows the patch to help.
The patch cannot not help some version of glibc which lacks POSIX_SPAWN_USEVFORK and returns 0 and keeps errno intact.
So, i updated the patch to check for __GLIBC__ at configure time and disable posix_spawn, unless POSIX_SPAWN_USEVFORK is available. This check should not affect modern versions of glibc, they have POSIX_SPAWN_USEVFORK.
Tested on glibc-2.17, glibc-2.32 and sun os.

Dmitry Goncharov <dgoncharov>
Sat 17 Sep 2022 11:18:28 PM UTC, comment #4: 


> it's not feasible to test the runtime behavior of posix_spawn() in configure

Despite it being "problematic", isn't that how we solved the not entirely dissimilar bug #57022 (Error 127 executing a script with no #!) in http://git.savannah.gnu.org/cgit/make.git/commit/?id=e64674b718600bd4fad0b5ac1cc37eb89fba5ef7?

> posix_spawn() was completely rewritten in glibc 2.24

That's the version of glibc that I reckoned, in yon bug, fixed:

https://sourceware.org/bugzilla/show_bug.cgi?id=18433
posix_spawn does not return correctly upon failure to execute

Martin Dorey <mdorey>
Sat 17 Sep 2022 04:52:56 PM UTC, comment #3: 

This is strange.  I looked in the glibc bug tracker and couldn't find anything about this issue.  I also looked at the actual implementation of glibc 2.17 and it doesn't seem to me like this should happen (although I'm not a glibc developer so I might have missed something).  I was looking at the version for x86_64 Linux kernel though: Dmitry can you give more info about what system you were testing on that had this issue?

The patch does clear errno before invoking posix_spawn() so it's LIKELY that if errno is ENOEXEC after return then we had this issue, but it is true that the value of errno is not determinate if the function returns success.

Unfortunately due to cross-compilation etc. it's not feasible to test the runtime behavior of posix_spawn() in configure and even if it were, that would only be relevant for the version of glibc used at compile time, which could well not be the same version used at runtime.  So a configure check is problematic at best.

I looked through the history of glibc and saw that posix_spawn() was completely rewritten in glibc 2.24.  But, the issue might not be with posix_spawn() at all, it could be in fork() or vfork().

Dmitry can you modify the code in src/job.c to remove the POSIX_SPAWN_USEVFORK flag and see if you still see the problem with glibc 2.17?  Maybe that will help narrow down where the problem actually is.

Paul D. Smith <psmith>
Group administrator
Sat 17 Sep 2022 05:11:38 AM UTC, comment #2: 

No, if posix_spawn() returns zero then then errno's value is unspecified and any checks against it are invalid resulting in false positives.If some versions of glibc have posix_spawn() return 0 when they did not create the target process then the configure script needs to check for that and not use that broken implementation.  Otherwise, the result will be double invocations on correct systems.

NEVER test errno after a function returned success!

Anonymous
Sat 17 Sep 2022 01:24:18 AM UTC, comment #1: 

posix_spawn fails to run a child process.

Some versions of glibc (i observed this with glibc-2.17) fail to spawn a shell
program missing a shbang and return 0 from posix_spawn.


$ ls
hello.sh  makefile
$ cat hello.sh
printf "hello, world\n"
$ cat makefile
all:; ./hello.sh
$ # this is make built from master
$ ~/src/make/m64/make
./hello.sh
make: *** [makefile:1: all] Error 127
$ # this is make with the patch applied
$ ~/src/make/l64/make
./hello.sh
hello, world



Even though posix_spawn returns 0 in this situation, it still sets errno to
ENOEXEC.
This patch causes child_execute_job to compare errno against ENOEXEC after
posix_spawn returned 0.

Dmitry Goncharov <dgoncharov>
Sat 17 Sep 2022 01:23:19 AM UTC, original submission:  

.

Dmitry Goncharov <dgoncharov>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #53736:  sv63070_fix3.diff added by dgoncharov (1KiB - text/x-patch)
file #53712:  sv63070_fix2.diff added by dgoncharov (2KiB - text/x-patch)
file #53705:  sv63070_fix.diff added by dgoncharov (1KiB - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by alx (Posted a comment)
  • -email is unavailable- added by mdorey (Posted a comment)
  • -email is unavailable- added by psmith (Posted a comment)
  • -email is unavailable- added by dgoncharov (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

    Follow 7 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2022-09-25 psmith StatusNone Fixed
        Open/ClosedOpen Closed
        Fixed ReleaseNone 4.4
        Triage StatusNone Small Effort
    2022-09-21 dgoncharov Attached File- Added sv63070_fix3.diff, #53736
    2022-09-18 dgoncharov Attached File- Added sv63070_fix2.diff, #53712
    2022-09-17 dgoncharov Attached File- Added sv63070_fix.diff, #53705

    Back to the top

    Powered by Savane 3.13-d3ae.
    Corresponding source code