We shouldn't fork off a utmp helper subprocess when we aren't setuid,

because (a) under that circumstance we won't be writing to utmp
anyway, and (b) if we aren't setuid, then we won't have created the
pty at the point we fork, so even if our subprocess _could_ have
written to utmp it wouldn't have done it right!

Spotted by valgrind (triggering on the access beyond the end of the
ttyname string in setup_utmp, clueing me in to it having been empty).

[originally from svn r9309]

diff --git a/unix/uxpty.c b/unix/uxpty.c
index a9c0676caeaaa65b832560878d2bfebe70175530..afd64afb8385b49bf56215dab084cbc0b52ee0ea 100644 (file)
--- a/unix/uxpty.c
+++ b/unix/uxpty.c
@@ -167,8 +167,8 @@ static tree234 *ptys_by_pid = NULL;
 static Pty single_pty = NULL;
 
 #ifndef OMIT_UTMP
-static pid_t pty_utmp_helper_pid;
-static int pty_utmp_helper_pipe;
+static pid_t pty_utmp_helper_pid = -1;
+static int pty_utmp_helper_pipe = -1;
 static int pty_stamped_utmp;
 static struct utmpx utmp_entry;
 #endif
@@ -408,106 +408,106 @@ void pty_pre_init(void)
 
     if (geteuid() != getuid() || getegid() != getgid()) {
        pty_open_master(pty);
-    }
 
 #ifndef OMIT_UTMP
