In term_restore(), only restores the terminal if we are in the

foregroup process group.  Instead of calling tcgetpgrp(), which is
racy, we set a temporary handler for SIGTTOU and check whether it
was received after a failed call to tcsetattr().

diff --git a/common/term.c b/common/term.c
index b534c29829e66a063f501731c023d254fb2d4dda..e03b29b065afb15b4d81f2a456750de153033bdc 100644 (file)
--- a/common/term.c
+++ b/common/term.c
@@ -36,7 +36,9 @@
 # include <strings.h>
 #endif /* HAVE_STRINGS_H */
 #include <errno.h>
+#include <signal.h>
 #include <termios.h>
+#include <unistd.h>
 
 #include "missing.h"
 #include "sudo_debug.h"
@@ -70,24 +72,47 @@ static int changed;
 int term_erase;
 int term_kill;
 
+static volatile sig_atomic_t got_sigttou;
+
 /*
- * Like tcsetattr() but restarts on EINTR.
+ * SIGTTOU signal handler for term_restore that just sets a flag.
+ */
+static void sigttou(int signo)
+{
+    got_sigttou = 1;
+}
+
+/*
+ * Like tcsetattr() but restarts on EINTR _except_ for SIGTTOU.
  * Returns 0 on success or -1 on failure, setting errno.
+ * Sets got_sigttou on failure if interrupted by SIGTTOU.
  */
 static int
-tcsetattr_nointr(int fd, int flags, struct termios *tp)
+tcsetattr_nobg(int fd, int flags, struct termios *tp)
 {
+    sigaction_t sa, osa;
     int rc;
 
+    /*
+     * If we receive SIGTTOU from tcsetattr() it means we are
+     * not in the foreground process group.
+     * This should be less racy than using tcgetpgrp().
+     */
+    memset(&sa, 0, sizeof(sa));
+    sigemptyset(&sa.sa_mask);
+    sa.sa_handler = sigttou;
+    got_sigttou = 0;
+    sigaction(SIGTTOU, &sa, &osa);
     do {
        rc = tcsetattr(fd, flags, tp);
-    } while (rc != 0 && errno == EINTR);
+    } while (rc != 0 && errno == EINTR && !got_sigttou);
+    sigaction(SIGTTOU, &osa, NULL);
 
     return rc;
 }
 
 /*
- * Restore saved terminal settings.
+ * Restore saved terminal settings if we are in the foreground process group.
  * Returns true on success or false on failure.
  */
 bool
@@ -97,7 +122,7 @@ term_restore(int fd, bool flush)
 
     if (changed) {
        const int flags = flush ? (TCSASOFT|TCSAFLUSH) : (TCSASOFT|TCSADRAIN);
-       if (tcsetattr_nointr(fd, flags, &oterm) != 0)
+       if (tcsetattr_nobg(fd, flags, &oterm) != 0)
            debug_return_bool(false);
        changed = 0;
     }
