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 602ba004d3be1a887ad1ddd8188884583449bc18..470f7a069a2ac86d1b6b740747310f1bdcd41382 100644 (file)
--- 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 ae41282cf47c367eb39146a0527a00925fa8bc5b..9beeb9b41e181cc5f866f8f3afadceb53e66887c 100644 (file)
--- 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 26821aafb3e8780d4d49ef76eb07bbd70e4d647c..592d46e7891ae4cfaa37697dba1e990242ac4ea4 100644 (file)
--- 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);
     }