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
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/[email protected]/msg00717.html
  • Loading branch information
JohnoKing committed Jan 26, 2022
1 parent 7f4935e commit fac59b1
Show file tree
Hide file tree
Showing 6 changed files with 71 additions and 14 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 the POSIX_SPAWN_TCSETPGROUP flag.
*/
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
36 changes: 30 additions & 6 deletions src/lib/libast/comp/spawnveg.c
Original file line number Diff line number Diff line change
Expand Up @@ -44,23 +44,44 @@ NoN(spawnveg)
#include <error.h>
#include <wait.h>

#if !_lib_posix_spawn
#error "DEBUG ERROR 1"
#elif !_lib_posix_spawnattr_tcsetpgrp_np
#error "DEBUG ERROR 2"
#elif !POSIX_SPAWN_TCSETPGROUP
#error "DEBUG ERROR 3"
#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)
{
int err;
int err, flags = 0;
pid_t pid;
posix_spawnattr_t attr;

if (err = posix_spawnattr_init(&attr))
goto nope;
if (pgid)
{
flags |= POSIX_SPAWN_SETPGROUP;
#if _lib_posix_spawnattr_tcsetpgrp_np
if (tcfd >= 0)
flags |= POSIX_SPAWN_TCSETPGROUP;
#endif
#if POSIX_SPAWN_SETSID
if (pgid == -1)
flags |= POSIX_SPAWN_SETSID;
#endif
if (err = posix_spawnattr_setflags(&attr, flags))
goto bad;
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 +120,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 +141,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 +179,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 +194,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
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
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 fac59b1

Please sign in to comment.