Incorrect Argument Order for Calls to _winapi.DuplicateHandle() in multiprocessing.reduction.DupHandle

[m3rc1fulcameron]

BPO	38188
Nosy	@pfmoore, @tjguk, @zware, @eryksun, @zooba

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = None
created_at = <Date 2019-09-16.17:51:37.160>
labels = ['type-bug', '3.8', '3.9', '3.10', 'library', 'OS-windows']
title = 'Incorrect Argument Order for Calls to _winapi.DuplicateHandle() in multiprocessing.reduction.DupHandle'
updated_at = <Date 2021-03-22.00:46:22.682>
user = 'https://bugs.python.org/m3rc1fulcameron'

bugs.python.org fields:

activity = <Date 2021-03-22.00:46:22.682>
actor = 'eryksun'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Library (Lib)', 'Windows']
creation = <Date 2019-09-16.17:51:37.160>
creator = 'm3rc1fulcameron'
dependencies = []
files = []
hgrepos = []
issue_num = 38188
keywords = []
message_count = 3.0
messages = ['352560', '352610', '352615']
nosy_count = 6.0
nosy_names = ['paul.moore', 'tim.golden', 'zach.ware', 'eryksun', 'steve.dower', 'm3rc1fulcameron']
pr_nums = []
priority = 'normal'
resolution = None
stage = 'needs patch'
status = 'open'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue38188'
versions = ['Python 3.8', 'Python 3.9', 'Python 3.10']

[m3rc1fulcameron]

The DuplicateHandle function is utilized by the DupHandle object to duplicate handles for the purpose of sending and receiving between processes on Windows systems. At least on Python 3.7.3, this function is invoked with an incorrect argument order. In multiprocessing.reduction, send_handle passes _winapi.DUPLICATE_SAME_ACCESS as the access argument to the DupHandle constructor, which in-turn passes it to the access argument for _winapi.DuplicateHandle(). Instead, per https://docs.microsoft.com/en-us/windows/win32/api/handleapi/nf-handleapi-duplicatehandle this constant should be passed into the options argument. This bug results in any handles communicated via this method to have meaningless permissions set, which makes them unusable.

I've monkeypatched the issue with the following code:

try:
import _winapi
log = logging.getLogger('')
log.warning('Patching multiprocessing.reduction to deal with the _winapi.DuplicateHandle() PROCESS_DUP_HANDLE argument order bug.')
class _PatchedDupHandle(object):
'''Picklable wrapper for a handle.'''
def __init__(self, handle, access, pid=None, options=0):
if pid is None:
# We just duplicate the handle in the current process and
# let the receiving process steal the handle.
pid = os.getpid()
proc = _winapi.OpenProcess(_winapi.PROCESS_DUP_HANDLE, False, pid)
try:
self._handle = _winapi.DuplicateHandle(
_winapi.GetCurrentProcess(),
handle, proc, access, False, options)
finally:
_winapi.CloseHandle(proc)
self._options = options
self._access = access
self._pid = pid

        def detach(self):
            '''Get the handle.  This should only be called once.'''
            # retrieve handle from process which currently owns it
            if self._pid == os.getpid():
                # The handle has already been duplicated for this process.
                return self._handle
            # We must steal the handle from the process whose pid is self._pid.
            proc = _winapi.OpenProcess(_winapi.PROCESS_DUP_HANDLE, False,
                                       self._pid)
            try:
                return _winapi.DuplicateHandle(
                    proc, self._handle, _winapi.GetCurrentProcess(),
                    self._access, False, self._options|_winapi.DUPLICATE_CLOSE_SOURCE)
            finally:
                _winapi.CloseHandle(proc)
    DupHandle = _PatchedDupHandle
    def _patched_send_handle(conn, handle, destination_pid):
        '''Send a handle over a local connection.'''
        dh = DupHandle(handle, 0, destination_pid, _winapi.DUPLICATE_SAME_ACCESS)
        conn.send(dh)
    send_handle=_patched_send_handle
except ImportError:
    pass

The above seems to fix the problem on my machine by adding an additional options property to the DupHandle object and an options argument to send_handle function.

[eryksun]

