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

use a stack of self._fds_to_close to prevent double closes #481

Merged
merged 8 commits into from
Aug 23, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 53 additions & 1 deletion tests/test_process.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import subprocess
import sys
import tempfile
import textwrap
import time
import unittest

Expand Down Expand Up @@ -796,7 +797,58 @@ async def test():


class Test_UV_Process(_TestProcess, tb.UVTestCase):
pass
def test_process_double_close(self):
script = textwrap.dedent("""
import os
import sys
from unittest import mock

import asyncio

pipes = []
original_os_pipe = os.pipe
def log_pipes():
pipe = original_os_pipe()
pipes.append(pipe)
return pipe

dups = []
original_os_dup = os.dup
def log_dups(*args, **kwargs):
dup = original_os_dup(*args, **kwargs)
dups.append(dup)
return dup

with mock.patch(
"os.close", wraps=os.close
) as os_close, mock.patch(
"os.pipe", new=log_pipes
), mock.patch(
"os.dup", new=log_dups
):
import uvloop


async def test():
proc = await asyncio.create_subprocess_exec(
sys.executable, "-c", "pass"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you don't actually need a preexec_fn to trigger the double close it just made it slow enough that the race was more detectable

)
await proc.communicate()

uvloop.install()
asyncio.run(test())

stdin, stdout, stderr = dups
(r, w), = pipes
assert os_close.mock_calls == [
mock.call(w),
mock.call(r),
mock.call(stderr),
mock.call(stdout),
mock.call(stdin),
]
""")
subprocess.run([sys.executable, '-c', script], check=True)


class Test_AIO_Process(_TestProcess, tb.AIOTestCase):
Expand Down
2 changes: 1 addition & 1 deletion uvloop/handles/process.pxd
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ cdef class UVProcess(UVHandle):
object _preexec_fn
bint _restore_signals

set _fds_to_close
list _fds_to_close
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fantix I think there's a proper list[int] data type in cython for this but I don't know how to use it


# Attributes used to compose uv_process_options_t:
uv.uv_process_options_t options
Expand Down
31 changes: 12 additions & 19 deletions uvloop/handles/process.pyx
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ cdef class UVProcess(UVHandle):
self.uv_opt_args = NULL
self._returncode = None
self._pid = None
self._fds_to_close = set()
self._fds_to_close = list()
self._preexec_fn = None
self._restore_signals = True
self.context = Context_CopyCurrent()
Expand Down Expand Up @@ -69,6 +69,11 @@ cdef class UVProcess(UVHandle):
'Racing with another loop to spawn a process.')

self._errpipe_read, self._errpipe_write = os_pipe()
fds_to_close = self._fds_to_close
self._fds_to_close = None
fds_to_close.append(self._errpipe_read)
# add the write pipe last so we can close it early
fds_to_close.append(self._errpipe_write)
graingert marked this conversation as resolved.
Show resolved Hide resolved
try:
os_set_inheritable(self._errpipe_write, True)

Expand Down Expand Up @@ -100,8 +105,8 @@ cdef class UVProcess(UVHandle):

self._finish_init()

os_close(self._errpipe_write)
self._errpipe_write = -1
# close the write pipe early
os_close(fds_to_close.pop())

if preexec_fn is not None:
errpipe_data = bytearray()
Expand All @@ -115,17 +120,8 @@ cdef class UVProcess(UVHandle):
break

finally:
os_close(self._errpipe_read)
try:
os_close(self._errpipe_write)
except OSError:
# Might be already closed
pass

fds_to_close = self._fds_to_close
self._fds_to_close = None
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think self._fds_to_close = None is still needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yep I see it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved it up so that _close_after_spawn can't interfere with the errpipe_write fd

for fd in fds_to_close:
os_close(fd)
while fds_to_close:
os_close(fds_to_close.pop())

for fd in restore_inheritable:
os_set_inheritable(fd, False)
Expand Down Expand Up @@ -202,7 +198,7 @@ cdef class UVProcess(UVHandle):
if self._fds_to_close is None:
raise RuntimeError(
'UVProcess._close_after_spawn called after uv_spawn')
self._fds_to_close.add(fd)
self._fds_to_close.append(fd)

def __dealloc__(self):
if self.uv_opt_env is not NULL:
Expand Down Expand Up @@ -500,10 +496,7 @@ cdef class UVProcessTransport(UVProcess):
# shouldn't ever happen
raise RuntimeError('cannot apply subprocess.STDOUT')

newfd = os_dup(io[1])
os_set_inheritable(newfd, True)
self._close_after_spawn(newfd)
io[2] = newfd
io[2] = self._file_redirect_stdio(io[1])
Comment on lines -503 to +499
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when reviewing all the calls to _close_after_spawn to make sure a set wasn't needed, I found this duplicate code

cdef _file_redirect_stdio(self, int fd):
fd = os_dup(fd)
os_set_inheritable(fd, True)
self._close_after_spawn(fd)
return fd

elif _stderr == subprocess_DEVNULL:
io[2] = self._file_devnull()
else:
Expand Down