From 14b389cb467d4c3eb5715b0c8be98922c5a027cc Mon Sep 17 00:00:00 2001
From: David Wilson <dw@botanicus.net>
Date: Sat, 3 Nov 2018 15:49:52 +0000
Subject: [PATCH] issue #406: don't leak FDs on failed child start.

---
 docs/changelog.rst |  3 +++
 mitogen/parent.py  | 16 ++++++++++++++--
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/docs/changelog.rst b/docs/changelog.rst
index d9746375..781c42b9 100644
--- a/docs/changelog.rst
+++ b/docs/changelog.rst
@@ -210,6 +210,9 @@ Core Library
   :class:`mitogen.core.Broker` did not call :meth:`mitogen.core.Poller.close`
   during shutdown, leaking the underlying poller FD in masters and parents.
 
+* `#406 <https://github.com/dw/mitogen/issues/406>`_: connections could leak
+  FDs when a child process failed to start.
+
 * `#411 <https://github.com/dw/mitogen/issues/411>`_: the SSH method typed
   "``y``" rather than the requisite "``yes``" when `check_host_keys="accept"`
   was configured. This would lead to connection timeouts due to the hung
diff --git a/mitogen/parent.py b/mitogen/parent.py
index 780cecd7..3e30f475 100644
--- a/mitogen/parent.py
+++ b/mitogen/parent.py
@@ -211,7 +211,7 @@ def create_socketpair():
     return parentfp, childfp
 
 
-def detach_popen(*args, **kwargs):
+def detach_popen(close_on_error=None, **kwargs):
     """
     Use :class:`subprocess.Popen` to construct a child process, then hack the
     Popen so that it forgets the child it created, allowing it to survive a
@@ -223,6 +223,8 @@ def detach_popen(*args, **kwargs):
     delivered to this process, causing later 'legitimate' calls to fail with
     ECHILD.
 
+    :param list close_on_error:
+        Array of integer file descriptors to close on exception.
     :returns:
         Process ID of the new child.
     """
@@ -230,7 +232,13 @@ def detach_popen(*args, **kwargs):
     # handling, without tying the surrounding code into managing a Popen
     # object, which isn't possible for at least :mod:`mitogen.fork`. This
     # should be replaced by a swappable helper class in a future version.
-    proc = subprocess.Popen(*args, **kwargs)
+    try:
+        proc = subprocess.Popen(**kwargs)
+    except Exception:
+        for fd in close_on_error or ():
+            os.close(fd)
+        raise
+
     proc._child_created = False
     return proc.pid
 
@@ -277,6 +285,7 @@ def create_child(args, merge_stdio=False, stderr_pipe=False, preexec_fn=None):
         stdout=childfp,
         close_fds=True,
         preexec_fn=preexec_fn,
+        close_on_error=[parentfp.fileno(), childfp.fileno()],
         **extra
     )
     if stderr_pipe:
@@ -344,6 +353,7 @@ def tty_create_child(args):
         stdout=slave_fd,
         stderr=slave_fd,
         preexec_fn=_acquire_controlling_tty,
+        close_on_error=[master_fd, slave_fd],
         close_fds=True,
     )
 
@@ -372,6 +382,7 @@ def hybrid_tty_create_child(args):
     mitogen.core.set_block(childfp)
     disable_echo(master_fd)
     disable_echo(slave_fd)
+
     pid = detach_popen(
         args=args,
         stdin=childfp,
@@ -379,6 +390,7 @@ def hybrid_tty_create_child(args):
         stderr=slave_fd,
         preexec_fn=_acquire_controlling_tty,
         close_fds=True,
+        close_on_error=[master_fd, slave_fd, parentfp.fileno(), childfp.fileno()],
     )
 
     os.close(slave_fd)