Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement support for POSIX_SPAWN_TCSETPGROUP and fix spawnveg bugs #438

Merged
merged 13 commits into from
Feb 3, 2022

Conversation

JohnoKing
Copy link

@JohnoKing JohnoKing commented Jan 27, 2022

This commit implements support for the glibc POSIX_SPAWN_TCSETPGROUP and posix_spawnattr_tcsetpgrp_np extensions1. This was done with the intention of improving performance in interactive shells without reintroducing previous race conditions23. These changes were tested using 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 is -1, use POSIX_SPAWN_SETSID when it's available (spawnveg uses setsid(2) in the fork fallback, so it should do the same thing when posix_spawn is used instead).

src/lib/libast/man/spawnveg.3:
- Document the changes to spawnveg 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.

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/[email protected]/msg00717.html
@JohnoKing JohnoKing force-pushed the posix-spawn-tcsetpgrp branch from cce6d2a to 6dc4f9f Compare January 27, 2022 17:28
This changes spawnveg to prefer posix_spawn whenever it's available,
which allows some formerly dead code (in the _lib_posix_spawn < 2 code
block) to be used. The additional definition for WNOWAIT fixes a
build error on OpenBSD.
For some reason the previous change caused many regression test failures on
OpenBSD. To fix that, refactor to remove previous dead code and avoid
posix_spawn on OpenBSD.
On closer inspection only the broken dead code removed in this
pull request's third commit was actually using WNOWAIT. Since
this added definition is now completely unused, remove it.
@McDutchie
Copy link

McDutchie commented Jan 27, 2022

How would I go about testing this in a way that is not massively time-consuming? Replacing glibc on an existing distro generally breaks things, doesn't it?

Since these changes are new to both glibc and ksh, it's probably best to keep them to the dev branch only, so that they can see a good amount of testing before making it into 1.1, when that time comes.

I'd like to separate the bugfixes into a commit of their own that I can also backport to the 1.0 branch – particularly the syntax errors in features/lib which are my fault, I introduced them in 1064933 – oops. That was a sed gone wrong. I'm going to have to go over that commit again with a fine-toothed comb to check if any other problems were introduced.

@JohnoKing
Copy link
Author

JohnoKing commented Jan 28, 2022

I'm not entirely sure of how to go about replacing glibc on most distributions. I'm currently using the glibc-git AUR package to test these changes. The PKGBUILD script could be used as a reference for building and installing glibc using the latest git commit.

I'd like to separate the bugfixes into a commit of their own that I can also backport to the 1.0 branch

The other bugfix in this pull request is the change to use POSIX_SPAWN_SETSID when it's available (so spawnveg when using posix_spawn acts the same as when using the fork fallback). I'll separate that into it's own pull request (edit: now #439).

One of the previous code changes could break if neither _lib_tcgetpgrp
or TIOCSPGRP were defined. This fixes that potential problem.
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.) Avoid using
that since it would result in a guaranteed build failure.
@McDutchie
Copy link

So, I looked up how AUR packages work and finally got as far as running makepkg on that glibc-git AUR package, and now the build fails:
Schermafbeelding 2022-02-02 om 23 56 23
I really don't have time to mess with this any more, I'll just take your word for it all. If it breaks, well, it's the dev branch.

@McDutchie McDutchie merged commit 8e9ed5b into ksh93:dev Feb 3, 2022
@JohnoKing JohnoKing deleted the posix-spawn-tcsetpgrp branch February 3, 2022 02:10
@JohnoKing
Copy link
Author

I figured out that the build failure you encountered is caused by the default CFLAGS in /etc/makepkg.conf (I hadn't encountered that failure on my system because my makepkg.conf had different CFLAGS). This should fix the glibc-git build failure:

--- makepkg.conf	2022-01-08 01:13:06.000000000 -0800
+++ makepkg.conf.patch	2022-02-03 11:16:59.612926347 -0800
@@ -38,9 +38,10 @@
 
 #-- Compiler and Linker Flags
 #CPPFLAGS=""
-CFLAGS="-march=x86-64 -mtune=generic -O2 -pipe -fno-plt -fexceptions \
-        -Wp,-D_FORTIFY_SOURCE=2 -Wformat -Werror=format-security \
-        -fstack-clash-protection -fcf-protection"
+#CFLAGS="-march=x86-64 -mtune=generic -O2 -pipe -fno-plt -fexceptions \
+#        -Wp,-D_FORTIFY_SOURCE=2 -Wformat -Werror=format-security \
+#        -fstack-clash-protection -fcf-protection"
+CFLAGS="-march=x86-64 -O2 -pipe "
 CXXFLAGS="$CFLAGS -Wp,-D_GLIBCXX_ASSERTIONS"
 LDFLAGS="-Wl,-O1,--sort-common,--as-needed,-z,relro,-z,now"
 #RUSTFLAGS="-C opt-level=2"

Additionally, glibc 2.35 has recently been released (changelog), although right before the 2.35 release posix_spawnattr_tcsetpgrp_np was replaced by posix_spawn_file_actions_addtcsetpgrp_np. I'm currently testing a patch that updates spawnveg in accordance with that change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants