search

ksh93 / ksh

ksh 93u+m: KornShell lives! | Latest release: https://github.com/ksh93/ksh/releases
Eclipse Public License 2.0
185 stars 31 forks source link

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

Closed aweeraman closed 4 years ago

aweeraman commented 4 years ago

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.

posguy99 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 notifications@github.com wrote:

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 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 https://github.com/ksh93/ksh/issues/79, or unsubscribe https://github.com/notifications/unsubscribe-auth/AQKGJUJOFTOVXOBFI6ZRDYDR4C6RPANCNFSM4O65E6KA .

posguy99 commented 4 years ago

Is this 2020 bug relevant?

https://github.com/att/ast/pull/470 which leads to https://github.com/att/ast/issues/468

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

McDutchie commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

Agreed. That's not going to happen though.

JohnoKing commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

~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 commented 4 years ago

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

JohnoKing commented 4 years ago

Some notes regarding ksh usage of vfork:

McDutchie commented 4 years ago

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: https://github.com/ksh93/ksh/blob/37a9c34515c8c966447f75b10a5205934ca51d7d/src/lib/libast/sfio/sfhdr.h#L438-L448 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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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 notifications@github.com 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 https://github.com/ksh93/ksh/issues/79#issuecomment-660502713, or unsubscribe https://github.com/notifications/unsubscribe-auth/AQKGJUO23JAMHYWJYJGFRIDR4HBJDANCNFSM4O65E6KA .

McDutchie commented 4 years ago

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().

mirabilos commented 4 years ago

chrisbertin dixit:

vfork() is a pre-VM, or, at least pre-COW, semi-kludge and I thought it would have been retired by now... IMHO...

In many operating systems, it’s equivalent to fork(); some removed it even. In any case, the pre-COW behaviour of it in some historic OSes MUST NOT be relied on.

bye, //mirabilos -- “It is inappropriate to require that a time represented as seconds since the Epoch precisely represent the number of seconds between the referenced time and the Epoch.” -- IEEE Std 1003.1b-1993 (POSIX) Section B.2.2.2

mirabilos commented 4 years ago

Johnothan King dixit:

The OpenSUSE patch causes vfork to be used instead of posix_spawn

SuSE has another patch to disable vfork.

bye, //mirabilos --

Beware of ritual lest you forget the meaning behind it. yeah but it means if you really care about something, don't ritualise it, or you will lose it. don't fetishise it, don't obsess. or you'll forget why you love it in the first place.
posguy99 commented 4 years ago

@mirabilos said:

SuSE has another patch to disable vfork.

That's this one: ksh93-disable-vfork-dif

But ksh.changes there doesn't say why and doesn't even give a SuSE bug reference that you can't read. :(

chrisbertin commented 4 years ago

I see no point in using vfork() over fork() but, if the vfork synchronisation was done right, it seems to me it still should work. The fact that it occasionally succeeds seems to indicate a timing issue.

sixt $ cat /tmp/list 1 2 3 sixt $ cat /tmp/list | while read f; do /bin/echo $f; done 1 /bin/echo: /bin/echo: cannot execute [Exec format error] /bin/echo: /bin/echo: cannot execute [Exec format error] sixt $ cat /tmp/list | while read f; do /bin/echo $f; done /bin/echo: /bin/echo: cannot execute [Exec format error] /bin/echo: /bin/echo: cannot execute [Exec format error] /bin/echo: /bin/echo: cannot execute [Exec format error] sixt $ cat /tmp/list | while read f; do df /; done Filesystem 1K-blocks Used Available Use% Mounted on /dev/mapper/kubuntu--vg-newroot 124779940 93010072 25388300 79% / /bin/df: /bin/df: cannot execute [Exec format error] /bin/df: /bin/df: cannot execute [Exec format error] sixt $ cat /tmp/list | while read f; do df /; done /bin/df: /bin/df: cannot execute [Exec format error] /bin/df: /bin/df: cannot execute [Exec format error] /bin/df: /bin/df: cannot execute [Exec format error] sixt $

