Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

_winapi.CreateProcess (used by subprocess) is not thread-safe #75627

Closed
evanunderscore mannequin opened this issue Sep 13, 2017 · 4 comments
Closed

_winapi.CreateProcess (used by subprocess) is not thread-safe #75627

evanunderscore mannequin opened this issue Sep 13, 2017 · 4 comments
Assignees
Labels
3.7 (EOL) end of life 3.8 only security fixes OS-windows stdlib Python modules in the Lib dir

Comments

@evanunderscore
Copy link
Mannequin

evanunderscore mannequin commented Sep 13, 2017

BPO 31446
Nosy @pfmoore, @vstinner, @tjguk, @zware, @serhiy-storchaka, @zooba, @evanunderscore
PRs
  • bpo-31446: copy command line that should be passed to CreateProcessW #11141
  • [3.7] bpo-31446: Copy command line that should be passed to CreateProcessW(). (GH-11141). #11149
  • 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 = 'https://github.com/serhiy-storchaka'
    closed_at = <Date 2018-12-14.09:25:42.298>
    created_at = <Date 2017-09-13.09:59:03.191>
    labels = ['3.8', '3.7', 'library', 'OS-windows']
    title = '_winapi.CreateProcess (used by subprocess) is not thread-safe'
    updated_at = <Date 2018-12-14.09:25:42.297>
    user = 'https://github.com/evanunderscore'

    bugs.python.org fields:

    activity = <Date 2018-12-14.09:25:42.297>
    actor = 'serhiy.storchaka'
    assignee = 'serhiy.storchaka'
    closed = True
    closed_date = <Date 2018-12-14.09:25:42.298>
    closer = 'serhiy.storchaka'
    components = ['Library (Lib)', 'Windows']
    creation = <Date 2017-09-13.09:59:03.191>
    creator = 'evan_'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 31446
    keywords = ['patch']
    message_count = 4.0
    messages = ['302045', '302046', '331786', '331788']
    nosy_count = 7.0
    nosy_names = ['paul.moore', 'vstinner', 'tim.golden', 'zach.ware', 'serhiy.storchaka', 'steve.dower', 'evan_']
    pr_nums = ['11141', '11149']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue31446'
    versions = ['Python 3.7', 'Python 3.8']

    @evanunderscore
    Copy link
    Mannequin Author

    evanunderscore mannequin commented Sep 13, 2017

    The method used for spawning subprocesses on Windows is not thread-safe under certain circumstances. The following example demonstrates how this manifests:

        >>> import threading
        >>> import subprocess
        >>> for i in range(100):
        ...     threading.Thread(
        ...         target=subprocess.Popen,
        ...         args=('ping localhost',),
        ...         kwargs={'stdout': subprocess.DEVNULL},
        ...     ).start()
        ...
        Exception in thread Thread-1202:
        Traceback (most recent call last):
          File "C:\Program Files\Python36\lib\threading.py", line 916, in _bootstrap_inner
            self.run()
          File "C:\Program Files\Python36\lib\threading.py", line 864, in run
            self._target(*self._args, **self._kwargs)
          File "C:\Program Files\Python36\lib\subprocess.py", line 707, in __init__
            restore_signals, start_new_session)
          File "C:\Program Files\Python36\lib\subprocess.py", line 990, in _execute_child
            startupinfo)
        ValueError: embedded null character
    
        Exception in thread Thread-1206:
        Traceback (most recent call last):
          File "C:\Program Files\Python36\lib\threading.py", line 916, in _bootstrap_inner
            self.run()
          File "C:\Program Files\Python36\lib\threading.py", line 864, in run
            self._target(*self._args, **self._kwargs)
          File "C:\Program Files\Python36\lib\subprocess.py", line 707, in __init__
            restore_signals, start_new_session)
          File "C:\Program Files\Python36\lib\subprocess.py", line 990, in _execute_child
            startupinfo)
        ValueError: embedded null character
    >>>
    

    subprocess.Popen calls down to _winapi.CreateProcess, which calls CreateProcessW. When args is passed as a fixed string, the result of the argument conversion is attached to the object and shared by future calls into C code. However, the documentation for CreateProcess states:

    "The Unicode version of this function, CreateProcessW, can modify the contents of this string. Therefore, this parameter cannot be a pointer to read-only memory (such as a const variable or a literal string). If this parameter is a constant string, the function may cause an access violation." (Source: https://msdn.microsoft.com/en-us/library/windows/desktop/ms682425(v=vs.85).aspx)

    It appears CreateProcessW is briefly inserting null characters into the buffer, causing errors if that buffer is used elsewhere before it is changed back.

    The call to CreateProcessW using the shared buffer can be seen here:

    result = CreateProcessW(application_name,

    Note that this error does not occur when passing args as a list, as subprocess.list2cmdline creates a new (though identical) string for each invocation.

    One potential solution is to allocate a copy of command_line (the shared buffer) instead of using the original.

    @evanunderscore evanunderscore mannequin added 3.7 (EOL) end of life stdlib Python modules in the Lib dir labels Sep 13, 2017
    @vstinner
    Copy link
    Member

    I remove Python 3.3-3.5 from Python versions since these versions don't accept bugfixes anymore, only security fixes:
    https://devguide.python.org/#status-of-python-branches

    @asvetlov asvetlov added the 3.8 only security fixes label Dec 13, 2018
    @serhiy-storchaka serhiy-storchaka self-assigned this Dec 13, 2018
    @serhiy-storchaka
    Copy link
    Member

    New changeset 7b36016 by Serhiy Storchaka (Vladimir Matveev) in branch 'master':
    bpo-31446: Copy command line that should be passed to CreateProcessW(). (GH-11141)
    7b36016

    @serhiy-storchaka
    Copy link
    Member

    New changeset 922b2a0 by Serhiy Storchaka in branch '3.7':
    [3.7] bpo-31446: Copy command line that should be passed to CreateProcessW(). (GH-11141). (GH-11149)
    922b2a0

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 (EOL) end of life 3.8 only security fixes OS-windows stdlib Python modules in the Lib dir
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants