gh-137335: Fix unlikely name conflicts for named pipes in multiprocessing and asyncio on Windows#137389
Conversation
…processing and asyncio on Windows Since os.stat() raises an OSError for existing named pipe "\\.\pipe\...", os.path.exists() always returns False for it, and tempfile.mktemp() can return a name that matches an existing named pipe. So, tempfile.mktemp() cannot be used to generate unique names for named pipes. Instead, CreateNamedPipe() should be called in a loop with different names until it completes successfully.
Lib/asyncio/windows_utils.py
Outdated
| address, openmode, _winapi.PIPE_WAIT, | ||
| 1, obsize, ibsize, _winapi.NMPWAIT_WAIT_FOREVER, _winapi.NULL) | ||
| while True: | ||
| address = r'\\.\pipe\python-pipe-' + random.randbytes(8).hex() |
There was a problem hiding this comment.
A comment:
we could use os.urandom(8).hex() but this would only reduce the number of loop iterations we would do until we find a suitable name. OTOH, this makes normal cases much slower (usually, we are not in the presence of an adversary that is trying to create pipes...), so we would be only doing the loop once.
Now, I'm actually worried that it if we're able to interact with the process that is creating the pipes, then we could actually recover enough samples from the underlying PRNG instance and get the original seed. But this is only if 1) there are no random calls in between and 2) we can get 624 consecutive 32-bit samples from the random source.
Condition 2) already holds because random.randbytes(8) is actually equivalent to two calls of random.randbytes(4) that are concatenated. Since reverting MT-19937 requires only consecutive 624 32-bit words, this is the same as doing 312 pipe creations where we don't have calls to random.* in between (and then inspect the named file). This could be possible in practice, especially if we're talking about the multiprocessing and the asyncio components which could have some interactivness (e.g., a server).
Therefore, I would still suggest using os.urandom(8).hex() even if it slows down the calls.
There was a problem hiding this comment.
Good catch, I did not thought about this.
Sorry for not answering immediately, I missed your comment.
kumaraditya303
left a comment
There was a problem hiding this comment.
asyncio changes LGTM
|
LGTM too. I'll be happy to see the |
| break | ||
| except OSError as e: | ||
| if e.winerror not in (_winapi.ERROR_PIPE_BUSY, | ||
| _winapi.ERROR_ACCESS_DENIED): |
There was a problem hiding this comment.
an infinite loop retrying on this error could potentially hide an error if the system simply doesn't allow this process to create a pipe with the given address? (here and below)
There was a problem hiding this comment.
I do not know if this is even possible (\\.\pipe\ is not a real directory which you can forbid access to), but it will not harm if limit the number of attempts.
|
Thanks @serhiy-storchaka for the PR 🌮🎉.. I'm working now to backport this PR to: 3.13, 3.14. |
…processing and asyncio on Windows (pythonGH-137389) Since os.stat() raises an OSError for existing named pipe "\\.\pipe\...", os.path.exists() always returns False for it, and tempfile.mktemp() can return a name that matches an existing named pipe. So, tempfile.mktemp() cannot be used to generate unique names for named pipes. Instead, CreateNamedPipe() should be called in a loop with different names until it completes successfully. (cherry picked from commit d6a71f4) Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
…processing and asyncio on Windows (pythonGH-137389) Since os.stat() raises an OSError for existing named pipe "\\.\pipe\...", os.path.exists() always returns False for it, and tempfile.mktemp() can return a name that matches an existing named pipe. So, tempfile.mktemp() cannot be used to generate unique names for named pipes. Instead, CreateNamedPipe() should be called in a loop with different names until it completes successfully. (cherry picked from commit d6a71f4) Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
|
GH-145170 is a backport of this pull request to the 3.14 branch. |
|
GH-145171 is a backport of this pull request to the 3.13 branch. |
…iprocessing and asyncio on Windows (GH-137389) (GH-145171) Since os.stat() raises an OSError for existing named pipe "\\.\pipe\...", os.path.exists() always returns False for it, and tempfile.mktemp() can return a name that matches an existing named pipe. So, tempfile.mktemp() cannot be used to generate unique names for named pipes. Instead, CreateNamedPipe() should be called in a loop with different names until it completes successfully. (cherry picked from commit d6a71f4) Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
…iprocessing and asyncio on Windows (GH-137389) (GH-145170) Since os.stat() raises an OSError for existing named pipe "\\.\pipe\...", os.path.exists() always returns False for it, and tempfile.mktemp() can return a name that matches an existing named pipe. So, tempfile.mktemp() cannot be used to generate unique names for named pipes. Instead, CreateNamedPipe() should be called in a loop with different names until it completes successfully. (cherry picked from commit d6a71f4) Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
Since os.stat() raises an OSError for existing named pipe
"\\.\pipe\...", os.path.exists() always returns False for it, and tempfile.mktemp() can return a name that matches an existing named pipe.So, tempfile.mktemp() cannot be used to generate unique names for named pipes. Instead, CreateNamedPipe() should be called in a loop with different names until it completes successfully.
tempfile.mktemp()for creating named pipes on Windows #137335