Skip to content

Commit

Permalink
Implement support for POSIX_SPAWN_TCSETPGROUP & fix spawnveg bugs
Browse files Browse the repository at this point in the history
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]: #79
[3]: https://www.mail-archive.com/[email protected]/msg00717.html
  • Loading branch information
JohnoKing committed Jan 27, 2022
1 parent 632214c commit cce6d2a
Show file tree
Hide file tree
Showing 7 changed files with 105 additions and 21 deletions.
2 changes: 1 addition & 1 deletion src/cmd/ksh93/sh/path.c
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ static pid_t _spawnveg(const char *path, char* const argv[], char* const envp[],
while(1)
{
sh_stats(STAT_SPAWN);
pid = spawnveg(path,argv,envp,pgid);
pid = spawnveg(path,argv,envp,pgid,job.jobcontrol?job.fd:-1);
if(pid>=0 || errno!=EAGAIN)
break;
}
Expand Down
34 changes: 33 additions & 1 deletion src/cmd/ksh93/sh/xec.c
Original file line number Diff line number Diff line change
Expand Up @@ -1623,7 +1623,11 @@ int sh_exec(register const Shnode_t *t, int flags)
fifo_save_ppid = sh.current_pid;
#endif
#if SHOPT_SPAWN
#if !_lib_posix_spawnattr_tcsetpgrp_np
if(com && !job.jobcontrol)
#else
if(com)
#endif /* !_lib_posix_spawnattr_tcsetpgrp_np */
{
parent = sh_ntfork(t,com,&jobid,ntflag);
if(parent<0)
Expand Down Expand Up @@ -3462,7 +3466,8 @@ static void sigreset(int mode)

/*
* A combined fork/exec for systems with slow fork().
* Incompatible with job control on interactive shells (job.jobcontrol).
* Incompatible with job control on interactive shells (job.jobcontrol) if
* the system does not support posix_spawnattr_tcsetpgrp_np().
*/
static pid_t sh_ntfork(const Shnode_t *t,char *argv[],int *jobid,int flag)
{
Expand All @@ -3474,6 +3479,9 @@ static pid_t sh_ntfork(const Shnode_t *t,char *argv[],int *jobid,int flag)
char **arge, *path;
volatile pid_t grp = 0;
Pathcomp_t *pp;
#if _lib_posix_spawnattr_tcsetpgrp_np
volatile int jobwasset=0;
#endif /* _lib_posix_spawnattr_tcsetpgrp_np */
if(flag)
{
otype = savetype;
Expand Down Expand Up @@ -3538,8 +3546,21 @@ static pid_t sh_ntfork(const Shnode_t *t,char *argv[],int *jobid,int flag)
}
arge = sh_envgen();
sh.exitval = 0;
#if _lib_posix_spawnattr_tcsetpgrp_np
if(job.jobcontrol)
{
signal(SIGTTIN,SIG_DFL);
signal(SIGTTOU,SIG_DFL);
signal(SIGTSTP,SIG_DFL);
jobwasset++;
}
#endif /* _lib_posix_spawnattr_tcsetpgrp_np */
#ifdef JOBS
#if _lib_posix_spawnattr_tcsetpgrp_np
if(sh_isstate(SH_MONITOR) && (job.jobcontrol || (otype&FAMP)))
#else
if(sh_isstate(SH_MONITOR) && (otype&FAMP))
#endif /* _lib_posix_spawnattr_tcsetpgrp_np */
{
if((otype&FAMP) || job.curpgid==0)
grp = 1;
Expand Down Expand Up @@ -3600,6 +3621,17 @@ static pid_t sh_ntfork(const Shnode_t *t,char *argv[],int *jobid,int flag)
sh_popcontext(buffp);
if(buffp->olist)
free_list(buffp->olist);
#if _lib_posix_spawnattr_tcsetpgrp_np
if(jobwasset)
{
signal(SIGTTIN,SIG_IGN);
signal(SIGTTOU,SIG_IGN);
if(sh_isstate(SH_INTERACTIVE))
signal(SIGTSTP,SIG_IGN);
else
signal(SIGTSTP,SIG_DFL);
}
#endif /* _lib_posix_spawnattr_tcsetpgrp_np */
if(sigwasset)
sigreset(1); /* restore ignored signals */
if(scope)
Expand Down
44 changes: 35 additions & 9 deletions src/lib/libast/comp/spawnveg.c
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,12 @@

#include <ast.h>

/* Make sure posix_spawn was detected correctly to avoid
creating difficult to debug race conditions. */
#if !_lib_posix_spawn && _lib_posix_spawnattr_tcsetpgrp_np
#error "The posix_spawn feature test is borked."
#endif

#if _lib_spawnveg

NoN(spawnveg)
Expand All @@ -45,22 +51,38 @@ NoN(spawnveg)
#include <wait.h>

pid_t
spawnveg(const char* path, char* const argv[], char* const envv[], pid_t pgid)
spawnveg(const char* path, char* const argv[], char* const envv[], pid_t pgid, int tcfd)
{
int err;
int err, flags = 0;
pid_t pid;
posix_spawnattr_t attr;

if (err = posix_spawnattr_init(&attr))
goto nope;
if (pgid)
#if POSIX_SPAWN_SETSID
if (pgid == -1)
flags |= POSIX_SPAWN_SETSID;
#endif
if (pgid > 0)
{
flags |= POSIX_SPAWN_SETPGROUP;
#if _lib_posix_spawnattr_tcsetpgrp_np
if (tcfd >= 0)
flags |= POSIX_SPAWN_TCSETPGROUP;
#endif
}
if (flags && (err = posix_spawnattr_setflags(&attr, flags)))
goto bad;
if (pgid > 0)
{
if (pgid <= 1)
pgid = 0;
if (err = posix_spawnattr_setpgroup(&attr, pgid))
goto bad;
if (err = posix_spawnattr_setflags(&attr, POSIX_SPAWN_SETPGROUP))
#if _lib_posix_spawnattr_tcsetpgrp_np
if (tcfd >= 0 && (err = posix_spawnattr_tcsetpgrp_np(&attr, tcfd)))
goto bad;
#endif
}
if (err = posix_spawn(&pid, path, NiL, &attr, argv, envv ? envv : environ))
{
Expand Down Expand Up @@ -99,8 +121,9 @@ spawnveg(const char* path, char* const argv[], char* const envv[], pid_t pgid)
#endif

pid_t
spawnveg(const char* path, char* const argv[], char* const envv[], pid_t pgid)
spawnveg(const char* path, char* const argv[], char* const envv[], pid_t pgid, int tcfd)
{
NOT_USED(tcfd);
#if defined(P_DETACH)
return spawnve(pgid ? P_DETACH : P_NOWAIT, path, argv, envv ? envv : environ);
#else
Expand All @@ -119,10 +142,11 @@ spawnveg(const char* path, char* const argv[], char* const envv[], pid_t pgid)
*/

pid_t
spawnveg(const char* path, char* const argv[], char* const envv[], pid_t pgid)
spawnveg(const char* path, char* const argv[], char* const envv[], pid_t pgid, int tcfd)
{
struct inheritance inherit;

NOT_USED(tcfd);
inherit.flags = 0;
if (pgid)
{
Expand Down Expand Up @@ -156,7 +180,7 @@ spawnveg(const char* path, char* const argv[], char* const envv[], pid_t pgid)
*/

pid_t
spawnveg(const char* path, char* const argv[], char* const envv[], pid_t pgid)
spawnveg(const char* path, char* const argv[], char* const envv[], pid_t pgid, int tcfd)
{
#if _lib_fork || _lib_vfork
int n;
Expand All @@ -171,6 +195,7 @@ spawnveg(const char* path, char* const argv[], char* const envv[], pid_t pgid)
#endif
#endif

NOT_USED(tcfd);
if (!envv)
envv = environ;
#if _lib_spawnve
Expand Down Expand Up @@ -213,15 +238,16 @@ spawnveg(const char* path, char* const argv[], char* const envv[], pid_t pgid)
pgid = getpid();
if (setpgid(0, pgid) < 0 && errno == EPERM)
setpgid(pgid, 0);
#if _lib_tcgetpgrp
if (m)
{
#if _lib_tcgetpgrp
tcsetpgrp(2, pgid);
#else
#ifdef TIOCSPGRP
if (m)
ioctl(2, TIOCSPGRP, &pgid);
#endif
#endif
}
}
execve(path, argv, envv);
#if _real_vfork
Expand Down
7 changes: 4 additions & 3 deletions src/lib/libast/features/lib
Original file line number Diff line number Diff line change
Expand Up @@ -240,9 +240,9 @@ tst lib_posix_spawn unistd.h stdlib.h spawn.h -Dfork=______fork note{ posix_spaw
#include <string.h>
/* if it uses fork() why bother? */
#undef fork
pid_t fork(void)) { NOTE("uses fork()"; return -1; }
pid_t _fork(void)) { NOTE("uses _fork()"; return -1; }
pid_t __fork(void)) { NOTE("uses __fork()"; return -1; }
pid_t fork(void) { NOTE("uses fork()"); return -1; }
pid_t _fork(void) { NOTE("uses _fork()"); return -1; }
pid_t __fork(void) { NOTE("uses __fork()"); return -1; }
int
main(argc, argv)
int argc;
Expand Down Expand Up @@ -343,6 +343,7 @@ tst lib_posix_spawn unistd.h stdlib.h spawn.h -Dfork=______fork note{ posix_spaw
_exit(n);
}
}end
lib posix_spawnattr_tcsetpgrp_np

tst lib_spawn_mode unistd.h stdlib.h note{ first spawn arg is mode and it works }end execute{
#include <signal.h>
Expand Down
2 changes: 1 addition & 1 deletion src/lib/libast/features/sys
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,7 @@ extern setpgid int (pid_t, pid_t)
extern setsid pid_t (void)
extern setuid int (uid_t)
extern sleep unsigned (unsigned int)
extern spawnveg pid_t (const char*, char* const[], char* const[], pid_t)
extern spawnveg pid_t (const char*, char* const[], char* const[], pid_t, int)
extern srand void (unsigned int)
extern strcasecmp int (const char*, const char*)
extern strcat char* (char*, const char*)
Expand Down
33 changes: 29 additions & 4 deletions src/lib/libast/man/spawnveg.3
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ spawnveg \- process spawn with process group and session control
.SH SYNOPSIS
.L "#include <ast.h>"
.sp
.L "int spawnveg(const char* command, char** argv, char** envv, pid_t pgid);"
.L "int spawnveg(const char* command, char** argv, char** envv, pid_t pgid, int tcfd);"
.SH DESCRIPTION
.L spawnveg
combines
Expand Down Expand Up @@ -81,10 +81,35 @@ The new process becomes a process group leader.
.L >1
The new process joins the process group
.IR pgid .
.SH CAVEATS
.PP
The
.L tcfd
argument is currently only used if the operating system supports the
.I posix_spawnattr_tcsetpgrp_np
function.
When
.L tcfd
is
.L >=0
and
.L pgid
is
.LR >0 ,
spawnveg will set the controlling terminal for the new process to
.IR tcfd .
.SH CAVEATS
If the
.I posix_spawnattr_tcsetpgrp_np
function is not available, then
.L spawnveg
function cannot set the terminal process group.
cannot reliably set the terminal process group.
As a result, it is incompatible with job control when used with terminals.
Additionally, if the
.L POSIX_SPAWN_SETSID
attribute is not supported, then
.L spawnveg
cannot make the new process a session leader when using the
.I posix_spawn
API.
.SH "SEE ALSO"
fork(2), exec(2), setpgid(2), setsid(2), spawnve(2)
fork(2), posix_spawn(3), exec(2), setpgid(2), setsid(2), spawnve(2)
4 changes: 2 additions & 2 deletions src/lib/libast/misc/procopen.c
Original file line number Diff line number Diff line change
Expand Up @@ -744,7 +744,7 @@ sfsync(sfstderr);
if (forked || (flags & PROC_OVERLAY))
execve(path, p, environ);
#if _use_spawnveg
else if ((proc->pid = spawnveg(path, p, environ, proc->pgrp)) != -1)
else if ((proc->pid = spawnveg(path, p, environ, proc->pgrp, -1)) != -1)
goto cleanup;
#endif
if (errno != ENOEXEC)
Expand Down Expand Up @@ -773,7 +773,7 @@ sfsync(sfstderr);
execve(env + 2, p, environ);
#if _use_spawnveg
else
proc->pid = spawnveg(env + 2, p, environ, proc->pgrp);
proc->pid = spawnveg(env + 2, p, environ, proc->pgrp, -1);
#endif
cleanup:
if (forked)
Expand Down

0 comments on commit cce6d2a

Please sign in to comment.