Failure to launch external commands due to race condition in 'posix_spawn' usage #79

aweeraman · 2020-07-17T21:24:25Z

There has been a bug report of an exec format error when executing "cat TODO | while read line; do ls; done".

Ref:

I was able to reproduce this issue on 93u+m 2020-07-14 and it appears to be an issue at least since 93u+ 2012-08-01.

$ cat TODO
1
2
$ cat TODO | while read line; do ls; done
ls: ls: cannot execute [Exec format error]
ls: ls: cannot execute [Exec format error]

Tagging @chrisbertin and @mirabilos - reporters of the issue.

The text was updated successfully, but these errors were encountered:

posguy99 · 2020-07-17T21:32:00Z

Can't reproduce it here on macOS 10.14.6..

[02:28 PM][ttys001][~/ksh-test]
[34] iMac $ echo $KSH_VERSION
Version AJM 93u+m 2020-07-16
[02:29 PM][ttys001][~/ksh-test]
[35] iMac $ cat TODO
1
2
[02:29 PM][ttys001][~/ksh-test]
[36] iMac $ cat TODO | while read line; do ls; done
TODO
TODO
[02:29 PM][ttys001][~/ksh-test]
[37] iMac $ /bin/ksh
[02:29 PM][ttys001][~/ksh-test]
[38] iMac $ echo $KSH_VERSION
Version AJM 93u+ 2012-08-01
[02:29 PM][ttys001][~/ksh-test]
[39] iMac $ cat TODO
1
2
[02:29 PM][ttys001][~/ksh-test]
[40] iMac $ cat TODO | while read line; do ls; done
TODO
TODO
[02:29 PM][ttys001][~/ksh-test]
[41] iMac $

The slightly different test in the Ubuntu bug doesn't fail either.

mirabilos · 2020-07-17T21:43:56Z

Marc Wilson dixit:

Can't reproduce it here on macOS 10.14.6..

According to the reporter, they get a working ksh93 if they disable posix_spawn configure option so it may be a, very widespread, bug in GNU libc (Linux/kFreeBSD/Hurd-only, I presume).

JohnoKing · 2020-07-17T22:41:31Z

According to the reporter, they get a working ksh93 if they disable posix_spawn configure option so it may be a, very widespread, bug in GNU libc (Linux/kFreeBSD/Hurd-only, I presume).

I can confirm that disabling posix_spawn causes this bug to go away. OpenSUSE disables it in their build of ksh with a patch titled ksh-qemu.patch, which implies there is another bug related to posix_spawn that affects QEMU.

McDutchie · 2020-07-17T22:55:03Z