-    /*
-     * Fork off the utmp helper.
-     */
-    if (pipe(pipefd) < 0) {
-       perror("pterm: pipe");
-       exit(1);
-    }
-    cloexec(pipefd[0]);
-    cloexec(pipefd[1]);
-    pid = fork();
-    if (pid < 0) {
-       perror("pterm: fork");
-       exit(1);
-    } else if (pid == 0) {
-       char display[128], buffer[128];
-       int dlen, ret;
-
-       close(pipefd[1]);
-       /*
-        * Now sit here until we receive a display name from the
-        * other end of the pipe, and then stamp utmp. Unstamp utmp
-        * again, and exit, when the pipe closes.
-        */
+        /*
+         * Fork off the utmp helper.
+         */
+        if (pipe(pipefd) < 0) {
+            perror("pterm: pipe");
+            exit(1);
+        }
+        cloexec(pipefd[0]);
+        cloexec(pipefd[1]);
+        pid = fork();
+        if (pid < 0) {
+            perror("pterm: fork");
+            exit(1);
+        } else if (pid == 0) {
+            char display[128], buffer[128];
+            int dlen, ret;
+
+            close(pipefd[1]);
+            /*
+             * Now sit here until we receive a display name from the
+             * other end of the pipe, and then stamp utmp. Unstamp utmp
+             * again, and exit, when the pipe closes.
+             */
 
-       dlen = 0;
-       while (1) {
+            dlen = 0;
+            while (1) {
            
-           ret = read(pipefd[0], buffer, lenof(buffer));
-           if (ret <= 0) {
-               cleanup_utmp();
-               _exit(0);
-           } else if (!pty_stamped_utmp) {
-               if (dlen < lenof(display))
-                   memcpy(display+dlen, buffer,
-                          min(ret, lenof(display)-dlen));
-               if (buffer[ret-1] == '\0') {
-                   /*
-                    * Now we have a display name. NUL-terminate
-                    * it, and stamp utmp.
-                    */
-                   display[lenof(display)-1] = '\0';
-                   /*
-                    * Trap as many fatal signals as we can in the
-                    * hope of having the best possible chance to
-                    * clean up utmp before termination. We are
-                    * unfortunately unprotected against SIGKILL,
-                    * but that's life.
-                    */
-                   putty_signal(SIGHUP, fatal_sig_handler);
-                   putty_signal(SIGINT, fatal_sig_handler);
-                   putty_signal(SIGQUIT, fatal_sig_handler);
-                   putty_signal(SIGILL, fatal_sig_handler);
-                   putty_signal(SIGABRT, fatal_sig_handler);
-                   putty_signal(SIGFPE, fatal_sig_handler);
-                   putty_signal(SIGPIPE, fatal_sig_handler);
-                   putty_signal(SIGALRM, fatal_sig_handler);
-                   putty_signal(SIGTERM, fatal_sig_handler);
-                   putty_signal(SIGSEGV, fatal_sig_handler);
-                   putty_signal(SIGUSR1, fatal_sig_handler);
-                   putty_signal(SIGUSR2, fatal_sig_handler);
+                ret = read(pipefd[0], buffer, lenof(buffer));
+                if (ret <= 0) {
+                    cleanup_utmp();
+                    _exit(0);
+                } else if (!pty_stamped_utmp) {
+                    if (dlen < lenof(display))
+                        memcpy(display+dlen, buffer,
+                               min(ret, lenof(display)-dlen));
+                    if (buffer[ret-1] == '\0') {
+                        /*
+                         * Now we have a display name. NUL-terminate
+                         * it, and stamp utmp.
+                         */
+                        display[lenof(display)-1] = '\0';
+                        /*
+                         * Trap as many fatal signals as we can in the
+                         * hope of having the best possible chance to
+                         * clean up utmp before termination. We are
+                         * unfortunately unprotected against SIGKILL,
+                         * but that's life.
+                         */
+                        putty_signal(SIGHUP, fatal_sig_handler);
+                        putty_signal(SIGINT, fatal_sig_handler);
+                        putty_signal(SIGQUIT, fatal_sig_handler);
+                        putty_signal(SIGILL, fatal_sig_handler);
+                        putty_signal(SIGABRT, fatal_sig_handler);
+                        putty_signal(SIGFPE, fatal_sig_handler);
+                        putty_signal(SIGPIPE, fatal_sig_handler);
+                        putty_signal(SIGALRM, fatal_sig_handler);
+                        putty_signal(SIGTERM, fatal_sig_handler);
+                        putty_signal(SIGSEGV, fatal_sig_handler);
+                        putty_signal(SIGUSR1, fatal_sig_handler);
+                        putty_signal(SIGUSR2, fatal_sig_handler);
 #ifdef SIGBUS
-                   putty_signal(SIGBUS, fatal_sig_handler);
+                        putty_signal(SIGBUS, fatal_sig_handler);
 #endif
 #ifdef SIGPOLL
-                   putty_signal(SIGPOLL, fatal_sig_handler);
+                        putty_signal(SIGPOLL, fatal_sig_handler);
 #endif
 #ifdef SIGPROF
-                   putty_signal(SIGPROF, fatal_sig_handler);
+                        putty_signal(SIGPROF, fatal_sig_handler);
 #endif
 #ifdef SIGSYS
-                   putty_signal(SIGSYS, fatal_sig_handler);
+                        putty_signal(SIGSYS, fatal_sig_handler);
 #endif
 #ifdef SIGTRAP
-                   putty_signal(SIGTRAP, fatal_sig_handler);
+                        putty_signal(SIGTRAP, fatal_sig_handler);
 #endif
 #ifdef SIGVTALRM
-                   putty_signal(SIGVTALRM, fatal_sig_handler);
+                        putty_signal(SIGVTALRM, fatal_sig_handler);
 #endif
 #ifdef SIGXCPU
-                   putty_signal(SIGXCPU, fatal_sig_handler);
+                        putty_signal(SIGXCPU, fatal_sig_handler);
 #endif
 #ifdef SIGXFSZ
-                   putty_signal(SIGXFSZ, fatal_sig_handler);
+                        putty_signal(SIGXFSZ, fatal_sig_handler);
 #endif
 #ifdef SIGIO
-                   putty_signal(SIGIO, fatal_sig_handler);
+                        putty_signal(SIGIO, fatal_sig_handler);
 #endif
-                   setup_utmp(pty->name, display);
-               }
-           }
-       }
-    } else {
-       close(pipefd[0]);
-       pty_utmp_helper_pid = pid;
-       pty_utmp_helper_pipe = pipefd[1];
-    }
+                        setup_utmp(pty->name, display);
+                    }
+                }
+            }
+        } else {
+            close(pipefd[0]);
+            pty_utmp_helper_pid = pid;
+            pty_utmp_helper_pipe = pipefd[1];
+        }
 #endif
+    }
 
     /* Drop privs. */
     {
@@ -735,7 +735,7 @@ static const char *pty_init(void *frontend, void **backend_handle, Conf *conf,
     if (!conf_get_int(conf, CONF_stamp_utmp)) {
        close(pty_utmp_helper_pipe);   /* just let the child process die */
        pty_utmp_helper_pipe = -1;
-    } else {
+    } else if (pty_utmp_helper_pipe >= 0) {
        char *location = get_x_display(pty->frontend);
        int len = strlen(location)+1, pos = 0;   /* +1 to include NUL */
        while (pos < len) {