@@ -113,6 +138,7 @@ term_noecho(int fd)
 {
     debug_decl(term_noecho, SUDO_DEBUG_UTIL)
 
+again:
     if (!changed && tcgetattr(fd, &oterm) != 0)
        debug_return_bool(false);
     (void) memcpy(&term, &oterm, sizeof(term));
@@ -120,10 +146,15 @@ term_noecho(int fd)
 #ifdef VSTATUS
     term.c_cc[VSTATUS] = _POSIX_VDISABLE;
 #endif
-    if (tcsetattr_nointr(fd, TCSADRAIN|TCSASOFT, &term) == 0) {
+    if (tcsetattr_nobg(fd, TCSADRAIN|TCSASOFT, &term) == 0) {
        changed = 1;
        debug_return_bool(true);
     }
+    if (got_sigttou) {
+       /* We were in the background, so oterm is probably bogus. */
+       kill(getpid(), SIGTTOU);
+       goto again;
+    }
     debug_return_bool(false);
 }
 
@@ -137,6 +168,7 @@ term_raw(int fd, int isig)
     struct termios term;
     debug_decl(term_raw, SUDO_DEBUG_UTIL)
 
+again:
     if (!changed && tcgetattr(fd, &oterm) != 0)
        return 0;
     (void) memcpy(&term, &oterm, sizeof(term));
@@ -148,10 +180,15 @@ term_raw(int fd, int isig)
     CLR(term.c_lflag, ECHO | ICANON | ISIG | IEXTEN);
     if (isig)
        SET(term.c_lflag, ISIG);
-    if (tcsetattr_nointr(fd, TCSADRAIN|TCSASOFT, &term) == 0) {
+    if (tcsetattr_nobg(fd, TCSADRAIN|TCSASOFT, &term) == 0) {
        changed = 1;
        debug_return_bool(true);
     }
+    if (got_sigttou) {
+       /* We were in the background, so oterm is probably bogus. */
+       kill(getpid(), SIGTTOU);
+       goto again;
+    }
     debug_return_bool(false);
 }
 
@@ -164,6 +201,7 @@ term_cbreak(int fd)
 {
     debug_decl(term_cbreak, SUDO_DEBUG_UTIL)
 
+again:
     if (!changed && tcgetattr(fd, &oterm) != 0)
        return 0;
     (void) memcpy(&term, &oterm, sizeof(term));
@@ -177,12 +215,17 @@ term_cbreak(int fd)
 #ifdef VSTATUS
     term.c_cc[VSTATUS] = _POSIX_VDISABLE;
 #endif
-    if (tcsetattr_nointr(fd, TCSADRAIN|TCSASOFT, &term) == 0) {
+    if (tcsetattr_nobg(fd, TCSADRAIN|TCSASOFT, &term) == 0) {
        term_erase = term.c_cc[VERASE];
        term_kill = term.c_cc[VKILL];
        changed = 1;
        debug_return_bool(true);
     }
+    if (got_sigttou) {
+       /* We were in the background, so oterm is probably bogus. */
+       kill(getpid(), SIGTTOU);
+       goto again;
+    }
     debug_return_bool(false);
 }
 
@@ -196,9 +239,15 @@ term_copy(int src, int dst)
     struct termios tt;
     debug_decl(term_copy, SUDO_DEBUG_UTIL)
 
+again:
     if (tcgetattr(src, &tt) != 0)
        debug_return_bool(false);
-    if (tcsetattr_nointr(dst, TCSANOW|TCSASOFT, &tt) != 0)
-       debug_return_bool(false);
-    debug_return_bool(true);
+    if (tcsetattr_nobg(dst, TCSANOW|TCSASOFT, &tt) == 0)
+       debug_return_bool(true);
+    if (got_sigttou) {
+       /* We were in the background, so oterm is probably bogus. */
+       kill(getpid(), SIGTTOU);
+       goto again;
+    }
+    debug_return_bool(false);
 }

diff --git a/src/exec_pty.c b/src/exec_pty.c
index 0287c5d1b37a0b99ca0525ece23973aeda3e1eb9..d2975d54a6aa10bcca7925fe784954329a9046c2 100644 (file)
--- a/src/exec_pty.c
+++ b/src/exec_pty.c
@@ -115,11 +115,8 @@ pty_cleanup(void)
 {
     debug_decl(cleanup, SUDO_DEBUG_EXEC);
 
-    if (!TAILQ_EMPTY(&io_plugins) && io_fds[SFD_USERTTY] != -1) {
-       check_foreground();
-       if (foreground)
-           term_restore(io_fds[SFD_USERTTY], 0);
-    }
+    if (!TAILQ_EMPTY(&io_plugins) && io_fds[SFD_USERTTY] != -1)
+       term_restore(io_fds[SFD_USERTTY], 0);
 #ifdef HAVE_SELINUX
     selinux_restore_tty();
 #endif
@@ -812,11 +809,8 @@ pty_close(struct command_status *cstat)
     }
 
     /* Restore terminal settings. */
-    if (io_fds[SFD_USERTTY] != -1) {
-       check_foreground();
-       if (foreground)
-           term_restore(io_fds[SFD_USERTTY], 0);
-    }
+    if (io_fds[SFD_USERTTY] != -1)
+       term_restore(io_fds[SFD_USERTTY], 0);
 
     /* If child was signalled, write the reason to stdout like the shell. */
     if (cstat->type == CMD_WSTATUS && WIFSIGNALED(cstat->val)) {