On macOS, the compiler flags used by the Apple version (which I've incorporated into src/cmd/INIT/cc.darwin) disable SHOPT_SPAWN. I can confirm that, if I change that flag to -DSHOPT_SPAWN=1 and recompile from scratch, I can reproduce this bug on the Mac.

I can also reproduce this bug on FreeBSD, on which SHOPT_SPAWN is not disabled by default.

Neither of these two systems use GNU libc, so it seems likely that this is a bug in ksh and not in any C library. Does anyone here have an idea how to fix it?

Alternatively, I wonder if there would be any disadvantage to simply removing the use of posix_spawn altogether.

McDutchie · 2020-07-17T23:02:45Z

Minimal reproducer for regression test:

printf '%s\n' 1 2 3 | while read line; do ls /dev/null; done

Normal output:

/dev/null
/dev/null
/dev/null

Bug output:

/dev/null
ls: ls: cannot execute [Exec format error]
ls: ls: cannot execute [Exec format error]

McDutchie · 2020-07-18T00:50:58Z

Well, I think the advantage of using posix_spawn(2) is made clear by the following:

$ arch/*/bin/ksh -c 'time for ((i=1; i<=10000; i++)); do /usr/bin/true; done'

real	0m22,02s
user	0m07,42s
sys	0m11,15s
$ ./ksh_SHOPT_SPAWN -c 'time for ((i=1; i<=10000; i++)); do /usr/bin/true; done'

real	0m13,66s
user	0m04,43s
sys	0m07,66s

So it would be really nice if someone could figure out how to fix ksh's use of posix_spawn.

chrisbertin · 2020-07-18T01:04:12Z

A few observations: If input is redirected, instead of piped, no errors. The invalid behavior is not 100% consistent. Running a failing command in a sub-shell works (which is the workaround I have been using). Using built-in commands work. sixt $ cat > /tmp/list 1 2 3 sixt $ while read f; do ls -ld /; done < /tmp/xx drwxr-xr-x 30 root root 4096 Jul 14 08:08 / drwxr-xr-x 30 root root 4096 Jul 14 08:08 / drwxr-xr-x 30 root root 4096 Jul 14 08:08 / sixt $ cat /tmp/list | while read f; do ls -ld /; done drwxr-xr-x 30 root root 4096 Jul 14 08:08 / /bin/ls: /bin/ls: cannot execute [Exec format error] /bin/ls: /bin/ls: cannot execute [Exec format error] sixt $ cat /tmp/list | while read f; do ls -ld /; done /bin/ls: /bin/ls: cannot execute [Exec format error] /bin/ls: /bin/ls: cannot execute [Exec format error] /bin/ls: /bin/ls: cannot execute [Exec format error] sixt $ ksh -c 'cat /tmp/xx | while read f; do ls -ld /; done' drwxr-xr-x 30 root root 4096 Jul 14 08:08 / drwxr-xr-x 30 root root 4096 Jul 14 08:08 / drwxr-xr-x 30 root root 4096 Jul 14 08:08 / sixt $ cat /tmp/list | while read f; do echo $f /; done 1 / 2 / 3 / sixt $ Chris (chris.bertin@gmail.com or chris.bertin@protonmail.com)

…

On Fri, Jul 17, 2020 at 2:24 PM Anuradha Weeraman ***@***.***> wrote: There has been a bug report of an exec format error when executing "cat TODO | while read line; do ls; done". Ref: - https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=965072 - https://bugs.launchpad.net/ubuntu/+source/ksh/+bug/1887863 I was able to reproduce this issue on 93u+m 2020-07-14 and it appears to be an issue at least since 93u+ 2012-08-01. $ cat TODO 1 2 $ cat TODO | while read line; do ls; done ls: ls: cannot execute [Exec format error] ls: ls: cannot execute [Exec format error] Tagging @chrisbertin <https://github.com/chrisbertin> and @mirabilos <https://github.com/mirabilos> - reporters of the issue. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#79>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AQKGJUJOFTOVXOBFI6ZRDYDR4C6RPANCNFSM4O65E6KA> .

posguy99 · 2020-07-18T02:55:31Z

Is this 2020 bug relevant?

att#470
which leads to
att#468

and the original ast ML discussion:
https://www.mail-archive.com/ast-developers@research.att.com/msg00717.html

McDutchie · 2020-07-18T03:57:57Z

Thanks @posguy99, yes, that is very relevant indeed.

So that is another problem with using posix_spawn(2): it "doesn't have a way to set the terminal group before the exec()". That might be fine for many use cases, but clearly that's unacceptable behaviour for a shell!

The RATIONALE section of the POSIX spec for posix_spawn(2) also makes for interesting reading. Note that the spec title itself is marked "ADVANCED REALTIME", indicating that this is intended for real-time applications (which a shell very much is not, no matter how fast it is). Some quotes:

[…] although they may be an efficient replacement for many fork()/ exec pairs, their goal is to provide useful process creation primitives for systems that have difficulty with fork(), not to provide drop-in replacements for fork()/ exec.
[…] We can identify several problems with posix_spawn() and posix_spawnp(), but there does not appear to be a solution that introduces fewer problems. Environment modification for child process attributes not specifiable via the attrp or file_actions arguments must be done in the parent process, and since the parent generally wants to save its context, it is more costly than similar functionality with fork()/exec.
[…] The fork() function is a wonderfully powerful operation. We do not expect to duplicate its functionality in a simple, fast function with no special hardware requirements. It is worth noting that posix_spawn() and posix_spawnp() are very similar to the process creation operations on many operating systems that are not UNIX systems.

So, while running /usr/bin/true in a loop might be quite a bit faster, it looks clear that it's not worth the cost in bugs and undefined behaviour. And if you run a command that actually does something, that performance difference quickly approaches insignificance. So, this is another instance where the ksh2020 developers made the right call. We must get rid.

~~On the plus side, that makes fixing this bug quite simple.~~

posguy99 · 2020-07-18T03:59:59Z

Pointing out, tho, that if you follow those 2020 bugs all the way through, they lead you back to the performance degradation that was an large part of the original ksh2020 kerfuffle.

McDutchie · 2020-07-18T04:26:59Z

Agreed. That's not going to happen though.

JohnoKing · 2020-07-18T05:53:28Z

If posix_spawn is removed it shouldn't be done by removing the SHOPT_SPAWN ifdef; OpenSUSE's patch should be instead. Results from performance testing on my Linux machine (ksh is compiled with CCFLAGS=-O2):

# Normal build with posix_spawn
$ ./ksh-posix-spawn -c 'time for ((i=1; i!=10000; i++)); do /usr/bin/true; done'

real    0m02.03s
user    0m01.73s
sys 0m00.36s

# posix_spawn removed with OpenSUSE's patch
$ ./ksh-no-posix-spawn -c 'time for ((i=1; i!=10000; i++)); do /usr/bin/true; done'

real    0m01.85s
user    0m01.60s
sys 0m00.28s

# SHOPT_SPAWN ifdef removed with unifdef(1)
$ ./ksh-no-shopt-spawn -c 'time for ((i=1; i!=10000; i++)); do /usr/bin/true; done'

real    0m03.15s
user    0m02.37s
sys 0m00.89s

Relevant strace output from ksh -c '/usr/bin/true; /usr/bin/true':

#  Normal build with posix_spawn
getpid()                                = 4296
mmap(NULL, 36864, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS|MAP_STACK, -1, 0) = 0x7fd12c55f000
rt_sigprocmask(SIG_BLOCK, ~[], [], 8)   = 0 
clone(child_stack=0x7fd12c567ff0, flags=CLONE_VM|CLONE_VFORK|SIGCHLD) = 4297
munmap(0x7fd12c55f000, 36864)           = 0 
rt_sigprocmask(SIG_SETMASK, [], NULL, 8) = 0 rt_sigaction(SIGINT, {sa_handler=0x55f8f72bbfd0, sa_mask=[], sa_flags=SA_RESTORER|SA_INTERRUPT, sa_restorer=0x7fd12c39d3e0}, {sa_handler=SIG_DFL, sa_mask=[], sa_flags=0}, 8) = 0

# posix_spawn removed with OpenSUSE's patch
getpid()                                = 4115
rt_sigprocmask(SIG_BLOCK, [HUP INT QUIT PIPE CHLD], [], 8) = 0
vfork()                                 = 4116
rt_sigprocmask(SIG_SETMASK, [], NULL, 8) = 0
rt_sigaction(SIGINT, {sa_handler=0x56194d18b9f0, sa_mask=[], sa_flags=SA_RESTORER|SA_INTERRUPT, sa_restorer=0x7fb7625d73e0}, {sa_handler=SIG_DFL, sa_mask=[], sa_flags=0}, 8) = 0

# SHOPT_SPAWN ifdef removed with unifdef(1)
stat(".", {st_mode=S_IFDIR|0755, st_size=130, ...}) = 0
rt_sigaction(SIGCHLD, {sa_handler=0x55e765fe75a0, sa_mask=[], sa_flags=SA_RESTORER|SA_INTERRUPT, sa_restorer=0x7f9afb8523e0}, {sa_handler=SIG_DFL, sa_mask=[], sa_flags=0}, 8) = 0
clone(child_stack=NULL, flags=CLONE_CHILD_CLEARTID|CLONE_CHILD_SETTID|SIGCHLD, child_tidptr=0x7f9afb814e50) = 12221
rt_sigaction(SIGINT, {sa_handler=0x55e765fd9a40, sa_mask=[], sa_flags=SA_RESTORER|SA_INTERRUPT, sa_restorer=0x7f9afb8523e0}, {sa_handler=SIG_DFL, sa_mask=[], sa_flags=0}, 8) = 0

The OpenSUSE patch causes vfork to be used instead of posix_spawn which appears to increase performance slightly while still fixing the exec format error. Removing SHOPT_SPAWN causes the OS implementation of fork to be used instead of vfork and posix_spawn, which is slower (on Linux fork uses clone, but that doesn't make up for the performance deficit).

McDutchie · 2020-07-18T13:05:29Z

@JohnoKing: Sounds good to me, but I am having trouble locating the OpenSUSE patch you are referring to. (Their patches are mostly undocumented and their bug tracker is closed to the public.) ~~Could you do a PR that implements that fix?~~

The only reference to vfork I can find in their patchset is ksh93-disable-vfork.diff which has the ksh.changes entry "disable vfork, use fork instead".

JohnoKing · 2020-07-18T13:08:44Z

~~My first post in this thread linked to the patch; it seems you missed it.~~ Here is the full link:
https://build.opensuse.org/package/view_file/shells/ksh/ksh-qemu.patch?expand=1

hyenias · 2020-07-18T13:24:18Z

Just to be safe and thorough when using vfork, have you all considered vfork's NOTES section and especially the content under Caveats?

JohnoKing · 2020-07-18T14:03:18Z

Some notes regarding ksh usage of vfork:

The libast spawnveg function uses the shared memory of vfork to its advantage. There is an iffe feature test for detecting this part of vfork behavior:

ksh/src/lib/libast/features/lib

Lines 228 to 241 in 37a9c34

    
           tst	real_vfork note{ vfork child shares data with parent }end execute{ 
        
           	_BEGIN_EXTERNS_ 
        
           	extern int	_exit _ARG_((int)); 
        
           	extern int	vfork _ARG_((void)); 
        
           	_END_EXTERNS_ 
        
           	int		code; 
        
           	int 
        
           	main() 
        
           	{ 
        
           		code = 1; 
        
           		if (!vfork()) 
        
           			code = 0; 
        
           		_exit(code); 
        
           	}

The sigcritical function has a comment about vfork, which is related to the first caveat in the vfork man page.:

ksh/src/lib/libast/misc/sigcrit.c

Lines 167 to 173 in 37a9c34

    
           		/* 
        
           		 * a vfork() may have intervened so we 
        
           		 * allow apparent nesting mismatches 
        
           		 */ 
        
           		if (--level <= 0) 
        
           		{

The multi-threading caveat in the vfork man page isn't relevant because ksh is a single-threaded program. There are multi-threaded ksh plugins, but those are incompatible with vmalloc, which ksh uses instead of standard malloc (see serious performance breakdown of ksh2020 in comparison to ksh93u+ att/ast#1449 (comment)).

McDutchie · 2020-07-18T14:47:49Z

Right, that all makes sense now, thanks for the explanations. So OpenSUSE's ksh-qemu.patch causes ksh to fall back to using vfork by removing the feature test for posix_spawn, so #if _lib_posix_spawn always yields false.

But it also removes another feature test (lib_poll_fd_2). The _lib_poll_fd_2 result is used just once, in sfhdr.h, to decide whether poll(2) can be used:

ksh/src/lib/libast/sfio/sfhdr.h

Lines 438 to 448 in 37a9c34

    
           /* functions for polling readiness of streams */ 
        
           #if _lib_select 
        
           #undef _lib_poll 
        
           #if _sys_select 
        
           #include	<sys/select.h> 
        
           #endif 
        
           #else 
        
           #if _lib_poll_fd_1 || _lib_poll_fd_2 
        
           #define _lib_poll	1 
        
           #endif 
        
           #endif /*_lib_select_*/

I don't understand how that is related to posix_spawn, so I'm thinking we should perhaps keep that test. Am I still missing something?

JohnoKing · 2020-07-18T15:05:35Z

The lib_poll_fd_2 feature test doesn't need to be removed to fix the posix_spawn related bugs. That change was likely made to fix something specific to QEMU.

hyenias · 2020-07-18T15:13:26Z

@JohnoKing: Thank you for your review of the vfork caveats. Would it be possible to introduce the vfork implementation so one could selectively compile using vfork, posix_spawn, or fork [something like SHOPT_SPAWN]? I think having the ability to alter the way ksh creates a child process may assist in confirming its use and/or troubleshooting in the future. It also might be helpful to have some feature flag identifying it.

JohnoKing · 2020-07-18T15:26:29Z

That is definitely doable. I imagine the ifdefs would look like this:

/* SHOPT_SPAWN is used for spawnveg, which can call
   posix_spawn, vfork or fork depending on compile 
   time options. As a result the posix_spawn ifdef is
   SHOPT_POSIX_SPAWN since SHOPT_SPAWN needs to
   be defined for any of this to have much effect. */
#if !defined(SHOPT_POSIX_SPAWN)
#undef _lib_posix_spawn
#endif
#if defined(SHOPT_VFORK) && defined(_lib_vfork)
#undef _lib_posix_spawn
#endif
#if defined(SHOPT_FORK) && defined(_lib_fork)
#undef _lib_vfork
#undef _real_vfork
#undef _lib_posix_spawn
#endif
/* On Linux syscall(2) can be used to get the real fork system call.
   By default fork is a wrapper that calls clone(2). */
#if defined(SHOPT_REAL_FORK) && defined(__linux__)
#undef _lib_vfork
#undef _real_vfork
#undef _lib_posix_spawn
#undef fork
#include <sys/syscall.h>
#define fork syscall(SYS_fork)
#endif

McDutchie · 2020-07-18T15:32:25Z

I'm not in favour of that. It is now clear that ksh's use of posix_spawn creates inherent race conditions. One such impossible-to-fix race condition was acknowledged by David Korn in 2011 (the "solution" he proposed in that message is no solution at all; we cannot expect people to recompile programs to work around ksh bugs). And another such race condition was reported right here as a bug. Sorry, but I'm against keeping code that is inherently broken.

McDutchie · 2020-07-18T15:34:00Z

Here are some new tests on my Mac laptop.

With ksh's racy use of posix_spawn:

$ ./ksh_POSIX_SPAWN
$ printf "%s\n" 1 2 3 | while read line; do ls /dev/null; done
/dev/null
ls: ls: cannot execute [Exec format error]
ls: ls: cannot execute [Exec format error]
$ time for ((i=1; i!=10000; i++)); do /usr/bin/true; done

real	0m13,84s
user	0m04,48s
sys	0m08,30s

With ksh's vfork fallback, after applying the patch based on OpenSUSE's:

$ ./ksh_VFORK
$ printf "%s\n" 1 2 3 | while read line; do ls /dev/null; done
/dev/null
/dev/null
/dev/null
$ time for ((i=1; i!=10000; i++)); do /usr/bin/true; done

real	0m13,97s
user	0m04,54s
sys	0m08,39s

…and, for reference, the classic fork/exec that ksh2020 reverted to:

$ ./ksh_FORK
$ printf "%s\n" 1 2 3 | while read line; do ls /dev/null; done
/dev/null
/dev/null
/dev/null
$ time for ((i=1; i!=10000; i++)); do /usr/bin/true; done

real	0m21,76s
user	0m07,23s
sys	0m11,65s

@JohnoKing's Linux tests and my own Mac tests (above) suggest that vfork performs just as well as posix_spawn (if there is any difference, it's lost in the margin of error), and it doesn't have these problems. So in terms of performance, this is not going to be a significant loss at all.

hyenias · 2020-07-18T15:41:57Z

My suggestion with providing possible compiling options to use one of the 3 methods was not based on performance but troubleshooting. So, if not in favor of having a posix_spawn option; would it still be possible to have a fork option? Or is that already done by using SHOPT_SPAWN?

McDutchie · 2020-07-18T16:00:03Z

@hyenias, yes – disabling SHOPT_SPAWN (by adding -DSHOPT_SPAWN=0 to the compiler option) will still revert to the classic fork/exec mechanism, just as it does now. The patch causes the AST spawnveg() function to use vfork, but disabling SHOPT_SPAWN causes spawnveg() to not be used at all.

That's what Apple has been doing all along for their /bin/ksh, which is how I know this. I found I had to remove -DSHOPT_SPAWN=0 as a default compiler option from cc.darwin in order for vfork to be used.

chrisbertin · 2020-07-18T16:06:02Z

vfork() is a pre-VM, or, at least pre-COW, semi-kludge and I thought it would have been retired by now... IMHO... Chris (chris.bertin@gmail.com or chris.bertin@protonmail.com)

…

On Sat, Jul 18, 2020 at 9:00 AM Martijn Dekker ***@***.***> wrote: @hyenias <https://github.com/hyenias>, yes – disabling SHOPT_SPAWN (by adding -DSHOPT_SPAWN=0 to the compiler option) will still revert to the classic fork/exec mechanism, just as it does now. The patch causes the AST spawnveg() function to use vfork, but disabling SHOPT_SPAWN causes spawnveg() to not be used at all. That's what Apple has been doing all along for their /bin/ksh, which is how I know this. I found I had to remove -DSHOPT_SPAWN=0 as a default compiler option from cc.darwin in order for vfork to be used. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#79 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AQKGJUO23JAMHYWJYJGFRIDR4HBJDANCNFSM4O65E6KA> .

McDutchie · 2020-07-18T17:03:20Z

Well, for ksh, it clearly works more correctly than posix_spawn() (as ksh uses that for purposes it was not intended for, which inherently creates race conditions) while being significantly faster than fork(). So, given available evidence, I believe we should keep that. If there are any bugs with using it, hopefully they'll show up in pre-release testing.

It's true that POSIX no longer specifies it. But if systems remove vfork() in future, then iffe should stop detecting it, causing ksh to fall back to fork().

JohnoKing · 2020-07-21T21:14:46Z

This is the uname output I get:

$ uname -vm
FreeBSD 12.1-RELEASE-p7 GENERIC  amd64

I have FreeBSD running on an old Compaq Presario CQ60-211DX (which runs a single core Intel Celeron) and in a virtual machine running under VirtualBox, with two CPU cores given to the vm (VirtualBox is running on a desktop with an AMD Ryzen 2600 overclocked to 4.05GHz on all six cores, SMT is enabled).

McDutchie · 2020-07-21T21:21:25Z

Update: After letting it run for a long time, I was able to reproduce the race condition on FreeBSD. It occurred in one out of 1413150 output lines.

McDutchie · 2020-07-22T03:55:09Z

@JohnoKing, first a question: I've sent you a test email, did you receive it? I think there may be a delivery problem.

Second, something occurred to me. The original bug report above is only reproducible on an interactive shell. So I decided to test for the testprocessgroup.c race condition on FreeBSD from a script. And I can't reproduce it. Can you confirm the race condition cannot be reproduced if the test loop is run from a script?

If so, that opens the door to a workaround: make both the posix_spawn and the vfork version of spawnveg(3) fall back to fork if the shell is interactive. This should be acceptable to the community; scripts are where speed really matters.

JohnoKing · 2020-07-22T04:31:00Z

@JohnoKing, first a question: I've sent you a test email, did you receive it? I think there may be a delivery problem.

I did receive the email. I also sent a reply.

Second, something occurred to me. The original bug report above is only reproducible on an interactive shell. So I decided to test for the testprocessgroup.c race condition on FreeBSD from a script. And I can't reproduce it. Can you confirm the race condition cannot be reproduced if the test loop is run from a script?

I can confirm that the race condition is not reproducible in scripts with either posix_spawn or vfork; it only occurs in interactive shells (tested on Linux and FreeBSD). This means we can keep using posix_spawn, but only in scripts.

McDutchie · 2020-07-22T06:02:42Z

More specifically, the culprit turns out to be job control (the m/-o monitor shell option). If you turn that off in an interactive shell, both of these bugs also vanish, in my tests at least.

Conversely, if you set -m in a script, then the bug originally reported here returns, but with a different symptom: instead of an exec format error, the script is suspended on each affected external command invocation, as if you press Ctrl+Z.

McDutchie · 2020-07-22T06:51:19Z

So, after this slightly desperate megathread, could the fix really be this trivial? Please test…

diff --git a/src/cmd/ksh93/sh/xec.c b/src/cmd/ksh93/sh/xec.c
index 4f99919..0991406 100644
--- a/src/cmd/ksh93/sh/xec.c
+++ b/src/cmd/ksh93/sh/xec.c
@@ -1587,7 +1587,7 @@ int sh_exec(register const Shnode_t *t, int flags)
 #else
 #if SHOPT_SPAWN
 #   ifdef _lib_fork
-                               if(com)
+                               if(com && !job.jobcontrol)
                                        parent = sh_ntfork(shp,t,com,&jobid,ntflag);
                                else
                                        parent = sh_fork(shp,type,&jobid);

JohnoKing · 2020-07-22T08:37:09Z

That patch is an effective fix for the race conditions, at least from my own testing.

McDutchie · 2020-07-22T10:25:10Z

I think it makes sense now. If job control is not active, the terminal process group ID never needs to be set; child processes simply inherit the terminal group from the parent.

However, with job control active, each child process spawned from the main shell environment (not subshells) needs to be in its own terminal process group, or you can't separate the jobs from each other. But you can't do this with posix_spawn. Hence this bug.

So it's clear that this posix_spawn usage is incompatible with job control and that is why this is so trivial to fix/avoid.

What I don't understand is why it doesn't fail consistently.

aweeraman · 2020-07-22T15:18:02Z

This is brilliant, @McDutchie @JohnoKing et al. Thanks for your persistence!

@chrisbertin and @mirabilos - this will be included in the next upload fairly shortly.

McDutchie · 2020-07-23T11:29:27Z

@aweeraman, hold off a little – this fix exposes a variant of #86 that only occurs with job control active. I'm working out a good way to fix it.

ksh93#79

Currently there is no proper way to set the controlling terminal through posix_spawn() in race free manner [1]. This forces shell implementations to keep using fork()+exec() when launching background process groups, even when using posix_spawn() yields better performance. This patch adds a new GNU extension so the creating process can configure the creating process terminal group. This is done with a new flag, POSIX_SPAWN_TCSETPGROUP, along with two new attribute functions, posix_spawnattr_tcsetpgrp_np(), and posix_spawnattr_tcgetpgrp_np(). The function sets a new attribute, spawn-tcgroupfd, that references to the controlling terminal. The controlling terminal is set after the spawn-pgroup attribute, and uses the spawn-tcgroupfd along with current creating process group (so it is composable with POSIX_SPAWN_SETPGROUP). To create a process and set the controlling terminal, one can use the following sequence: posix_spawnattr_t attr; posix_spawnattr_init (&attr); posix_spawnattr_setflags (&attr, POSIX_SPAWN_TCSETPGROUP); posix_spawnattr_tcsetpgrp_np (&attr, tcfd); If the idea is also to create a new process groups: posix_spawnattr_t attr; posix_spawnattr_init (&attr); posix_spawnattr_setflags (&attr, POSIX_SPAWN_TCSETPGROUP | POSIX_SPAWN_SETPGROUP); posix_spawnattr_tcsetpgrp_np (&attr, tcfd); posix_spawnattr_setpgroup (&attr, 0); The controlling terminal file descriptor is ignored if the new flag is not set. This interface is slight different than the one provided by QNX [2], which it only provides the POSIX_SPAWN_TCSETPGROUP flag. The QNX documentation is not on how the controlling terminal is obtained not how it iteracts with POSIX_SPAWN_SETPGROUP. Since a glibc implementation is library based, I moving the controlling terminal open to the caller should be more straighforward since it mimics the tcsetpgrp(), and allow less error handling by posix_spawn(). Checked on x86_64-linux-gnu and i686-linux-gnu. [1] ksh93/ksh#79 [2] https://www.qnx.com/developers/docs/7.0.0/index.html#com.qnx.doc.neutrino.lib_ref/topic/p/posix_spawn.html

posguy99 · 2021-12-26T22:21:42Z

macOS Monterey is apparently deprecating vfork() in favor of posix_spawn()?

textmate/textmate@ee585ca

https://bugs.openjdk.java.net/browse/JDK-8274293

https://www.reddit.com/r/emacs/comments/qp2sxf/anyone_with_macos_monterey_on_an_m1/

McDutchie · 2021-12-26T22:35:58Z

Thanks @posguy99, vfork() has been deprecated for a long time on some other systems, but it doesn't matter as ksh already uses posix_spawn() in preference to vfork(), so vfork() is only used as a fallback for old systems where posix_spawn() is not available.

JohnoKing · 2021-12-30T02:05:33Z

I'll note that there is currently a patch that adds a POSIX_SPAWN_TCSETPGROUP flag to glibc: https://patchwork.sourceware.org/project/glibc/patch/20211223173003.1037286-1-adhemerval.zanella@linaro.org

If that patch gets merged into glibc's upstream codebase then perhaps we could use that flag to safely avoid usage of fork() in interactive shells.

zatrazz · 2022-01-25T18:02:36Z

I'll note that there is currently a patch that adds a POSIX_SPAWN_TCSETPGROUP flag to glibc: https://patchwork.sourceware.org/project/glibc/patch/20211223173003.1037286-1-adhemerval.zanella@linaro.org

If that patch gets merged into glibc's upstream codebase then perhaps we could use that flag to safely avoid usage of fork() in interactive shells.

I just merge this patch [1] and it will be included on glibc 2.35.

[1] https://sourceware.org/git/?p=glibc.git;a=commit;h=342cc934a3bf74ac618e2318d738f22ac93257ba

Currently there is no proper way to set the controlling terminal through posix_spawn in race free manner [1]. This forces shell implementations to keep using fork+exec when launching background process groups, even when using posix_spawn yields better performance. This patch adds a new GNU extension so the creating process can configure the created process terminal group. This is done with a new flag, POSIX_SPAWN_TCSETPGROUP, along with two new attribute functions: posix_spawnattr_tcsetpgrp_np, and posix_spawnattr_tcgetpgrp_np. The function sets a new attribute, spawn-tcgroupfd, that references to the controlling terminal. The controlling terminal is set after the spawn-pgroup attribute, and uses the spawn-tcgroupfd along with current creating process group (so it is composable with POSIX_SPAWN_SETPGROUP). To create a process and set the controlling terminal, one can use the following sequence: posix_spawnattr_t attr; posix_spawnattr_init (&attr); posix_spawnattr_setflags (&attr, POSIX_SPAWN_TCSETPGROUP); posix_spawnattr_tcsetpgrp_np (&attr, tcfd); If the idea is also to create a new process groups: posix_spawnattr_t attr; posix_spawnattr_init (&attr); posix_spawnattr_setflags (&attr, POSIX_SPAWN_TCSETPGROUP | POSIX_SPAWN_SETPGROUP); posix_spawnattr_tcsetpgrp_np (&attr, tcfd); posix_spawnattr_setpgroup (&attr, 0); The controlling terminal file descriptor is ignored if the new flag is not set. This interface is slight different than the one provided by QNX [2], which only provides the POSIX_SPAWN_TCSETPGROUP flag. The QNX documentation does not specify how the controlling terminal is obtained nor how it iteracts with POSIX_SPAWN_SETPGROUP. Since a glibc implementation is library based, it is more straightforward and avoid requires additional file descriptor operations to request the caller to setup the controlling terminal file descriptor (and it also allows a bit less error handling by posix_spawn). Checked on x86_64-linux-gnu and i686-linux-gnu. [1] ksh93/ksh#79 [2] https://www.qnx.com/developers/docs/7.0.0/index.html#com.qnx.doc.neutrino.lib_ref/topic/p/posix_spawn.html Reviewed-by: Carlos O'Donell <carlos@redhat.com> Tested-by: Carlos O'Donell <carlos@redhat.com>

Note that this is still a WIP. Current TODOs: - Error out if posix_spawnattr_tcsetpgrp_np is detected but posix_spawn is not. - Document the changes to spawnveg in the relevant man page. - Maybe: Use tcsetpgrp in the spawnveg fork fallback (if this is implemented, vfork will not be allowed to use that function since the behavior of tcsetpgrp after vfork is unpredictable and not portable). This commit implements support for the glibc POSIX_SPAWN_TCSETPGROUP and posix_spawnattr_tcsetpgrp_np extension [1]. This was done with the intention of improving performance in interactive shells without reintroducing previous race conditions[2][3]. These changes were tested with glibc commit 342cc934. src/cmd/ksh93/sh/path.c: - Tell spawnveg to set the terminal process group when launching a child process in an interactive shell. src/cmd/ksh93/sh/xec.c: - Allow posix_spawn when posix_spawnattr_tcsetpgrp_np is available. - Reimplement most of the sh_ntfork() SIGTSTP handling code removed in commit 66c3720. src/lib/libast/comp/spawnveg.c, src/lib/libast/misc/procopen.c, src/lib/libast/features/sys: - Add support for POSIX_SPAWN_TCSETPGROUP as implemented in glibc. This code (with a few changes) might also work on QNX, but I've avoided changing anything on that OS since I don't have a QNX system to test with. - Bugfix 1: If pgid == -1, use POSIX_SPAWN_SETSID when it's available (spawnveg uses setsid(2) after fork/vfork, so it should do the same thing when posix_spawn is used instead. src/lib/libast/features/lib: - Bugfix 2: Fix syntax errors in the posix_spawn feature test that caused ksh to fall back to vfork instead. - Detect the existence of posix_spawnattr_tcsetpgrp_np(). [1]: https://sourceware.org/git/?p=glibc.git;a=commit;h=342cc934a3bf74ac618e2318d738f22ac93257ba [2]: ksh93#79 [3]: https://www.mail-archive.com/ast-developers@research.att.com/msg00717.html

This commit implements support for the glibc POSIX_SPAWN_TCSETPGROUP and posix_spawnattr_tcsetpgrp_np extension [1]. This was done with the intention of improving performance in interactive shells without reintroducing previous race conditions[2][3]. These changes were tested with glibc commit 342cc934. src/cmd/ksh93/sh/path.c: - Tell spawnveg to set the terminal process group when launching a child process in an interactive shell. src/cmd/ksh93/sh/xec.c: - Allow usage of spawnveg when posix_spawnattr_tcsetpgrp_np is available. - Reimplement most of the sh_ntfork() SIGTSTP handling code removed in commit 66c3720. src/lib/libast/comp/spawnveg.c, src/lib/libast/misc/procopen.c, src/lib/libast/features/sys: - Add support for POSIX_SPAWN_TCSETPGROUP as implemented in glibc. This code (with a few changes) might also work on QNX, but I've avoided changing anything on that OS since I don't have a QNX system to test with. - Bugfix 1: If pgid == -1, use POSIX_SPAWN_SETSID when it's available (spawnveg uses setsid(2) after fork/vfork, so it should do the same thing when posix_spawn is used instead. src/lib/libast/man/spawnveg.3: - Document the changes to spawnveg(3) in the corresponding man page. Currently spawnveg will only use the new tcfd argument if posix_spawnattr_tcsetpgrp_np is supported. This could also be implemented for the fork fallback, but for now I've avoided changing that since actually using it in the fork code would likely require a lot of hackery to avoid attempting tcsetpgrp with vfork (the behavior of tcsetpgrp after vfork is not portable) and would only benefit systems that don't have posix_spawn and vfork (I can't recall any off the top of my head that would fall under that category). src/lib/libast/features/lib: - Bugfix 2: Fix syntax errors in the posix_spawn feature test that caused ksh to fall back to vfork instead. - Detect the existence of the GNU posix_spawnattr_tcsetpgrp_np() extension. [1]: https://sourceware.org/git/?p=glibc.git;a=commit;h=342cc934a3bf74ac618e2318d738f22ac93257ba [2]: ksh93#79 [3]: https://www.mail-archive.com/ast-developers@research.att.com/msg00717.html

