Fix a bug where iTerm2 would hang when a fd server exited before we attached...

Fix a bug where iTerm2 would hang when a fd server exited before we attached to it (which could happen if the command failed to exec or exited right away). Also fix a leak of file descriptors (serverConnectionFd).

sources/PTYSession.m

@@ -1994,6 +1994,12 @@ ITERM_WEAKLY_REFERENCEABLE
     });
 }
+- (void)taskDiedImmediately {
+    // Let initial creation finish, then report the broken pipe. This happens if the file descriptor
+    // server dies immediately.
+    [self performSelector:@selector(brokenPipe) withObject:nil afterDelay:0];
+}
+
 - (void)brokenPipe {
     if (_exited) {
         return;

sources/PTYTask.h

@@ -19,6 +19,9 @@ extern NSString *kCoprocessStatusChangeNotification;
 - (void)brokenPipe;  // Called in main thread
 - (void)taskWasDeregistered;
 - (void)writeForCoprocessOnlyTask:(NSData *)data;
+
+// Called on main thread from within launchWithPath:arguments:environment:width:height:isUTF8:.
+- (void)taskDiedImmediately;
 @end
 @interface PTYTask : NSObject

sources/PTYTask.m

@@ -35,6 +35,7 @@
 #include <util.h>
 #define CTRLKEY(c) ((c)-'A'+1)
+const int kNumFileDescriptorsToDup = 4;
 NSString *kCoprocessStatusChangeNotification = @"kCoprocessStatusChangeNotification";
@@ -271,42 +272,47 @@ static void HandleSigChld(int n) {
     [[TaskNotifier sharedInstance] registerTask:self];
 }
-// Like login_tty but makes fd 0 the master, fd 1 the slave, and fd 2 an open unix-domain socket
-// for transferring file descriptors.
-static void MyLoginTTY(int master, int slave, int serverSocketFd) {
+// Like login_tty but makes fd 0 the master, fd 1 the slave, fd 2 an open unix-domain socket
+// for transferring file descriptors, and fd 3 the write end of a pipe that closes when the server
+// dies.
+static void MyLoginTTY(int master, int slave, int serverSocketFd, int deadMansPipeWriteEnd) {
     setsid();
     ioctl(slave, TIOCSCTTY, NULL);
     // This array keeps track of which file descriptors are in use and should not be dup2()ed over.
-    // It has |inuseCount| valid elements.
-    int inuse[9] = { 0, 1, 2, master, slave, serverSocketFd, -1, -1, -1 };
-    int inuseCount = 6;
+    // It has |inuseCount| valid elements. inuse must have inuseCount + arraycount(orig) elements.
+    int inuse[3 * kNumFileDescriptorsToDup] = {
+       0, 1, 2, 3,  // FDs get duped to the lowest numbers so reserve them
+       master, slave, serverSocketFd, deadMansPipeWriteEnd,  // FDs to get duped, which mustn't be overwritten
+       -1, -1, -1, -1 };  // Space for temp values to ensure they don't get resused
+    int inuseCount = 2 * kNumFileDescriptorsToDup;
     // File descriptors get dup2()ed to temporary numbers first to avoid stepping on each other or
     // on any of the desired final values. Their temporary values go in here. The first is always
     // master, then slave, then server socket.
-    int temp[3];
+    int temp[kNumFileDescriptorsToDup];
     // The original file descriptors to renumber.
-    int orig[3] = { master, slave, serverSocketFd };
+    int orig[kNumFileDescriptorsToDup] = { master, slave, serverSocketFd, deadMansPipeWriteEnd };
     for (int o = 0; o < sizeof(orig) / sizeof(*orig); o++) {  // iterate over orig
         int original = orig[o];
-        // Try to find a temp value that doesn't belong to inuse
-        for (int t = 0; t < sizeof(inuse) / sizeof(*inuse); t++) {
+        // Try to find a candidate file descriptor that is not important to us (i.e., does not belong
+        // to the inuse array).
+        for (int candidate = 0; candidate < sizeof(inuse) / sizeof(*inuse); candidate++) {
             BOOL isInUse = NO;
             for (int i = 0; i < sizeof(inuse) / sizeof(*inuse); i++) {
-                if (inuse[i] == t) {
+                if (inuse[i] == candidate) {
                     isInUse = YES;
                     break;
                 }
             }
             if (!isInUse) {
                 // t is good. dup orig[o] to t and close orig[o]. Save t in temp[o].
-                inuse[inuseCount++] = t;
-                temp[o] = t;
-                dup2(original, t);
+                inuse[inuseCount++] = candidate;
+                temp[o] = candidate;
+                dup2(original, candidate);
                 close(original);
                 break;
             }
@@ -326,7 +332,8 @@ static int MyForkPty(int *amaster,
                      char *name,
                      struct termios *termp,
                      struct winsize *winp,
-                     int serverSocketFd) {
+                     int serverSocketFd,
+                     int deadMansPipeWriteEnd) {
     assert([iTermAdvancedSettingsModel runJobsInServers]);
     int master;
     int slave;
@@ -345,13 +352,15 @@ static int MyForkPty(int *amaster,
         case 0:
             // child
-            MyLoginTTY(master, slave, serverSocketFd);
+            MyLoginTTY(master, slave, serverSocketFd, deadMansPipeWriteEnd);
             return 0;
         default:
             // parent
             *amaster = master;
             close(slave);
+            close(serverSocketFd);
+            close(deadMansPipeWriteEnd);
             return pid;
     }
 }
@@ -471,11 +480,13 @@ static int MyForkPty(int *amaster,
     DLog(@"Preparing to launch a job. Command is %@ and args are %@", commandToExec, args);
     DLog(@"Environment is\n%@", env);
     char **newEnviron = [self environWithOverrides:env];
-
+    int deadMansPipe[2] = { 0, 0 };
+    
     // Note: stringByStandardizingPath will automatically call stringByExpandingTildeInPath.
     const char *initialPwd = [[[env objectForKey:@"PWD"] stringByStandardizingPath] UTF8String];
     pid_t pid;
     int connectionFd = -1;
+    int numFileDescriptorsToPreserve;
     if ([iTermAdvancedSettingsModel runJobsInServers]) {
         // Create a temporary filename for the unix domain socket. It'll only exist for a moment.
         NSString *tempPath = [[NSWorkspace sharedWorkspace] temporaryFileNameWithPrefix:@"iTerm2-temp-socket."
@@ -502,6 +513,7 @@ static int MyForkPty(int *amaster,
         // Connect to the server running in a thread.
         connectionFd = iTermFileDescriptorClientConnect(tempPath.UTF8String);
+        assert(connectionFd != -1);  // If this happens the block dispatched above never returns. Ran out of FDs, presumably.
         // Wait for serverConnectionFd to be written to.
         dispatch_semaphore_wait(semaphore, DISPATCH_TIME_FOREVER);
@@ -516,11 +528,17 @@ static int MyForkPty(int *amaster,
         // Now fork. This variant of forkpty passes through the master, slave,
         // and serverConnectionFd to the child job.
-        pid = _serverPid = MyForkPty(&fd, theTtyname, &term, &win, serverConnectionFd);
+        pipe(deadMansPipe);
+        
+        // This closes serverConnectionFd and deadMansPipe[1] in the parent process but not the child.
+        pid = _serverPid = MyForkPty(&fd, theTtyname, &term, &win, serverConnectionFd, deadMansPipe[1]);
+        numFileDescriptorsToPreserve = kNumFileDescriptorsToDup;
     } else {
         pid = _childPid = forkpty(&fd, theTtyname, &term, &win);
+        numFileDescriptorsToPreserve = 3;
     }
     if (pid == (pid_t)0) {
+        // Child
         // Do not start the new process with a signal handler.
         signal(SIGCHLD, SIG_DFL);
         signal(SIGPIPE, SIG_DFL);
@@ -531,7 +549,7 @@ static int MyForkPty(int *amaster,
         // Apple opens files without the close-on-exec flag (e.g., Extras2.rsrc).
         // See issue 2662.
-        for (int j = 3; j < getdtablesize(); j++) {
+        for (int j = numFileDescriptorsToPreserve; j < getdtablesize(); j++) {
             close(j);
         }
@@ -550,9 +568,10 @@ static int MyForkPty(int *amaster,
         sleep(1);
         _exit(-1);
     } else if (pid < (pid_t)0) {
+        // Error
         PtyTaskDebugLog(@"%@ %s", progpath, strerror(errno));
         NSRunCriticalAlertPanel(@"Unable to Fork!",
-                                @"iTerm cannot launch the program for this session.",
+                                @"iTerm2 cannot launch the program for this session.",
                                 @"OK",
                                 nil,
                                 nil);
@@ -576,7 +595,8 @@ static int MyForkPty(int *amaster,
         // really need the rest of the stuff in serverConnection since we already know
         // it, but that's ok.
         iTermFileDescriptorServerConnection serverConnection =
-            iTermFileDescriptorClientRead(connectionFd);
+            iTermFileDescriptorClientRead(connectionFd, deadMansPipe[0]);
+        close(deadMansPipe[0]);
         if (serverConnection.ok) {
             // We intentionally leave connectionFd open. If iTerm2 stops unexpectedly then its closure
             // lets the server know it should call accept(). We now have two copies of the master PTY
@@ -597,7 +617,7 @@ static int MyForkPty(int *amaster,
         } else {
             close(fd);
             NSLog(@"Server died immediately!");
-            [_delegate brokenPipe];
+            [_delegate taskDiedImmediately];
         }
     } else {
         // Jobs are direct children of iTerm2

sources/iTermFileDescriptorClient.c

@@ -18,7 +18,8 @@
 static ssize_t ReceiveMessageAndFileDescriptor(int fd,
                                                void *buffer,
                                                size_t bufferCapacity,
-                                               int *receivedFileDescriptorPtr) {
+                                               int *receivedFileDescriptorPtr,
+                                               int deadMansPipeReadEnd) {
     // Loop because sometimes the dynamic loader spews warnings (for example, when malloc logging
     // is enabled)
     while (1) {
@@ -42,6 +43,17 @@ static ssize_t ReceiveMessageAndFileDescriptor(int fd,
         do {
             // There used to be a race condition where the server would die
             // really early and then we'd get stuck in recvmsg. See issue 4383.
+            if (deadMansPipeReadEnd >= 0) {
+                syslog(LOG_NOTICE, "Calling select...");
+                int fds[2] = { fd, deadMansPipeReadEnd };
+                int readable[2];
+                iTermSelect(fds, 2, readable);
+                if (readable[1]) {
+                    syslog(LOG_NOTICE, "Server was dead before recevmsg. Did the shell terminate immediately?");
+                    return -1;
+                }
+                syslog(LOG_NOTICE, "assuming socket is readable");
+            }
             syslog(LOG_NOTICE, "calling recvmsg...");
             n = recvmsg(fd, &message, 0);
             syslog(LOG_NOTICE, "recvmsg returned %zd, errno=%s\n", n, (n < 0 ? strerror(errno) : "n/a"));
@@ -132,7 +144,7 @@ iTermFileDescriptorServerConnection iTermFileDescriptorClientRun(pid_t pid) {
         return result;
     }
-    iTermFileDescriptorServerConnection result = iTermFileDescriptorClientRead(socketFd);
+    iTermFileDescriptorServerConnection result = iTermFileDescriptorClientRead(socketFd, -1);
     result.serverPid = pid;
     syslog(LOG_NOTICE, "Success: process id is %d, pty master fd is %d\n\n",
            (int)pid, result.ptyMasterFd);
@@ -140,12 +152,13 @@ iTermFileDescriptorServerConnection iTermFileDescriptorClientRun(pid_t pid) {
     return result;
 }
-iTermFileDescriptorServerConnection iTermFileDescriptorClientRead(int socketFd) {
+iTermFileDescriptorServerConnection iTermFileDescriptorClientRead(int socketFd, int deadMansPipeReadEnd) {
     iTermFileDescriptorServerConnection result = { 0 };
     int rc = ReceiveMessageAndFileDescriptor(socketFd,
                                              &result.childPid,
                                              sizeof(result.childPid),
-                                             &result.ptyMasterFd);
+                                             &result.ptyMasterFd,
+                                             deadMansPipeReadEnd);
     if (rc == -1 || result.ptyMasterFd == -1) {
         result.error = "Failed to read message from server";
         close(socketFd);

sources/iTermFileDescriptorClient.h

@@ -20,9 +20,12 @@ iTermFileDescriptorServerConnection iTermFileDescriptorClientRun(pid_t pid);
 // Returns the file descriptor to a socket or -1. Follow this with a call to
 // iTermFileDescriptorClientRead().
 // This is used when the client and server connect prior to fork().
+// If `deadMansPipeReadEnd` is not negative, it should be the read side of a pipe that is never
+// written. If the server dies before writing to the socket, it'll become readable and we avoid
+// hanging on a socket that will never get written to.
 int iTermFileDescriptorClientConnect(const char *path);
 // Blocks and reads a result from the socket.
-iTermFileDescriptorServerConnection iTermFileDescriptorClientRead(int socketFd);
+iTermFileDescriptorServerConnection iTermFileDescriptorClientRead(int socketFd, int deadMansPipeReadEnd);
 #endif  // __ITERM_FILE_DESCRIPTOR_CLIENT_H

sources/iTermFileDescriptorServer.c

@@ -75,7 +75,7 @@ static void SigUsr1Handler(int arg) {
 }
-static int Select(int *fds, int count, int *results) {
+int iTermSelect(int *fds, int count, int *results) {
     int result;
     int theError;
     fd_set readset;
@@ -130,7 +130,7 @@ static int SendFileDescriptorAndWait(int connectionFd) {
     syslog(LOG_NOTICE, "All done. Waiting for client to disconnect or child to die.");
     int fds[2] = { gPipe[0], connectionFd };
     int results[2];
-    Select(fds, sizeof(fds) / sizeof(*fds), results);
+    iTermSelect(fds, sizeof(fds) / sizeof(*fds), results);
     syslog(LOG_NOTICE, "select returned. child dead=%d, connection closed=%d", results[0], results[1]);
     close(connectionFd);

sources/iTermFileDescriptorServer.h

@@ -22,4 +22,9 @@ int iTermFileDescriptorServerSocketBindListen(const char *path);
 // suitable to pass to iTermFileDescriptorServerRun() in |connectionFd|.
 int iTermFileDescriptorServerAccept(int socketFd);
+// Takes an array of file descriptors and its length as input. `results` should be an array of
+// equal length. On return, the readable FDs will have the corresponding value in `results` set to
+// true. Takes care of EINTR. Return value is number of readable FDs.
+int iTermSelect(int *fds, int count, int *results);
+
 #endif  // __ITERM_FILE_DESCRIPTOR_SERVER_H