Fix file descriptor leak in iTerm2. Issue 9430

* Close file descriptor when iTermFileDescriptorMultiClientChild gets dealloced. This fixes a file descriptor leak. * Invoke all wait callbacks from handleWait:. Failing to call the one added in fileDescriptorMultiClient:childDidTerminate: caused a leak of iTermFileDescriptorMultiClientChild. * Add a call to brokenPipe in finishAttaching:task: for jobs that terminated while iTerm2 was not running. This ensures you get the "session ended" banner on terminated restored sessions and orphans. * Add support for tracing iTermCallback.
gnachman · Jan 25, 2021 · 7102048 · 7102048
1 parent 1b66798
commit 7102048
Show file tree

Hide file tree

Showing 7 changed files with 44 additions and 32 deletions.
diff --git a/sources/iTermFileDescriptorMultiClient.m b/sources/iTermFileDescriptorMultiClient.m
@@ -627,12 +627,14 @@ - (void)waitForChild:(iTermFileDescriptorMultiClientChild *)child
     [child addWaitCallback:waitCallback];
 
     __weak __typeof(self) weakSelf = self;
-    [self send:&message state:state callback:[_thread newCallbackWithBlock:^(iTermFileDescriptorMultiClientState *state,
+    iTermCallback<id, NSNumber *> *sendCallback =
+    [_thread newCallbackWithBlock:^(iTermFileDescriptorMultiClientState *state,
                                                                              NSNumber *value) {
         [weakSelf didWriteWaitRequestWithStatus:value.boolValue
                                           child:child
                                           state:state];
-    }]];
+    }];
+    [self send:&message state:state callback:sendCallback];
 }
 
 - (void)didWriteWaitRequestWithStatus:(BOOL)sendOK
@@ -667,7 +669,7 @@ - (void)handleWait:(iTermMultiServerResponseWait)wait
     } else {
         result = [iTermResult withObject:@(wait.status)];
     }
-    [child invokeWaitCallback:result];
+    [child invokeAllWaitCallbacks:result];
 }
 
 #pragma mark - Termination

diff --git a/sources/iTermFileDescriptorMultiClientChild.h b/sources/iTermFileDescriptorMultiClientChild.h
@@ -35,7 +35,6 @@ NS_ASSUME_NONNULL_BEGIN
 - (instancetype)init NS_UNAVAILABLE;
 
 - (void)addWaitCallback:(iTermCallback<id, iTermResult<NSNumber *> *> *)callback;
-- (void)invokeWaitCallback:(iTermResult<NSNumber *> *)status;
 - (void)invokeAllWaitCallbacks:(iTermResult<NSNumber *> *)status;
 
 // Must be called on child's thread.

diff --git a/sources/iTermFileDescriptorMultiClientChild.m b/sources/iTermFileDescriptorMultiClientChild.m
@@ -59,6 +59,13 @@ - (instancetype)initWithReport:(iTermMultiServerReportChild *)report
     return self;
 }
 
+- (void)dealloc {
+    if (_fd >= 0) {
+        close(_fd);
+        _fd = -1;
+    }
+}
+
 - (NSString *)description {
     return [NSString stringWithFormat:@"<%@: %p pid=%@ fd=%@>", NSStringFromClass(self.class),
             self, @(self.pid), @(self.fd)];
@@ -124,18 +131,6 @@ - (void)addWaitCallback:(iTermCallback<id, iTermResult<NSNumber *> *> *)callback
     }];
 }
 
-- (void)invokeWaitCallback:(iTermResult<NSNumber *> *)status {
-    __block iTermCallback<id, iTermResult<NSNumber *> *> *callback;
-    [_thread dispatchRecursiveSync:^(id _Nonnull state) {
-        callback = _callbacks.firstObject;
-        if (!callback) {
-            return;
-        }
-        [_callbacks removeObjectAtIndex:0];
-    }];
-    [callback invokeWithObject:status];
-}
-
 - (void)invokeAllWaitCallbacks:(iTermResult<NSNumber *> *)status {
     __block NSArray<iTermCallback<id, iTermResult<NSNumber *> *> *> *callbacks;
     [_thread dispatchRecursiveSync:^(id _Nonnull state) {

diff --git a/sources/iTermMultiServerConnection.m b/sources/iTermMultiServerConnection.m
@@ -411,24 +411,25 @@ - (void)fileDescriptorMultiClientDidClose:(iTermFileDescriptorMultiClient *)clie
 - (void)fileDescriptorMultiClient:(iTermFileDescriptorMultiClient *)client
                 childDidTerminate:(iTermFileDescriptorMultiClientChild *)child {
     const pid_t pid = child.pid;
+    iTermCallback *callback = [iTermThread.main newCallbackWithBlock:^(iTermMainThreadState *_Nonnull state,
+                                                                       iTermResult<NSNumber *> *result) {
+             [result handleObject:
+              ^(NSNumber * _Nonnull statusNumber) {
+                 DLog(@"Child with pid %d terminated with status %d", pid, statusNumber.intValue);
+
+                 // Post a notification that causes the task to be removed from the task notifier. Note that
+                 // this is usually unnecessary because the file descriptor will return 0 before we finish
+                 // round-tripping on Wait. This is a backstop in case of the unexpected.
+                 [[iTermMultiServerChildDidTerminateNotification notificationWithProcessID:child.pid
+                                                                         terminationStatus:child.terminationStatus] post];
+             } error:
+              ^(NSError * _Nonnull error) {
+                 DLog(@"Failed to wait on child with pid %d: %@", pid, error);
+             }];
+    }];
     [client waitForChild:child
       removePreemptively:NO
-                callback:[iTermThread.main newCallbackWithBlock:^(iTermMainThreadState *_Nonnull state,
-                                                                  iTermResult<NSNumber *> *result) {
-        [result handleObject:
-         ^(NSNumber * _Nonnull statusNumber) {
-            DLog(@"Child with pid %d terminated with status %d", pid, statusNumber.intValue);
-
-            // Post a notification that causes the task to be removed from the task notifier. Note that
-            // this is usually unnecessary because the file descriptor will return 0 before we finish
-            // round-tripping on Wait. This is a backstop in case of the unexpected.
-            [[iTermMultiServerChildDidTerminateNotification notificationWithProcessID:child.pid
-                                                                    terminationStatus:child.terminationStatus] post];
-        } error:
-         ^(NSError * _Nonnull error) {
-            DLog(@"Failed to wait on child with pid %d: %@", pid, error);
-        }];
-    }]];
+                callback:callback];
 }
 
 @end
diff --git a/sources/iTermMultiServerJobManager.m b/sources/iTermMultiServerJobManager.m
@@ -400,6 +400,9 @@ - (iTermJobManagerAttachResults)finishAttaching:(iTermMultiServerJobManagerParti
             results |= iTermJobManagerAttachResultsRegistered;
         }
     }
+    if (result.brokenPipe) {
+        [task brokenPipe];
+    }
     return results;
 }
 
@@ -559,6 +562,7 @@ - (void)killWithMode:(iTermJobManagerKillingMode)mode
               removePreemptively:YES
                         callback:[self.thread newCallbackWithBlock:^(iTermMultiServerJobManagerState *state,
                                                                      iTermResult<NSNumber *> *result) {
+            // NOTE: killWithMode:state: must be idempotent. Be careful here.
             DLog(@"Preemptive wait for %d finished with result %@", pid, result);
         }]];
     }

diff --git a/sources/iTermThreadSafety.h b/sources/iTermThreadSafety.h
@@ -90,6 +90,9 @@ NS_ASSUME_NONNULL_BEGIN
 // Combines a block and an iTermThread to run it on.
 @interface iTermCallback<__contravariant CallerState, __covariant ObjectType>: NSObject
 @property (nonatomic, readonly) iTermThread *thread;
+#if DEBUG
+@property (atomic) BOOL trace;
+#endif
 
 + (instancetype)onThread:(iTermThread *)thread block:(void (^)(CallerState state, ObjectType _Nullable result))block;
 - (void)invokeWithObject:(ObjectType _Nullable)object;

diff --git a/sources/iTermThreadSafety.m b/sources/iTermThreadSafety.m
@@ -337,6 +337,9 @@ - (void)dealloc {
 }
 
 - (void)invokeWithObject:(id)object {
+#if DEBUG
+    BOOL trace = self.trace;
+#endif
     void (^block)(id, id) = [_block retain];
     [self retain];
 #if CHECK_DOUBLE_INVOKES
@@ -348,6 +351,11 @@ - (void)invokeWithObject:(id)object {
                      _invokeStack, stack, _creationStack, _debugInfo);
         _invokeStack = [stack copy];
         [stack release];
+#endif
+#if DEBUG
+        if (trace) {
+            NSLog(@"%@", stack);
+        }
 #endif
         block(state, object);
         [block release];