This commit implements support for the glibc POSIX_SPAWN_TCSETPGROUP and posix_spawnattr_tcsetpgrp_np extensions[1]. This was done with the intention of improving performance in interactive shells without reintroducing previous race conditions[2][3]. These changes were tested with glibc commit 342cc934. src/cmd/ksh93/sh/path.c: - Tell spawnveg to set the terminal process group when launching a child process in an interactive shell. src/cmd/ksh93/sh/xec.c: - Allow usage of spawnveg when posix_spawnattr_tcsetpgrp_np is available. - Reimplement most of the sh_ntfork() SIGTSTP handling code removed in commit 66c3720. src/lib/libast/comp/spawnveg.c, src/lib/libast/misc/procopen.c, src/lib/libast/features/sys: - Add support for POSIX_SPAWN_TCSETPGROUP as implemented in glibc. This code (with a few changes) might also work on QNX, but I've avoided changing anything on that OS since I don't have a QNX system to test with. - Bugfix 1: If pgid == -1, use POSIX_SPAWN_SETSID when it's available (spawnveg uses setsid(2) after fork/vfork, so it should do the same thing when posix_spawn is used instead. src/lib/libast/man/spawnveg.3: - Document the changes to spawnveg(3) in the corresponding man page. Currently spawnveg will only use the new tcfd argument if posix_spawnattr_tcsetpgrp_np is supported. This could also be implemented for the fork fallback, but for now I've avoided changing that since actually using it in the fork code would likely require a lot of hackery to avoid attempting tcsetpgrp with vfork (the behavior of tcsetpgrp after vfork is not portable) and would only benefit systems that don't have posix_spawn and vfork (I can't recall any off the top of my head that would fall under that category). src/lib/libast/features/lib: - Bugfix 2: Fix syntax errors in the posix_spawn feature test that caused ksh to fall back to vfork instead. - Detect the existence of the GNU posix_spawnattr_tcsetpgrp_np() extension. [1]: https://sourceware.org/git/?p=glibc.git;a=commit;h=342cc934a3bf74ac618e2318d738f22ac93257ba [2]: ksh93#79 [3]: https://www.mail-archive.com/ast-developers@research.att.com/msg00717.html

