From: Todd C. Miller Date: Thu, 30 Nov 2017 16:53:21 +0000 (-0700) Subject: Don't loop over read/write, recv/send or tcgetpgrp/tcsetpgrp trying X-Git-Tag: SUDO_1_8_22^2~60 X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=b9adb3dd51d582c1e7144b177ca00f47692e6d45;p=sudo Don't loop over read/write, recv/send or tcgetpgrp/tcsetpgrp trying 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). --- diff --git a/src/exec_monitor.c b/src/exec_monitor.c index 602ba004d..470f7a069 100644 --- a/src/exec_monitor.c +++ b/src/exec_monitor.c @@ -70,7 +70,6 @@ static void deliver_signal(struct monitor_closure *mc, int signo, bool from_parent) { char signame[SIG2STR_MAX]; - int status; debug_decl(deliver_signal, SUDO_DEBUG_EXEC); /* 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; case SIGCONT_FG: /* Continue in foreground, grant it controlling tty. */ - do { - status = tcsetpgrp(io_fds[SFD_SLAVE], mc->cmnd_pgrp); - } while (status == -1 && errno == EINTR); + if (tcsetpgrp(io_fds[SFD_SLAVE], mc->cmnd_pgrp) == -1) { + sudo_debug_printf(SUDO_DEBUG_ERROR|SUDO_DEBUG_ERRNO, + "%s: unable to set foreground pgrp to %d (command)", + __func__, (int)mc->cmnd_pgrp); + } killpg(mc->cmnd_pid, SIGCONT); break; case SIGCONT_BG: /* Continue in background, I take controlling tty. */ - do { - status = tcsetpgrp(io_fds[SFD_SLAVE], mc->mon_pgrp); - } while (status == -1 && errno == EINTR); + if (tcsetpgrp(io_fds[SFD_SLAVE], mc->mon_pgrp) == -1) { + sudo_debug_printf(SUDO_DEBUG_ERROR|SUDO_DEBUG_ERRNO, + "%s: unable to set foreground pgrp to %d (monitor)", + __func__, (int)mc->mon_pgrp); + } killpg(mc->cmnd_pid, SIGCONT); break; case SIGKILL: @@ -130,12 +133,10 @@ send_status(int fd, struct command_status *cstat) sudo_debug_printf(SUDO_DEBUG_INFO, "sending status message to parent: [%d, %d]", cstat->type, cstat->val); - do { - n = send(fd, cstat, sizeof(*cstat), 0); - } while (n == -1 && errno == EINTR); + n = send(fd, cstat, sizeof(*cstat), 0); if (n != sizeof(*cstat)) { - sudo_debug_printf(SUDO_DEBUG_ERROR, - "unable to send status to parent: %s", strerror(errno)); + sudo_debug_printf(SUDO_DEBUG_ERROR|SUDO_DEBUG_ERRNO, + "%s: unable to send status to parent", __func__); } cstat->type = CMD_INVALID; /* prevent re-sending */ } @@ -203,9 +204,7 @@ mon_handle_sigchld(struct monitor_closure *mc) mc->cstat->val = status; if (WIFSTOPPED(status)) { /* Save the foreground pgid so we can restore it later. */ - do { - pid = tcgetpgrp(io_fds[SFD_SLAVE]); - } while (pid == -1 && errno == EINTR); + pid = tcgetpgrp(io_fds[SFD_SLAVE]); if (pid != mc->mon_pgrp) mc->cmnd_pgrp = pid; 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. * Note that the error pipe is *blocking*. */ - do { - nread = read(fd, &errval, sizeof(errval)); - } while (nread == -1 && errno == EINTR); - + nread = read(fd, &errval, sizeof(errval)); switch (nread) { case -1: - if (errno != EAGAIN) { + if (errno != EAGAIN && errno != EINTR) { if (mc->cstat->val == CMD_INVALID) { /* XXX - need a way to distinguish non-exec error. */ mc->cstat->type = CMD_ERRNO; @@ -567,10 +563,8 @@ exec_monitor(struct command_details *details, sigset_t *oset, /* setup tty and exec command */ exec_cmnd_pty(details, foreground, errpipe[1]); - while (write(errpipe[1], &errno, sizeof(int)) == -1) { - if (errno != EINTR) - break; - } + if (write(errpipe[1], &errno, sizeof(int)) == -1) + sudo_warn(U_("unable to execute %s"), details->command); _exit(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. */ if (foreground && !ISSET(details->flags, CD_EXEC_BG)) { - int n; - - do { - n = tcsetpgrp(io_fds[SFD_SLAVE], mc.cmnd_pgrp); - } while (n == -1 && errno == EINTR); + if (tcsetpgrp(io_fds[SFD_SLAVE], mc.cmnd_pgrp) == -1) { + sudo_debug_printf(SUDO_DEBUG_ERROR|SUDO_DEBUG_ERRNO, + "%s: unable to set foreground pgrp to %d (command)", + __func__, (int)mc.cmnd_pgrp); + } } /* diff --git a/src/exec_nopty.c b/src/exec_nopty.c index ae41282cf..9beeb9b41 100644 --- a/src/exec_nopty.c +++ b/src/exec_nopty.c @@ -73,13 +73,10 @@ errpipe_cb(int fd, int what, void *v) * Read errno from child or EOF when command is executed. * Note that the error pipe is *blocking*. */ - do { - nread = read(fd, &errval, sizeof(errval)); - } while (nread == -1 && errno == EINTR); - + nread = read(fd, &errval, sizeof(errval)); switch (nread) { case -1: - if (errno != EAGAIN) { + if (errno != EAGAIN && errno != EINTR) { if (ec->cstat->val == CMD_INVALID) { /* XXX - need a way to distinguish non-exec error. */ ec->cstat->type = CMD_ERRNO; diff --git a/src/exec_pty.c b/src/exec_pty.c index 26821aafb..592d46e78 100644 --- a/src/exec_pty.c +++ b/src/exec_pty.c @@ -832,8 +832,6 @@ backchannel_cb(int fd, int what, void *v) case -1: switch (errno) { case EINTR: - /* Should not happen now that we use SA_RESTART. */ - continue; case EAGAIN: /* Nothing ready. */ break; @@ -1042,11 +1040,9 @@ sigfwd_cb(int sock, int what, void *v) "sending SIG%s to monitor over backchannel", signame); cstat.type = CMD_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); free(sigfwd); + nsent = send(sock, &cstat, sizeof(cstat), 0); if (nsent != sizeof(cstat)) { if (errno == EPIPE) { 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]); cstat->type = CMD_ERRNO; cstat->val = errno; - while (send(sv[1], cstat, sizeof(*cstat), 0) == -1) { - if (errno != EINTR) - break; + if (send(sv[1], cstat, sizeof(*cstat), 0) == -1) { + sudo_debug_printf(SUDO_DEBUG_ERROR|SUDO_DEBUG_ERRNO, + "%s: unable to send status to parent", __func__); } _exit(1); }