github-mirror/fish-shell
github-mirror/fish-shell
2
0
Fork 0
mirror of https://github.com/fish-shell/fish-shell.git synced 2025-02-21 06:56:09 +08:00
Code Issues Actions Packages Projects Releases Wiki Activity

Don't attempt to unconditionally tcsetpgrp

Browse Source 
Setting the process group in a fork/exec scenario is a well-documented
race condition in pretty much any job control mechanism [0] [1]. The
Wikipedia article contradicts the glibc article and suggests that the
best approach is for the parent to wait for the child to become the
process group leader, while the glibc article suggests that both should
make it so (which is what fish did previously). However, I'm running
into cases where tcsetpgrp is causing an EPERM error, which it isn't
documented to do except if the session id for the calling process
differs from that of the target process group (which is never the case
in fish since they are all part of the same session), which should cause
a _different_ error (SIGTTOU to be sent to all members of the calling
process' group).

In all cases, this is easily remedied by checking if the process group
in question is already in control of the terimnal. There's still the
off-chance that in the time between we check that and the time that the
command completes that situation may have changed, but the parent
process is supposed to ignore the result of this call if it errors out.

[0]: https://en.wikipedia.org/wiki/Process_group
[1]: https://www.gnu.org/software/libc/manual/html_node/Launching-Jobs.html
This commit is contained in:
Mahmoud Al-Qudsi 2017-07-26 14:25:21 -05:00 committed by Kurtis Rader
parent 87394a9e0b
commit 0e9177b590
2 changed files with 44 additions and 18 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

43
src/postfork.cpp
View File

@ -106,24 +106,33 @@ bool set_child_group(job_t *j, process_t *p, int print_errors) {
}
if (j->get_flag(JOB_TERMINAL) && j->get_flag(JOB_FOREGROUND)) { //!OCLINT(early exit)
int result = -1;
errno = EINTR;
while (result == -1 && errno == EINTR) {
signal_block(true);
result = tcsetpgrp(STDIN_FILENO, j->pgid);
signal_unblock(true);
if (tcgetpgrp(STDIN_FILENO) == j->pgid) {
debug(4, L"Process group %d already has control of terminal\n", j->pgid);
}
if (result == -1) {
if (errno == ENOTTY) redirect_tty_output();
if (print_errors) {
char job_id_buff[64];
char command_buff[64];
format_long_safe(job_id_buff, j->job_id);
narrow_string_safe(command_buff, j->command_wcstr());
debug_safe(1, "Could not send job %s ('%s') to foreground", job_id_buff,
command_buff);
safe_perror("tcsetpgrp");
retval = false;
else {
debug(4, L"Attempting bring process group to foreground via tcsetpgrp for job->pgid %d\n", j->pgid);
debug(4, L"caller session id: %d, pgid %d has session id: %d\n", getsid(0), j->pgid, getsid(j->pgid));
int result = -1;
errno = EINTR;
while (result == -1 && errno == EINTR) {
signal_block(true);
result = tcsetpgrp(STDIN_FILENO, j->pgid);
signal_unblock(true);
}
if (result == -1) {
if (errno == ENOTTY) redirect_tty_output();
if (print_errors) {
char job_id_buff[64];
char command_buff[64];
char job_pgid_buff[128];
format_long_safe(job_id_buff, j->job_id);
narrow_string_safe(command_buff, j->command_wcstr());
format_long_safe(job_pgid_buff, j->pgid);
debug_safe(1, "Could not send job %s ('%s') with pgid %s to foreground", job_id_buff,
command_buff, job_pgid_buff);
safe_perror("tcsetpgrp");
retval = false;
}
}
}
}

19
src/proc.cpp
View File

@ -789,6 +789,23 @@ static bool terminal_give_to_job(job_t *j, int cont) {
return true;
}
//it may not be safe to call tcsetpgrp if we've already done so, as at that point we are no longer
//the controlling process group for the terminal and no longer have permission to set the process
//group that is in control, causing tcsetpgrp to return EPERM, even though that's not the documented
//behavior in tcsetpgrp(3), which instead says other bad things will happen (it says SIGTTOU will be
//sent to all members of the background *calling* process group, but it's more complicated than that,
//SIGTTOU may or may not be sent depending on the TTY configuration and whether or not signal handlers
//for SIGTTOU are installed. Read: http://curiousthing.org/sigttin-sigttou-deep-dive-linux
//In all cases, our goal here was just to hand over control of the terminal to this process group,
//which is a no-op if it's already been done.
if (tcgetpgrp(STDIN_FILENO) == j->pgid) {
debug(2, L"Process group %d already has control of terminal\n", j->pgid);
return true;
}
debug(4, L"Attempting bring process group to foreground via tcsetpgrp for job->pgid %d\n", j->pgid);
debug(4, L"caller session id: %d, pgid %d has session id: %d\n", getsid(0), j->pgid, getsid(j->pgid));
signal_block(true);
int result = -1;
errno = EINTR;
@ -797,7 +814,7 @@ static bool terminal_give_to_job(job_t *j, int cont) {
}
if (result == -1) {
if (errno == ENOTTY) redirect_tty_output();
debug(1, _(L"Could not send job %d ('%ls') to foreground"), j->job_id, j->command_wcstr());
debug(1, _(L"terminal_give_to_job(): Could not send job %d ('%ls') with pgid %d to foreground"), j->job_id, j->command_wcstr(), j->pgid);
wperror(L"tcsetpgrp");
signal_unblock(true);
return false;