This commit implements support for the glibc POSIX_SPAWN_TCSETPGROUP and posix_spawnattr_tcsetpgrp_np extensions[1]. This was done with the intention of improving performance in interactive shells without reintroducing previous race conditions[2][3]. These changes were tested with glibc commit 342cc934. src/cmd/ksh93/sh/path.c: - Tell spawnveg to set the terminal process group when launching a child process in an interactive shell. src/cmd/ksh93/sh/xec.c: - Allow usage of spawnveg when posix_spawnattr_tcsetpgrp_np is available. - Reimplement most of the sh_ntfork() SIGTSTP handling code removed in commit 66c3720. src/lib/libast/comp/spawnveg.c, src/lib/libast/misc/procopen.c, src/lib/libast/features/sys: - Add support for POSIX_SPAWN_TCSETPGROUP as implemented in glibc. This code (with a few changes) might also work on QNX, but I've avoided changing anything on that OS since I don't have a QNX system to test with. - Bugfix 1: If pgid == -1, use POSIX_SPAWN_SETSID when it's available (spawnveg uses setsid(2) after fork/vfork, so it should do the same thing when posix_spawn is used instead). src/lib/libast/man/spawnveg.3: - Document the changes to spawnveg(3) in the corresponding man page. Currently spawnveg will only use the new tcfd argument if posix_spawnattr_tcsetpgrp_np is supported. This could also be implemented for the fork fallback, but for now I've avoided changing that since actually using it in the fork code would likely require a lot of hackery to avoid attempting tcsetpgrp with vfork (the behavior of tcsetpgrp after vfork is not portable) and would only benefit systems that don't have posix_spawn and vfork (I can't recall any off the top of my head that would fall under that category). src/lib/libast/features/lib: - Bugfix 2: Fix syntax errors in the posix_spawn feature test that caused ksh to fall back to vfork instead. - Detect the existence of the GNU posix_spawnattr_tcsetpgrp_np() extension. [1]: https://sourceware.org/git/?p=glibc.git;a=commit;h=342cc934a3bf74ac618e2318d738f22ac93257ba [2]: ksh93#79 [3]: https://www.mail-archive.com/ast-developers@research.att.com/msg00717.html