Chris (chris.bertin@gmail.com or chris.bertin@protonmail.com)

On Sat, Jul 18, 2020 at 11:38 AM Marc Wilson notifications@github.com wrote:

@mirabilos https://github.com/mirabilos said:

SuSE has another patch to disable vfork.

That's this one: ksh93-disable-vfork-dif https://build.opensuse.org/package/view_file/shells/ksh/ksh93-disable-vfork.dif?expand=1

But ksh.changes https://build.opensuse.org/package/view_file/shells/ksh/ksh.changes?expand=1 there doesn't say why and doesn't even give a SuSE bug reference that you can't read. :(

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ksh93/ksh/issues/79#issuecomment-660523524, or unsubscribe https://github.com/notifications/unsubscribe-auth/AQKGJUKESXFSJQ3I5NJYCOLR4HT2HANCNFSM4O65E6KA .

McDutchie commented 4 years ago

Chris, I believe you're seeing the symptoms of using posix_spawn, which is what ksh currently does. Please see upthread. When we switch to using vfork instead, that bug should go away.

chrisbertin commented 4 years ago

OK, great. As long as you know what the issue is, I am happy! :-)

Chris (chris.bertin@gmail.com or chris.bertin@protonmail.com)

On Sat, Jul 18, 2020 at 12:24 PM Martijn Dekker notifications@github.com wrote:

Chris, I believe you're seeing the symptoms of using posix_spawn, which is what ksh currently does. Please see upthread. When we switch to using vfork instead, that bug should go away.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ksh93/ksh/issues/79#issuecomment-660529818, or unsubscribe https://github.com/notifications/unsubscribe-auth/AQKGJUJU4SH6CWXJX72JGR3R4HZGTANCNFSM4O65E6KA .

chrisbertin commented 4 years ago

But, also, BTW, how do you explain that running the whole command in an additional sub-shell (ksh -c 'command | while ...') always works?

Chris (chris.bertin@gmail.com or chris.bertin@protonmail.com)

On Sat, Jul 18, 2020 at 12:24 PM Martijn Dekker notifications@github.com wrote:

Chris, I believe you're seeing the symptoms of using posix_spawn, which is what ksh currently does. Please see upthread. When we switch to using vfork instead, that bug should go away.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ksh93/ksh/issues/79#issuecomment-660529818, or unsubscribe https://github.com/notifications/unsubscribe-auth/AQKGJUJU4SH6CWXJX72JGR3R4HZGTANCNFSM4O65E6KA .

posguy99 commented 4 years ago

The subshell avoids the race condition.

McDutchie commented 4 years ago

@mirabilos wrote (re vfork):

In many operating systems, it’s equivalent to fork(); some removed it even. In any case, the pre-COW behaviour of it in some historic OSes MUST NOT be relied on.

Are macOS and Linux "some historic OSes"? As seen upthread, both show a significant performance improvement over fork() for launching external commands.

As for relying on it, I believe that is covered by the current iffe feature tests for expected vfork() behaviour, which of course we'll keep. If you see anything wrong with those, please do say. https://github.com/ksh93/ksh/blob/37a9c34515c8c966447f75b10a5205934ca51d7d/src/lib/libast/features/lib#L200-L242

posguy99 commented 4 years ago

@McDutchie observed:

Are macOS and Linux "some historic OSes"? As seen upthread, both show a significant performance improvement over fork() for launching external commands.

And ksh on macOS will actually get a performance boost out of all this vs Apple's shipped build.

Can't argue with that.

mirabilos commented 4 years ago

Martijn Dekker dixit:

Are macOS and Linux "some historic OSes"? As seen upthread, both show a

Yes. Also, even the Linux manpage notes:

The vfork() function has the same effect as fork(2), ex‐
   cept that the behavior is undefined if the process created  by  vfork()
   either  modifies  any  data other than a variable of type pid_t used to
   store the return value from vfork(), or […]
            calls any other function before success‐
   fully calling _exit(2) or one of the exec(3) family of functions.

And:

    Users should
   not depend on the memory sharing semantics of vfork() as  it  will,  in
   that case, be made synonymous to fork(2).

And:

    In particular, the programmer cannot rely on the parent re‐
   maining blocked until the child either terminates or  calls  execve(2),
   and cannot rely on any specific behavior with respect to shared memory.

It does document the way this is implemented in Linux currently, but this is fragile (e.g. when threads are used), signal handlers cause mayhem, and the mentioned ioctl is also unsupported.

As for relying on it, I believe that is covered by the current iffe feature tests for expected vfork() behaviour, which of course we'll

How are you testing for this without executing a compiled program (cross-compile scenario)? You can’t.

McDutchie commented 4 years ago

@mirabilos wrote:

Yes. Also, even the Linux manpage notes: […]

Yes, all that is well known. Which is why vfork will be used instead of posix_spawn for launching external commands, i.e., when the (v)fork is immediately followed by an exec. That's the intended use. Obviously it's not going to be used for forking subshell environments.

How are you testing for this without executing a compiled program (cross-compile scenario)? You can’t.

What do you think iffe actually does with that C test code that I quoted you?

And what do you mean by "cross-compile scenario"? Of course we're going to test ksh 93u+m on all the OSs we can get our hands on before releasing it.

mirabilos commented 4 years ago

Martijn Dekker dixit:

Yes, all that is well known. Which is why vfork will be used instead of posix_spawn for launching external commands, i.e., when the (v)fork is immediately followed by an exec. That's the intended use.

I thought the whole problem with posix_spawn is that, between the fork and exec, you need to do something with tty?

You cannot do that with vfork either.

How are you testing for this without executing a compiled program (cross-compile scenario)? You can’t.

What do you think iffe actually does with that C test code that I quoted you?

You cannot do runtime tests in configury, only compile-time and link-time ones, because…

And what do you mean by "cross-compile scenario"? Of course we're going to test ksh 93u+m on all the OSs we can get our hands on before releasing it.

… people will want to be able to cross-compile it and receive the same result as a native build would be. (You can do this with, for example, mksh — if the compiler is good, it’ll be bitwise identical.)

bye, //mirabilos -- 11:56⎜«liwakura:#!/bin/mksh» also, i wanted to add mksh to my own distro │ i was disappointed that there is no makefile │ but somehow the Build.sh is the least painful built system i've ever seen │ honours CC, {CPP,C,LD}FLAGS properly │ looks cleary like done by someone who knows what they are doing

McDutchie commented 4 years ago

@mirabilos wrote:

I thought the whole problem with posix_spawn is that, between the fork and exec, you need to do something with tty? You cannot do that with vfork either.

It's not the whole point, I'm pretty sure the bug report that started this thread has nothing to do with that.

However, you do make an interesting point related to this old bug report and David Korn's response. Does using vfork instead of posix_spawn involve the same inherent race condition? @JohnoKing et al, any thoughts on this?

… people will want to be able to cross-compile it and receive the same result as a native build would be. (You can do this with, for example, mksh — if the compiler is good, it’ll be bitwise identical.)

People can't always get what they want. What you describe is incompatible with feature testing, and iffe is fundamentally a feature testing system that does all sorts of runtime tests. ksh93 and mksh have different priorities here, and that is fine.

JohnoKing commented 4 years ago

@McDutchie wrote:

However, you do make an interesting point related to this old bug report and David Korn's response. Does using vfork instead of posix_spawn involve the same inherent race condition? @JohnoKing et al, any thoughts on this?

When I run the reproducer from the old bug report in a while loop, the bug occurs with fork, vfork and posix_spawn if SHOPT_SPAWN is enabled (vfork is disabled with ksh93-disable-vfork.dif from OpenSUSE):

# Run this in an interactive shell compiled with posix_spawn and/or vfork disabled
$ cc -O2 -o testprocessgroup testprocessgroup.c
$ (while true; do ./testprocessgroup; done) > /tmp/output
# Run grep from a different terminal after running the above
# command for a while.
$ grep 'DO NOT' /tmp/output
PID:10156  MY_pgrp=10156 STDIN_PGRP:3452 --DO NOT own terminal
PID:14730  MY_pgrp=14730 STDIN_PGRP:3452 --DO NOT own terminal
PID:14881  MY_pgrp=14881 STDIN_PGRP:3452 --DO NOT own terminal
PID: 5572  MY_pgrp=5572 STDIN_PGRP:3452 --DO NOT own terminal
etc.

The bug goes away when SHOPT_SPAWN is disabled (-DSHOPT_SPAWN=0 disables spawnveg to force fork), but that causes the above while loop to ignore Ctrl+C.

McDutchie commented 4 years ago

@JohnoKing: I've run the same test as you did, in the same way, on my Mac with ksh binaries using posix_spawn, vfork, and fork. I cannot reproduce the race condition on any of these.

However, I can reproduce the "^C is ignored" problem on the binary compiled with -DSHOPT_SPAWN=0; this also occurs on stock /bin/ksh on the Mac (which is also compiled with -DSHOPT_SPAWN=0).

Meanwhile, here is a test branch with the use of posix_spawn removed.

McDutchie commented 4 years ago

So, on Linux (Debian and Ubuntu), my test branch that uses vfork fails hard: ksh freezes when running the regression tests. So, that's a no go.

Between that, and the finding (two comments up) that AST spawnveg(3) has race condition(s) no matter what method it uses, I'm afraid it looks like we're back to "the ksh2020 folks were right on this one". At this point I don't see how this can ever be made to work correctly.

McDutchie commented 4 years ago

Re the bug with ignoring Ctrl+C when compiling with -DSHOPT_SPAWN=0…

With the following, it's not ignored:

$ while true; do ./testprocessgroup; done > /tmp/output  
^C$ 
$ (ulimit -t unlimited; while true; do ./testprocessgroup; done) > /tmp/output 
^C$

So, either not using a subshell, or forking the subshell (using ulimit), works around the problem. You've found yet another problem with non-forking/virtual subshells.

It can be reproduced like this as well:

$ (while true; do /usr/bin/true; done) >/tmp/output
^C^C^C^C^C

...and even SIGTERM won't terminate it, I needed SIGKILL.

But I still think this stands a better chance of being fixed than AST spawnveg(3). Any ideas?

hyenias commented 4 years ago

I was reading through this issue's content again. In it, you referenced a Tue, 14 Jun 2011 13:33:15 -0700 email from David Korn that stated:

ksh93 uses posix_spawn() rather than fork/exec to run a simple command for performance reasons. However, posix_spawnv() doesn't have a way to set the terminal group before the exec(). The parent process sets the terminal group which leads to the race.

There is a simple change you can make to a program to guarentee that the terminal will get set at the beginning of the program. You can add two lines. One line is

include

and for the other line
tcsetpgrp(2,getpgrp()); Alternatively, you can write a program that does these two lines and then execs the original program.

Is it possible to create a shim program that performs these two lines of code then executes the original program? If so, does that resolve the issue, or would that be too messy and be slower than the fork solution? The shim program might even be able to be a builtin if possible. Perhaps an environment flag passed along to the subshell that tells ksh to fixup the terminal.

JohnoKing commented 4 years ago

@McDutchie said:

So, on Linux (Debian and Ubuntu), my test branch that uses vfork fails hard: ksh freezes when running the regression tests. Between that, and the finding (two comments up) that AST spawnveg(3) has race condition(s) no matter what method it uses, I'm afraid it looks like we're back to "the ksh2020 folks were right on this one".

I can't get the regression tests to freeze on Linux (Ubuntu 20.04 and Arch Linux) at all when using vfork. As for reproducing the race condition with vfork, I can't get it to occur in dash (which is another shell that uses vfork to run commands).

@hyenias said:

Is it possible to create a shim program that performs these two lines of code then executes the original program? If so, does that resolve the issue, or would that be too messy and be slower than the fork solution? The shim program might even be able to be a builtin if possible. Perhaps an environment flag passed along to the subshell that tells ksh to fixup the terminal.

Such a program can be made, but that will definitely be slower than just using fork if the shim is a separate binary. Making the shim a builtin is an interesting idea, but I don't see how that could work with posix_spawn.

McDutchie commented 4 years ago

I don't see how making such a shim a builtin command would make any sense, even if it could work. Builtin commands are C functions that are part of the ksh binary like all the other code, so the code of such a builtin might as well be included directly into the routines that handle whatever mechanism is used to launch external commands.

The problem, as I understand it now, is some race condition in the AST spawnveg(3) function, and the question is if we can fix it.

JohnoKing commented 4 years ago

This is the code spawnveg runs in the child process after fork and vfork: https://github.com/ksh93/ksh/blob/bd88cc7f4f7207b48104e37cca90848adf5cb10c/src/lib/libast/comp/spawnveg.c#L207-L225 tcsetpgrp and ioctl aren't run when launching commands. Inserting error(ERROR_warn(0) in the if(m) statements can be used to confirm it:

# Add a debug warning before tcsetpgrp
--- a/src/lib/libast/comp/spawnveg.c
+++ b/src/lib/libast/comp/spawnveg.c
@@ -216,7 +216,10 @@ spawnveg(const char* path, char* const argv[], char* const envv[], pid_t pgid)
                setpgid(pgid, 0);
 #if _lib_tcgetpgrp
            if (m)
+           {
+               error(ERROR_warn(0), "[DEBUG]: Setting process group");
                tcsetpgrp(2, pgid);
+           }
 #else
 #ifdef TIOCSPGRP
            if (m)
$ $(whence -p true)
# Even with the above patch, there is no debug output after running the external `true` command.
# This means the child process didn't set the terminal process group.

This issue can be fixed for vfork and fork by running tcsetpgrp (or ioctl) before setpgid. Below is a quick patch that does that:

--- a/src/lib/libast/comp/spawnveg.c
+++ b/src/lib/libast/comp/spawnveg.c
@@ -211,7 +211,14 @@ spawnveg(const char* path, char* const argv[], char* const envv[], pid_t pgid)
        {
            m = 0;
            if (pgid == 1 || pgid == -2 && (m = 1))
+           {
                pgid = getpid();
+               if(!m)
+               {
+                   error(ERROR_warn(0), "[DEBUG]: Setting process group");
+                   tcsetpgrp(2, pgid);
+               }
+           }
            if (setpgid(0, pgid) < 0 && errno == EPERM)
                setpgid(pgid, 0);
$ $(whence -p true)
arch/linux.i386-64/bin/ksh: /usr/bin/true: warning: [DEBUG]: Setting process group
$

After applying the above patch, I'm no longer able to reproduce the race condition with fork or vfork when SHOPT_SPAWN is enabled (reproducer). edit: This only fixes the race condition on FreeBSD when spawnveg uses fork. vfork can't be used on FreeBSD, although it works fine on Linux (see https://github.com/ksh93/ksh/issues/79#issuecomment-661426491).

Since the thread is now quite long, here are direct links to the OpenSUSE patches: Disable posix_spawn (fallback to vfork): https://build.opensuse.org/package/view_file/shells/ksh/ksh-qemu.patch?expand=1

Disable vfork (combine with above patch to force spawnveg to use fork): https://build.opensuse.org/package/view_file/shells/ksh/ksh93-disable-vfork.dif?expand=1

hyenias commented 4 years ago

@JohnoKing This is most certainly good news of the awesome kind! I want to celebrate. For clarity, does CTRL+C and/or SIGTERM work to terminate the vforked or forked process? Inquiring minds want to know.

JohnoKing commented 4 years ago

SIGINT and SIGTERM don't seem to work with vfork if execve is used on a non-existent binary (although fork works fine):

$ (while true; do ./nonexist; done)
^C^C^C # Can't cancel with vfork