classification
Title: _winapi.CreateProcess (used by subprocess) is not thread-safe
Type: Stage: resolved
Components: Library (Lib), Windows Versions: Python 3.8, Python 3.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: serhiy.storchaka Nosy List: evan_, paul.moore, serhiy.storchaka, steve.dower, tim.golden, vstinner, zach.ware
Priority: normal Keywords: patch

Created on 2017-09-13 09:59 by evan_, last changed 2018-12-14 09:25 by serhiy.storchaka. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 11141 merged v2m, 2018-12-13 07:00
PR 11149 merged serhiy.storchaka, 2018-12-14 08:40
Messages (4)
msg302045 - (view) Author: Evan Andrews (evan_) * Date: 2017-09-13 09:59
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: https://github.com/python/cpython/blob/b8f4163da30e16c7cd58fe04f4b17e38d53cd57e/Modules/_winapi.c#L879

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.
msg302046 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-09-13 10:01
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
msg331786 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-12-14 08:30
New changeset 7b36016a15aeed0d76a4c05a66203e6d7723aace by Serhiy Storchaka (Vladimir Matveev) in branch 'master':
bpo-31446: Copy command line that should be passed to CreateProcessW(). (GH-11141)
https://github.com/python/cpython/commit/7b36016a15aeed0d76a4c05a66203e6d7723aace
msg331788 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-12-14 09:18
New changeset 922b2a0d0d9928af420ea4d5c104f8be72517aa2 by Serhiy Storchaka in branch '3.7':
[3.7] bpo-31446: Copy command line that should be passed to CreateProcessW(). (GH-11141). (GH-11149)
https://github.com/python/cpython/commit/922b2a0d0d9928af420ea4d5c104f8be72517aa2
History
Date User Action Args
2018-12-14 09:25:42serhiy.storchakasetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2018-12-14 09:18:21serhiy.storchakasetmessages: + msg331788
2018-12-14 08:40:12serhiy.storchakasetpull_requests: + pull_request10378
2018-12-14 08:30:57serhiy.storchakasetmessages: + msg331786
2018-12-13 11:17:34serhiy.storchakasetassignee: serhiy.storchaka

nosy: + serhiy.storchaka
2018-12-13 11:07:56asvetlovsetversions: + Python 3.8, - Python 3.6
2018-12-13 07:00:38v2msetkeywords: + patch
stage: patch review
pull_requests: + pull_request10371
2017-09-13 10:01:19vstinnersetnosy: + vstinner

messages: + msg302046
versions: - Python 3.3, Python 3.4, Python 3.5
2017-09-13 10:00:27vstinnersetnosy: + tim.golden, steve.dower, zach.ware, paul.moore
components: + Windows
2017-09-13 09:59:03evan_create