Use fork()/exec() instead of posix_spawn() by default #470
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Using posix_spawn() may result in a race condition while setting
terminal foreground process group. Copying dgk's response from related
discussion in ast-developers mailing list:
"
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 <termios.h>
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.
"
Only way to reliably fix this problem in ksh is to switch back to fork()/
exec().
If there are any performance regressions or bugs caused by this change,
you can switch to posix_spawn() by setting '-D_AST_no_spawnveg=0'.
Related: #468
Related: rhbz#1160482
|
Test cases have started failing with this change. They are not failing if I build with |
spawnvex family of function were introduced after the last stable release as an improvement over spawnveg. They were being compiled even when _AST_no_spawnveg macro was defined. This was causing test cases to fail if ksh is compiled with '-D_AST_no_spawnveg=1' flag. This commit makes compilation of these functions conditional. Related: #468
|
It should be fixed now. |
|
LGTM |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Using posix_spawn() may result in a race condition while setting
terminal foreground process group. Copying dgk's response from related
discussion in ast-developers mailing list:
"
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 <termios.h>
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.
"
Only way to reliably fix this problem in ksh is to switch back to fork()/
exec().
If there are any performance regressions or bugs caused by this change,
you can switch to posix_spawn() by setting '-D_AST_no_spawnveg=0'.
Related: #468
Related: rhbz#1160482