This commit implements support for the glibc POSIX_SPAWN_TCSETPGROUP and posix_spawnattr_tcsetpgrp_np extensions[1]. This was done with the intention of improving performance in interactive shells without reintroducing previous race conditions[2][3]. These changes were tested with glibc commit 342cc934. [1]: https://sourceware.org/git/?p=glibc.git;a=commit;h=342cc934a3bf74ac618e2318d738f22ac93257ba [2]: #79 [3]: https://www.mail-archive.com/ast-developers@research.att.com/msg00717.html src/cmd/ksh93/sh/path.c: - Tell spawnveg to set the terminal process group when launching a child process in an interactive shell. src/cmd/ksh93/sh/xec.c: - Allow usage of spawnveg when posix_spawnattr_tcsetpgrp_np is available. - Reimplement most of the sh_ntfork() SIGTSTP handling code removed in commit 66c3720. src/lib/libast/comp/spawnveg.c, src/lib/libast/misc/procopen.c, src/lib/libast/features/sys: - Add support for POSIX_SPAWN_TCSETPGROUP as implemented in glibc. This code (with a few changes) might also work on QNX, but I've avoided changing anything on that OS since I don't have a QNX system to test with. src/lib/libast/man/spawnveg.3: - Document the changes to spawnveg(3) in the corresponding man page. Currently spawnveg will only use the new tcfd argument if posix_spawnattr_tcsetpgrp_np is supported. This could also be implemented for the fork fallback, but for now I've avoided changing that since actually using it in the fork code would likely require a lot of hackery to avoid attempting tcsetpgrp with vfork (the behavior of tcsetpgrp after vfork is not portable) and would only benefit systems that don't have posix_spawn and vfork (I can't recall any off the top of my head that would fall under that category). src/lib/libast/features/lib: - Detect the existence of the GNU posix_spawnattr_tcsetpgrp_np() extension. - Do not detect an OS spawnveg(3). With the API changes to spawnveg in this pull request ksh probably can't use the OS's spawnveg function anymore. (That's assuming anything else even provides a spawnveg function to begin with, which is unlikely.) src/lib/libast/features/api, src/cmd/ksh93/include/defs.h: - Bump libast version (20220101 => 20220201) due to the spawnveg(3) API change.

