isa/sudo
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Don't loop over read/write, recv/send or tcgetpgrp/tcsetpgrp trying

Browse Source 
to handle EINTR.  We now use SA_RESTART with signals so this is not
needed and is potentially dangerous if it is possible to receive
SIGTTIN or SIGTTOU (which it currently is not).
This commit is contained in:
Todd C. Miller
2017-11-30 09:53:21 -07:00
parent 9298a2a42e
commit b9adb3dd51
3 changed files with 29 additions and 42 deletions
Download Patch File Download Diff File Expand all files Collapse all files

52
src/exec_monitor.c
View File

@@ -70,7 +70,6 @@ static void
deliver_signal(struct monitor_closure *mc, int signo, bool from_parent) deliver_signal(struct monitor_closure *mc, int signo, bool from_parent)
{ {
char signame[SIG2STR_MAX]; char signame[SIG2STR_MAX];
int status;
debug_decl(deliver_signal, SUDO_DEBUG_EXEC); debug_decl(deliver_signal, SUDO_DEBUG_EXEC);
/* Avoid killing more than a single process or process group. */ /* Avoid killing more than a single process or process group. */
@@ -93,16 +92,20 @@ deliver_signal(struct monitor_closure *mc, int signo, bool from_parent)
break; break;
case SIGCONT_FG: case SIGCONT_FG:
/* Continue in foreground, grant it controlling tty. */ /* Continue in foreground, grant it controlling tty. */
do { if (tcsetpgrp(io_fds[SFD_SLAVE], mc->cmnd_pgrp) == -1) {
status = tcsetpgrp(io_fds[SFD_SLAVE], mc->cmnd_pgrp); sudo_debug_printf(SUDO_DEBUG_ERROR|SUDO_DEBUG_ERRNO,
} while (status == -1 && errno == EINTR); "%s: unable to set foreground pgrp to %d (command)",
__func__, (int)mc->cmnd_pgrp);
}
killpg(mc->cmnd_pid, SIGCONT); killpg(mc->cmnd_pid, SIGCONT);
break; break;
case SIGCONT_BG: case SIGCONT_BG:
/* Continue in background, I take controlling tty. */ /* Continue in background, I take controlling tty. */
do { if (tcsetpgrp(io_fds[SFD_SLAVE], mc->mon_pgrp) == -1) {
status = tcsetpgrp(io_fds[SFD_SLAVE], mc->mon_pgrp); sudo_debug_printf(SUDO_DEBUG_ERROR|SUDO_DEBUG_ERRNO,
} while (status == -1 && errno == EINTR); "%s: unable to set foreground pgrp to %d (monitor)",
__func__, (int)mc->mon_pgrp);
}
killpg(mc->cmnd_pid, SIGCONT); killpg(mc->cmnd_pid, SIGCONT);
break; break;
case SIGKILL: case SIGKILL:
@@ -130,12 +133,10 @@ send_status(int fd, struct command_status *cstat)
sudo_debug_printf(SUDO_DEBUG_INFO, sudo_debug_printf(SUDO_DEBUG_INFO,
"sending status message to parent: [%d, %d]", "sending status message to parent: [%d, %d]",
cstat->type, cstat->val); cstat->type, cstat->val);
do { n = send(fd, cstat, sizeof(*cstat), 0);
n = send(fd, cstat, sizeof(*cstat), 0);
} while (n == -1 && errno == EINTR);
if (n != sizeof(*cstat)) { if (n != sizeof(*cstat)) {
sudo_debug_printf(SUDO_DEBUG_ERROR, sudo_debug_printf(SUDO_DEBUG_ERROR|SUDO_DEBUG_ERRNO,
"unable to send status to parent: %s", strerror(errno)); "%s: unable to send status to parent", __func__);
} }
cstat->type = CMD_INVALID; /* prevent re-sending */ cstat->type = CMD_INVALID; /* prevent re-sending */
} }
@@ -203,9 +204,7 @@ mon_handle_sigchld(struct monitor_closure *mc)
mc->cstat->val = status; mc->cstat->val = status;
if (WIFSTOPPED(status)) { if (WIFSTOPPED(status)) {
/* Save the foreground pgid so we can restore it later. */ /* Save the foreground pgid so we can restore it later. */
do { pid = tcgetpgrp(io_fds[SFD_SLAVE]);
pid = tcgetpgrp(io_fds[SFD_SLAVE]);
} while (pid == -1 && errno == EINTR);
if (pid != mc->mon_pgrp) if (pid != mc->mon_pgrp)
mc->cmnd_pgrp = pid; mc->cmnd_pgrp = pid;
send_status(mc->backchannel, mc->cstat); send_status(mc->backchannel, mc->cstat);
@@ -271,13 +270,10 @@ mon_errpipe_cb(int fd, int what, void *v)
* Read errno from child or EOF when command is executed. * Read errno from child or EOF when command is executed.
* Note that the error pipe is *blocking*. * Note that the error pipe is *blocking*.
*/ */
do { nread = read(fd, &errval, sizeof(errval));
nread = read(fd, &errval, sizeof(errval));
} while (nread == -1 && errno == EINTR);
switch (nread) { switch (nread) {
case -1: case -1:
if (errno != EAGAIN) { if (errno != EAGAIN && errno != EINTR) {
if (mc->cstat->val == CMD_INVALID) { if (mc->cstat->val == CMD_INVALID) {
/* XXX - need a way to distinguish non-exec error. */ /* XXX - need a way to distinguish non-exec error. */
mc->cstat->type = CMD_ERRNO; mc->cstat->type = CMD_ERRNO;
@@ -567,10 +563,8 @@ exec_monitor(struct command_details *details, sigset_t *oset,
/* setup tty and exec command */ /* setup tty and exec command */
exec_cmnd_pty(details, foreground, errpipe[1]); exec_cmnd_pty(details, foreground, errpipe[1]);
while (write(errpipe[1], &errno, sizeof(int)) == -1) { if (write(errpipe[1], &errno, sizeof(int)) == -1)
if (errno != EINTR) sudo_warn(U_("unable to execute %s"), details->command);
break;
}
_exit(1); _exit(1);
} }
close(errpipe[1]); close(errpipe[1]);
@@ -609,11 +603,11 @@ exec_monitor(struct command_details *details, sigset_t *oset,
/* Make the command the foreground process for the pty slave. */ /* Make the command the foreground process for the pty slave. */
if (foreground && !ISSET(details->flags, CD_EXEC_BG)) { if (foreground && !ISSET(details->flags, CD_EXEC_BG)) {
int n; if (tcsetpgrp(io_fds[SFD_SLAVE], mc.cmnd_pgrp) == -1) {
sudo_debug_printf(SUDO_DEBUG_ERROR|SUDO_DEBUG_ERRNO,
do { "%s: unable to set foreground pgrp to %d (command)",
n = tcsetpgrp(io_fds[SFD_SLAVE], mc.cmnd_pgrp); __func__, (int)mc.cmnd_pgrp);
} while (n == -1 && errno == EINTR); }
} }
/* /*

7
src/exec_nopty.c
View File

@@ -73,13 +73,10 @@ errpipe_cb(int fd, int what, void *v)
* Read errno from child or EOF when command is executed. * Read errno from child or EOF when command is executed.
* Note that the error pipe is *blocking*. * Note that the error pipe is *blocking*.
*/ */
do { nread = read(fd, &errval, sizeof(errval));
nread = read(fd, &errval, sizeof(errval));
} while (nread == -1 && errno == EINTR);
switch (nread) { switch (nread) {
case -1: case -1:
if (errno != EAGAIN) { if (errno != EAGAIN && errno != EINTR) {
if (ec->cstat->val == CMD_INVALID) { if (ec->cstat->val == CMD_INVALID) {
/* XXX - need a way to distinguish non-exec error. */ /* XXX - need a way to distinguish non-exec error. */
ec->cstat->type = CMD_ERRNO; ec->cstat->type = CMD_ERRNO;

12
src/exec_pty.c
View File

@@ -832,8 +832,6 @@ backchannel_cb(int fd, int what, void *v)
case -1: case -1:
switch (errno) { switch (errno) {
case EINTR: case EINTR:
/* Should not happen now that we use SA_RESTART. */
continue;
case EAGAIN: case EAGAIN:
/* Nothing ready. */ /* Nothing ready. */
break; break;
@@ -1042,11 +1040,9 @@ sigfwd_cb(int sock, int what, void *v)
"sending SIG%s to monitor over backchannel", signame); "sending SIG%s to monitor over backchannel", signame);
cstat.type = CMD_SIGNO; cstat.type = CMD_SIGNO;
cstat.val = sigfwd->signo; cstat.val = sigfwd->signo;
do {
nsent = send(sock, &cstat, sizeof(cstat), 0);
} while (nsent == -1 && errno == EINTR);
TAILQ_REMOVE(&ec->sigfwd_list, sigfwd, entries); TAILQ_REMOVE(&ec->sigfwd_list, sigfwd, entries);
free(sigfwd); free(sigfwd);
nsent = send(sock, &cstat, sizeof(cstat), 0);
if (nsent != sizeof(cstat)) { if (nsent != sizeof(cstat)) {
if (errno == EPIPE) { if (errno == EPIPE) {
sudo_debug_printf(SUDO_DEBUG_ERROR, sudo_debug_printf(SUDO_DEBUG_ERROR,
@@ -1433,9 +1429,9 @@ exec_pty(struct command_details *details, struct command_status *cstat)
exec_monitor(details, &oset, foreground && !pipeline, sv[1]); exec_monitor(details, &oset, foreground && !pipeline, sv[1]);
cstat->type = CMD_ERRNO; cstat->type = CMD_ERRNO;
cstat->val = errno; cstat->val = errno;
while (send(sv[1], cstat, sizeof(*cstat), 0) == -1) { if (send(sv[1], cstat, sizeof(*cstat), 0) == -1) {
if (errno != EINTR) sudo_debug_printf(SUDO_DEBUG_ERROR|SUDO_DEBUG_ERRNO,
break; "%s: unable to send status to parent", __func__);
} }
_exit(1); _exit(1);
} }