[Python-checkins] bpo-33929: multiprocessing: fix handle leak on race condition (GH-7921)

[Victor Stinner]

https://github.com/python/cpython/commit/2cc9d21fffb8146d30e6fb4221e32410ba4b4ab7
commit: 2cc9d21fffb8146d30e6fb4221e32410ba4b4ab7
branch: master
author: Victor Stinner <vstinner at redhat.com>
committer: GitHub <noreply at github.com>
date: 2018-06-27T11:40:24+02:00
summary:

bpo-33929: multiprocessing: fix handle leak on race condition (GH-7921)

Fix a race condition in Popen of
multiprocessing.popen_spawn_win32. The child process now duplicates
the read end of pipe instead of "stealing" it.

Previously, the read end of pipe was "stolen" by the child process,
but it leaked a handle if the child process had been terminated
before it could steal the handle from the parent process.

files:
A Misc/NEWS.d/next/Library/2018-06-26-02-09-18.bpo-33929.OcCLah.rst
M Lib/multiprocessing/popen_spawn_win32.py
M Lib/multiprocessing/reduction.py
M Lib/multiprocessing/spawn.py

diff --git a/Lib/multiprocessing/popen_spawn_win32.py b/Lib/multiprocessing/popen_spawn_win32.py
index 3e42e9c33874..3b92c8a2b4ae 100644
--- a/Lib/multiprocessing/popen_spawn_win32.py
+++ b/Lib/multiprocessing/popen_spawn_win32.py
@@ -18,6 +18,12 @@
 WINEXE = (sys.platform == 'win32' and getattr(sys, 'frozen', False))
 WINSERVICE = sys.executable.lower().endswith("pythonservice.exe")
 
+
+def _close_handles(*handles):
+    for handle in handles:
+        _winapi.CloseHandle(handle)
+
+
 #
 # We define a Popen class similar to the one from subprocess, but
 # whose constructor takes a process object as its argument.
@@ -32,8 +38,12 @@ class Popen(object):
     def __init__(self, process_obj):
         prep_data = spawn.get_preparation_data(process_obj._name)
 
-        # read end of pipe will be "stolen" by the child process
+        # read end of pipe will be duplicated by the child process
         # -- see spawn_main() in spawn.py.
+        #
+        # bpo-33929: Previously, the read end of pipe was "stolen" by the child
+        # process, but it leaked a handle if the child process had been
+        # terminated before it could steal the handle from the parent process.
         rhandle, whandle = _winapi.CreatePipe(None, 0)
         wfd = msvcrt.open_osfhandle(whandle, 0)
         cmd = spawn.get_command_line(parent_pid=os.getpid(),
@@ -56,7 +66,8 @@ def __init__(self, process_obj):
             self.returncode = None
             self._handle = hp
             self.sentinel = int(hp)
-            self.finalizer = util.Finalize(self, _winapi.CloseHandle, (self.sentinel,))
+            self.finalizer = util.Finalize(self, _close_handles,
+                                           (self.sentinel, int(rhandle)))
 
             # send information to child
             set_spawning_popen(self)
diff --git a/Lib/multiprocessing/reduction.py b/Lib/multiprocessing/reduction.py
index deca19ccad7b..473fd59df61b 100644
--- a/Lib/multiprocessing/reduction.py
+++ b/Lib/multiprocessing/reduction.py
@@ -68,12 +68,16 @@ def dump(obj, file, protocol=None):
     __all__ += ['DupHandle', 'duplicate', 'steal_handle']
     import _winapi
 
-    def duplicate(handle, target_process=None, inheritable=False):
+    def duplicate(handle, target_process=None, inheritable=False,
+                  *, source_process=None):
         '''Duplicate a handle.  (target_process is a handle not a pid!)'''
+        current_process = _winapi.GetCurrentProcess()
+        if source_process is None:
+            source_process = current_process
         if target_process is None:
-            target_process = _winapi.GetCurrentProcess()
+            target_process = current_process
         return _winapi.DuplicateHandle(
-            _winapi.GetCurrentProcess(), handle, target_process,
+            source_process, handle, target_process,
             0, inheritable, _winapi.DUPLICATE_SAME_ACCESS)
 
     def steal_handle(source_pid, handle):
diff --git a/Lib/multiprocessing/spawn.py b/Lib/multiprocessing/spawn.py
index 1f4f3f496f51..2de4cb7f6378 100644
--- a/Lib/multiprocessing/spawn.py
+++ b/Lib/multiprocessing/spawn.py
@@ -96,7 +96,15 @@ def spawn_main(pipe_handle, parent_pid=None, tracker_fd=None):
     assert is_forking(sys.argv), "Not forking"
     if sys.platform == 'win32':
         import msvcrt
-        new_handle = reduction.steal_handle(parent_pid, pipe_handle)
+        import _winapi
+
+        if parent_pid is not None:
+            source_process = _winapi.OpenProcess(
+                _winapi.PROCESS_DUP_HANDLE, False, parent_pid)
+        else:
+            source_process = None
+        new_handle = reduction.duplicate(pipe_handle,
+                                         source_process=source_process)
         fd = msvcrt.open_osfhandle(new_handle, os.O_RDONLY)
     else:
         from . import semaphore_tracker
diff --git a/Misc/NEWS.d/next/Library/2018-06-26-02-09-18.bpo-33929.OcCLah.rst b/Misc/NEWS.d/next/Library/2018-06-26-02-09-18.bpo-33929.OcCLah.rst
new file mode 100644
index 000000000000..6ddb17c81cd9
--- /dev/null
+++ b/Misc/NEWS.d/next/Library/2018-06-26-02-09-18.bpo-33929.OcCLah.rst
@@ -0,0 +1,5 @@
+multiprocessing: Fix a race condition in Popen of
+multiprocessing.popen_spawn_win32. The child process now duplicates the read
+end of pipe instead of "stealing" it. Previously, the read end of pipe was
+"stolen" by the child process, but it leaked a handle if the child process had
+been terminated before it could steal the handle from the parent process.