This commit implements support for the glibc 2.35 posix_spawn_file_actions_addtcsetpgrp_np(3) extension[2][3], updating spawnveg(3) to use the new function for setting the terminal group. This was done with the intention of improving performance in interactive shells without reintroducing previous race conditions[4][5]. [1]: https://sourceware.org/pipermail/libc-alpha/2022-February/136040.html [2]: https://sourceware.org/git/?p=glibc.git;a=commit;h=342cc934 [3]: https://sourceware.org/git/?p=glibc.git;a=commit;h=6289d28d [4]: #79 [5]: https://www.mail-archive.com/ast-developers@research.att.com/msg00717.html src/cmd/ksh93/sh/path.c: - Tell spawnveg(3) to set the terminal process group when launching a child process in an interactive shell. src/cmd/ksh93/sh/xec.c: - If posix_spawn_file_actions_addtcsetpgrp_np(3) is available, allow use of spawnveg(3) (via sh_ntfork()) even with job control active. - sh_ntfork(): Reimplement most of the SIGTSTP handling code removed in commit 66c3720. src/lib/libast/comp/spawnveg.c, src/lib/libast/misc/procopen.c, src/lib/libast/features/sys: - Add support for posix_spawn_file_actions_addtcsetpgrp_np(3). - Allow spawnveg to set the terminal process group when pgid == 0. This was necessary to avoid race conditions when using the new function. src/lib/libast/features/lib: - Detect posix_spawn_file_actions_addtcsetpgrp_np(3). - Do not detect an OS spawnveg(3). With the API changes to spawnveg in this pull request ksh probably can't use the OS's spawnveg function anymore. (That's assuming anything else even provides a spawnveg function to begin with, which is unlikely.) src/lib/libast/features/api, src/cmd/ksh93/include/defs.h: - Bump libast version (20220101 => 20220201) due to the spawnveg(3) API change. src/lib/libast/man/spawnveg.3: - Document the changes to spawnveg(3) in the corresponding man page. Currently, it will only use the new tcfd argument if posix_spawn_file_actions_addtcsetpgrp_np(3) is supported. This could also be implemented for the fork(2) fallback, but for now I've avoided changing that since actually using it in the fork code would likely require a lot of hackery to avoid attempting tcsetpgrp with vfork (the behavior of tcsetpgrp after vfork is not portable) and would only benefit systems that don't have posix_spawn and vfork (I can't recall any off the top of my head that would fall under that category). - Updated the man page to account for spawnveg's change in behavior. Co-authored-by: Martijn Dekker <martijn@inlv.org>