As far as I can tell, reduction.send_handle isn't used internally in the Windows implementation, and it's also not a documented API function. However, it is tested on Windows in test_fd_transfer in Lib/test/_test_multiprocessing.py. As it turns out, the bug that Cameron's proposed solution fixes slips under the radar. By coincidence, DUPLICATE_SAME_ACCESS (2) has the same value as the file access right FILE_WRITE_DATA (2), and test_fd_transfer only checks whether the child can write to a file handle.

I propose adding test_fd_transfer_windows to _TestConnection in Lib/test/_test_multiprocessing.py, which will test whether the parent and child are granted the same access to a kernel file object after the handle is sent to the child.

    @classmethod
    def _check_handle_access(cls, conn):
        handle = reduction.recv_handle(conn)
        conn.send(get_handle_info(handle).GrantedAccess)

@unittest.skipUnless(HAS_REDUCTION, "test needs multiprocessing.reduction")
@unittest.skipIf(sys.platform != "win32", "Windows-only test")
def test_fd_transfer_windows(self):
    if self.TYPE != 'processes':
        self.skipTest("only makes sense with processes")
    conn, child_conn = self.Pipe(duplex=True)
    p = self.Process(target=self._check_handle_access, args=(child_conn,))
    p.daemon = True
    p.start()
    try:
        with open(test.support.TESTFN, "wb") as f:
            self.addCleanup(test.support.unlink, test.support.TESTFN)
            handle = msvcrt.get_osfhandle(f.fileno())
            parent_access = get_handle_info(handle).GrantedAccess
            reduction.send_handle(conn, handle, p.pid)
            child_access = conn.recv()
            self.assertEqual(parent_access, child_access)
    finally:
        p.join()

get_handle_info() and the required ctypes support definitions [1] would be defined at module scope as follows:

    if WIN32:
        from ctypes import (WinDLL, WinError, Structure, POINTER, 
                            byref, sizeof, c_void_p, c_ulong)
        ntdll = WinDLL('ntdll')
        
        ntdll.NtQueryObject.argtypes = (
            c_void_p, # Handle
            c_ulong,  # ObjectInformationClass
            c_void_p, # ObjectInformation
            c_ulong,  # ObjectInformationLength
            POINTER(c_ulong)) # ReturnLength

        ObjectBasicInformation = 0

        class PUBLIC_OBJECT_BASIC_INFORMATION(Structure):
            (r"https://docs.microsoft.com/en-us/windows/win32/api"
             r"/winternl/nf-winternl-ntqueryobject")
            _fields_ = (('Attributes', c_ulong),
                        ('GrantedAccess', c_ulong),
                        ('HandleCount', c_ulong),
                        ('PointerCount', c_ulong),
                        ('Reserved', c_ulong * 10))


        def get_handle_info(handle):
            info = PUBLIC_OBJECT_BASIC_INFORMATION()
            status = ntdll.NtQueryObject(handle, ObjectBasicInformation,
                        byref(info), sizeof(info), None)
            if status < 0:
                error = ntdll.RtlNtStatusToDosError(status)
                raise WinError(error)
            return info

[1] https://docs.microsoft.com/en-us/windows/win32/api/winternl/nf-winternl-ntqueryobject

[eryksun]

Let's make test_fd_transfer_windows a bit less hangy by polling for up to 60 seconds instead of simply trying to recv() and by terminating before trying to join().

@unittest.skipUnless(HAS_REDUCTION, "test needs multiprocessing.reduction")
@unittest.skipIf(sys.platform != "win32", "Windows-only test")
def test_fd_transfer_windows(self):
    if self.TYPE != 'processes':
        self.skipTest("only makes sense with processes")
    conn, child_conn = self.Pipe(duplex=True)
    p = self.Process(target=self._check_handle_access, args=(child_conn,))
    p.daemon = True
    p.start()
    try:
        with open(test.support.TESTFN, "wb") as f:
            self.addCleanup(test.support.unlink, test.support.TESTFN)
            handle = msvcrt.get_osfhandle(f.fileno())
            parent_access = get_handle_info(handle).GrantedAccess
            reduction.send_handle(conn, handle, p.pid)
            if not conn.poll(timeout=60):
                raise AssertionError("could not receive from child process")
            child_access = conn.recv()
            self.assertEqual(parent_access, child_access)
    finally:
        p.terminate()
        p.join()