bpo-35721: Close socket pair if Popen in _UnixSubprocessTransport fails (GH-11553)

This slightly expands an existing test case `test_popen_error` to trigger a `ResourceWarning` and fixes it.

https://bugs.python.org/issue35721

diff --git a/Lib/asyncio/unix_events.py b/Lib/asyncio/unix_events.py
index 73d4bda7c2341b7c90fb071b5b8ecdef4c6a3dac..1aa3b396086c59a479453c5ec6ca209a3e7960e2 100644 (file)
--- a/Lib/asyncio/unix_events.py
+++ b/Lib/asyncio/unix_events.py
@@ -757,12 +757,18 @@ class _UnixSubprocessTransport(base_subprocess.BaseSubprocessTransport):
             # other end).  Notably this is needed on AIX, and works
             # just fine on other platforms.
             stdin, stdin_w = socket.socketpair()
-        self._proc = subprocess.Popen(
-            args, shell=shell, stdin=stdin, stdout=stdout, stderr=stderr,
-            universal_newlines=False, bufsize=bufsize, **kwargs)
-        if stdin_w is not None:
-            stdin.close()
-            self._proc.stdin = open(stdin_w.detach(), 'wb', buffering=bufsize)
+        try:
+            self._proc = subprocess.Popen(
+                args, shell=shell, stdin=stdin, stdout=stdout, stderr=stderr,
+                universal_newlines=False, bufsize=bufsize, **kwargs)
+            if stdin_w is not None:
+                stdin.close()
+                self._proc.stdin = open(stdin_w.detach(), 'wb', buffering=bufsize)
+                stdin_w = None
+        finally:
+            if stdin_w is not None:
+                stdin.close()
+                stdin_w.close()
 
 
 class AbstractChildWatcher:

diff --git a/Lib/test/test_asyncio/test_subprocess.py b/Lib/test/test_asyncio/test_subprocess.py
index 3908aabf5a13213a9c3681816a005012e3d6e23d..551974a1806a842605ba9552b6bf00e9e3944d6b 100644 (file)
--- a/Lib/test/test_asyncio/test_subprocess.py
+++ b/Lib/test/test_asyncio/test_subprocess.py
@@ -468,9 +468,7 @@ class SubprocessMixin:
                 isinstance(self, SubprocessFastWatcherTests)):
             asyncio.get_child_watcher()._callbacks.clear()
 
-    def test_popen_error(self):
-        # Issue #24763: check that the subprocess transport is closed
-        # when BaseSubprocessTransport fails
+    def _test_popen_error(self, stdin):
         if sys.platform == 'win32':
             target = 'asyncio.windows_utils.Popen'
         else:
@@ -480,12 +478,23 @@ class SubprocessMixin:
             popen.side_effect = exc
 
             create = asyncio.create_subprocess_exec(sys.executable, '-c',
-                                                    'pass', loop=self.loop)
+                                                    'pass', stdin=stdin,
+                                                    loop=self.loop)
             with warnings.catch_warnings(record=True) as warns:
                 with self.assertRaises(exc):
                     self.loop.run_until_complete(create)
                 self.assertEqual(warns, [])
 
+    def test_popen_error(self):
+        # Issue #24763: check that the subprocess transport is closed
+        # when BaseSubprocessTransport fails
+        self._test_popen_error(stdin=None)
+
+    def test_popen_error_with_stdin_pipe(self):
+        # Issue #35721: check that newly created socket pair is closed when
+        # Popen fails
+        self._test_popen_error(stdin=subprocess.PIPE)
+
     def test_read_stdout_after_process_exit(self):
 
         async def execute():

diff --git a/Misc/ACKS b/Misc/ACKS
index 8b3232551e1b09738f0f34f456f834ac74f32971..77f19909b3005b70507b277078bb4f657f006a8b 100644 (file)
--- a/Misc/ACKS
+++ b/Misc/ACKS
@@ -486,6 +486,7 @@ Florian Festi
 John Feuerstein
 Carl Feynman
 Vincent Fiack
+Niklas Fiekas
 Anastasia Filatova
 Tomer Filiba
 Segev Finer

diff --git a/Misc/NEWS.d/next/Library/2019-01-18-16-23-00.bpo-35721.d8djAJ.rst b/Misc/NEWS.d/next/Library/2019-01-18-16-23-00.bpo-35721.d8djAJ.rst
new file mode 100644 (file)
index 0000000..5af4b1b
--- /dev/null
+++ b/Misc/NEWS.d/next/Library/2019-01-18-16-23-00.bpo-35721.d8djAJ.rst
@@ -0,0 +1,3 @@
+Fix :meth:`asyncio.SelectorEventLoop.subprocess_exec()` leaks file descriptors
+if ``Popen`` fails and called with ``stdin=subprocess.PIPE``.
+Patch by Niklas Fiekas.