McDutchie added blocker This had better be fixed before releasing bug Something is not working labels Jul 17, 2020

McDutchie added the help wanted Extra attention is needed label Jul 18, 2020

McDutchie removed the help wanted Extra attention is needed label Jul 18, 2020

McDutchie mentioned this issue Jul 22, 2020

external command causes SIGINT to be ignored within non-forking subshell #86

Closed

McDutchie closed this as completed in f207cd5 Jul 22, 2020

McDutchie mentioned this issue Jul 23, 2020

Faulty logic for disabling job control in virtual subshells #89

Closed

McDutchie mentioned this issue Aug 1, 2020

Regression with command substitution when compiled with SHOPT_SPAWN #104

Closed

citrus-it added a commit to citrus-it/ast that referenced this issue Jan 16, 2021

Failure to launch external commands due to posix_spawn race

90becb7

ksh93#79

McDutchie mentioned this issue Mar 3, 2021

echo "" | while read any_string ; ksh93 sh (AT&T Research) 93u+ 2012-08-01 #204

Closed

JohnoKing mentioned this issue Jan 27, 2022

Implement support for POSIX_SPAWN_TCSETPGROUP and fix spawnveg bugs #438

Merged

Failure to launch external commands due to race condition in 'posix_spawn' usage #79

Failure to launch external commands due to race condition in 'posix_spawn' usage #79

Comments

aweeraman commented Jul 17, 2020

posguy99 commented Jul 17, 2020 • edited Loading

mirabilos commented Jul 17, 2020 via email

JohnoKing commented Jul 17, 2020

McDutchie commented Jul 17, 2020 • edited Loading

McDutchie commented Jul 17, 2020

McDutchie commented Jul 18, 2020 • edited Loading

chrisbertin commented Jul 18, 2020 via email

posguy99 commented Jul 18, 2020

McDutchie commented Jul 18, 2020 • edited Loading

posguy99 commented Jul 18, 2020

McDutchie commented Jul 18, 2020

JohnoKing commented Jul 18, 2020 • edited Loading

McDutchie commented Jul 18, 2020 • edited Loading

JohnoKing commented Jul 18, 2020 • edited Loading

hyenias commented Jul 18, 2020

JohnoKing commented Jul 18, 2020 • edited Loading

McDutchie commented Jul 18, 2020

JohnoKing commented Jul 18, 2020 • edited Loading

hyenias commented Jul 18, 2020

JohnoKing commented Jul 18, 2020

McDutchie commented Jul 18, 2020

McDutchie commented Jul 18, 2020 • edited Loading

hyenias commented Jul 18, 2020

McDutchie commented Jul 18, 2020

chrisbertin commented Jul 18, 2020 via email

McDutchie commented Jul 18, 2020

JohnoKing commented Jul 21, 2020

McDutchie commented Jul 21, 2020

McDutchie commented Jul 22, 2020 • edited Loading

JohnoKing commented Jul 22, 2020

McDutchie commented Jul 22, 2020

McDutchie commented Jul 22, 2020 • edited Loading

JohnoKing commented Jul 22, 2020

McDutchie commented Jul 22, 2020 • edited Loading

aweeraman commented Jul 22, 2020

McDutchie commented Jul 23, 2020

posguy99 commented Dec 26, 2021 • edited Loading

McDutchie commented Dec 26, 2021

JohnoKing commented Dec 30, 2021 • edited Loading

zatrazz commented Jan 25, 2022

posguy99 commented Jul 17, 2020 •

edited

Loading

McDutchie commented Jul 17, 2020 •

edited

Loading

McDutchie commented Jul 18, 2020 •

edited

Loading

McDutchie commented Jul 18, 2020 •

edited

Loading

JohnoKing commented Jul 18, 2020 •

edited

Loading

McDutchie commented Jul 18, 2020 •

edited

Loading

JohnoKing commented Jul 18, 2020 •

edited

Loading

JohnoKing commented Jul 18, 2020 •

edited

Loading

JohnoKing commented Jul 18, 2020 •

edited

Loading

McDutchie commented Jul 18, 2020 •

edited

Loading

McDutchie commented Jul 22, 2020 •

edited

Loading

McDutchie commented Jul 22, 2020 •

edited

Loading

McDutchie commented Jul 22, 2020 •

edited

Loading

posguy99 commented Dec 26, 2021 •

edited

Loading

JohnoKing commented Dec 30, 2021 •

edited

Loading