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

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.

Sign up for GitHub

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

Merged
fantix merged 8 commits into from
Aug 23, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
b1f4f3d
Add test for preexec_fn fd double close issue
fantix Jul 17, 2022
757e39c
Update tests/test_process.py
fantix Jul 17, 2022
730dd1a
use a stack of self._fds_to_close to prevent double closes
graingert Jul 19, 2022
ddeb5e1
Merge branch 'master' into preexec-fn-double-close
graingert Jul 19, 2022
f4f57f6
Update process.pyx
graingert Jul 19, 2022
d5fe279
Update uvloop/handles/process.pyx
graingert Jul 20, 2022
5926386
DRY os_dup call
graingert Jul 20, 2022
aeb5560
Merge branch 'master' into preexec-fn-double-close
graingert Aug 14, 2022
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
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -7,6 +7,7 @@
import subprocess
import sys
import tempfile
import textwrap
import time
import unittest

Expand DownExpand 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"

@graingertgraingertJul 19, 2022

Copy link
Copy Markdown
ContributorAuthor

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
View file
Open in desktop
Original file line numberDiff line numberDiff 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

@graingertgraingertAug 16, 2022

Copy link
Copy Markdown
ContributorAuthor

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
View file
Open in desktop
Original file line numberDiff line numberDiff 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 DownExpand 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)
Comment thread
graingert marked this conversation as resolved.
try:
os_set_inheritable(self._errpipe_write, True)

Expand DownExpand 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

@fantixfantixJul 19, 2022

Copy link
Copy Markdown
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?

@graingertgraingertJul 19, 2022

Copy link
Copy Markdown
ContributorAuthor

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

@graingertgraingertJul 19, 2022

Copy link
Copy Markdown
ContributorAuthor

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 DownExpand 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 DownExpand 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

@graingertgraingertJul 20, 2022

Copy link
Copy Markdown
ContributorAuthor

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

uvloop/uvloop/handles/process.pyx

Lines 398 to 402 in 5926386

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
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all \u003cpre\u003e\u003ccode\u003e blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks"); } } catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); } })(); (function(){ try { var __m = "github.com"; var __re = new RegExp('^' + "github\\.com" + '
Skip to content

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

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.

Sign up for GitHub

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

Merged
fantix merged 8 commits into from
Aug 23, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
b1f4f3d
Add test for preexec_fn fd double close issue
fantix Jul 17, 2022
757e39c
Update tests/test_process.py
fantix Jul 17, 2022
730dd1a
use a stack of self._fds_to_close to prevent double closes
graingert Jul 19, 2022
ddeb5e1
Merge branch 'master' into preexec-fn-double-close
graingert Jul 19, 2022
f4f57f6
Update process.pyx
graingert Jul 19, 2022
d5fe279
Update uvloop/handles/process.pyx
graingert Jul 20, 2022
5926386
DRY os_dup call
graingert Jul 20, 2022
aeb5560
Merge branch 'master' into preexec-fn-double-close
graingert Aug 14, 2022
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
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -7,6 +7,7 @@
import subprocess
import sys
import tempfile
import textwrap
import time
import unittest

Expand DownExpand 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"

@graingertgraingertJul 19, 2022

Copy link
Copy Markdown
ContributorAuthor

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
View file
Open in desktop
Original file line numberDiff line numberDiff 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

@graingertgraingertAug 16, 2022

Copy link
Copy Markdown
ContributorAuthor

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
View file
Open in desktop
Original file line numberDiff line numberDiff 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 DownExpand 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)
Comment thread
graingert marked this conversation as resolved.
try:
os_set_inheritable(self._errpipe_write, True)

Expand DownExpand 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

@fantixfantixJul 19, 2022

Copy link
Copy Markdown
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?

@graingertgraingertJul 19, 2022

Copy link
Copy Markdown
ContributorAuthor

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

@graingertgraingertJul 19, 2022

Copy link
Copy Markdown
ContributorAuthor

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 DownExpand 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 DownExpand 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

@graingertgraingertJul 20, 2022

Copy link
Copy Markdown
ContributorAuthor

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

uvloop/uvloop/handles/process.pyx

Lines 398 to 402 in 5926386

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
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

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

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.

Sign up for GitHub

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

Merged
fantix merged 8 commits into from
Aug 23, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
b1f4f3d
Add test for preexec_fn fd double close issue
fantix Jul 17, 2022
757e39c
Update tests/test_process.py
fantix Jul 17, 2022
730dd1a
use a stack of self._fds_to_close to prevent double closes
graingert Jul 19, 2022
ddeb5e1
Merge branch 'master' into preexec-fn-double-close
graingert Jul 19, 2022
f4f57f6
Update process.pyx
graingert Jul 19, 2022
d5fe279
Update uvloop/handles/process.pyx
graingert Jul 20, 2022
5926386
DRY os_dup call
graingert Jul 20, 2022
aeb5560
Merge branch 'master' into preexec-fn-double-close
graingert Aug 14, 2022
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
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -7,6 +7,7 @@
import subprocess
import sys
import tempfile
import textwrap
import time
import unittest

Expand DownExpand 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"

@graingertgraingertJul 19, 2022

Copy link
Copy Markdown
ContributorAuthor

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
View file
Open in desktop
Original file line numberDiff line numberDiff 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

@graingertgraingertAug 16, 2022

Copy link
Copy Markdown
ContributorAuthor

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
View file
Open in desktop
Original file line numberDiff line numberDiff 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 DownExpand 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)
Comment thread
graingert marked this conversation as resolved.
try:
os_set_inheritable(self._errpipe_write, True)

Expand DownExpand 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

@fantixfantixJul 19, 2022

Copy link
Copy Markdown
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?

@graingertgraingertJul 19, 2022

Copy link
Copy Markdown
ContributorAuthor

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

@graingertgraingertJul 19, 2022

Copy link
Copy Markdown
ContributorAuthor

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 DownExpand 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 DownExpand 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

@graingertgraingertJul 20, 2022

Copy link
Copy Markdown
ContributorAuthor

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

uvloop/uvloop/handles/process.pyx

Lines 398 to 402 in 5926386

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
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length \u003e 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

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

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.

Sign up for GitHub

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

Merged
fantix merged 8 commits into from
Aug 23, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
b1f4f3d
Add test for preexec_fn fd double close issue
fantix Jul 17, 2022
757e39c
Update tests/test_process.py
fantix Jul 17, 2022
730dd1a
use a stack of self._fds_to_close to prevent double closes
graingert Jul 19, 2022
ddeb5e1
Merge branch 'master' into preexec-fn-double-close
graingert Jul 19, 2022
f4f57f6
Update process.pyx
graingert Jul 19, 2022
d5fe279
Update uvloop/handles/process.pyx
graingert Jul 20, 2022
5926386
DRY os_dup call
graingert Jul 20, 2022
aeb5560
Merge branch 'master' into preexec-fn-double-close
graingert Aug 14, 2022
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
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -7,6 +7,7 @@
import subprocess
import sys
import tempfile
import textwrap
import time
import unittest

Expand DownExpand 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"

@graingertgraingertJul 19, 2022

Copy link
Copy Markdown
ContributorAuthor

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
View file
Open in desktop
Original file line numberDiff line numberDiff 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

@graingertgraingertAug 16, 2022

Copy link
Copy Markdown
ContributorAuthor

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
View file
Open in desktop
Original file line numberDiff line numberDiff 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 DownExpand 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)
Comment thread
graingert marked this conversation as resolved.
try:
os_set_inheritable(self._errpipe_write, True)

Expand DownExpand 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

@fantixfantixJul 19, 2022

Copy link
Copy Markdown
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?

@graingertgraingertJul 19, 2022

Copy link
Copy Markdown
ContributorAuthor

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

@graingertgraingertJul 19, 2022

Copy link
Copy Markdown
ContributorAuthor

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 DownExpand 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 DownExpand 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

@graingertgraingertJul 20, 2022

Copy link
Copy Markdown
ContributorAuthor

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

uvloop/uvloop/handles/process.pyx

Lines 398 to 402 in 5926386

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
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

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

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.

Sign up for GitHub

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

Merged
fantix merged 8 commits into from
Aug 23, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
b1f4f3d
Add test for preexec_fn fd double close issue
fantix Jul 17, 2022
757e39c
Update tests/test_process.py
fantix Jul 17, 2022
730dd1a
use a stack of self._fds_to_close to prevent double closes
graingert Jul 19, 2022
ddeb5e1
Merge branch 'master' into preexec-fn-double-close
graingert Jul 19, 2022
f4f57f6
Update process.pyx
graingert Jul 19, 2022
d5fe279
Update uvloop/handles/process.pyx
graingert Jul 20, 2022
5926386
DRY os_dup call
graingert Jul 20, 2022
aeb5560
Merge branch 'master' into preexec-fn-double-close
graingert Aug 14, 2022
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
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -7,6 +7,7 @@
import subprocess
import sys
import tempfile
import textwrap
import time
import unittest

Expand DownExpand 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"

@graingertgraingertJul 19, 2022

Copy link
Copy Markdown
ContributorAuthor

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
View file
Open in desktop
Original file line numberDiff line numberDiff 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

@graingertgraingertAug 16, 2022

Copy link
Copy Markdown
ContributorAuthor

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
View file
Open in desktop
Original file line numberDiff line numberDiff 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 DownExpand 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)
Comment thread
graingert marked this conversation as resolved.
try:
os_set_inheritable(self._errpipe_write, True)

Expand DownExpand 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

@fantixfantixJul 19, 2022

Copy link
Copy Markdown
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?

@graingertgraingertJul 19, 2022

Copy link
Copy Markdown
ContributorAuthor

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

@graingertgraingertJul 19, 2022

Copy link
Copy Markdown
ContributorAuthor

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 DownExpand 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 DownExpand 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

@graingertgraingertJul 20, 2022

Copy link
Copy Markdown
ContributorAuthor

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

uvloop/uvloop/handles/process.pyx

Lines 398 to 402 in 5926386

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
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

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

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.

Sign up for GitHub

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

Merged
fantix merged 8 commits into from
Aug 23, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
b1f4f3d
Add test for preexec_fn fd double close issue
fantix Jul 17, 2022
757e39c
Update tests/test_process.py
fantix Jul 17, 2022
730dd1a
use a stack of self._fds_to_close to prevent double closes
graingert Jul 19, 2022
ddeb5e1
Merge branch 'master' into preexec-fn-double-close
graingert Jul 19, 2022
f4f57f6
Update process.pyx
graingert Jul 19, 2022
d5fe279
Update uvloop/handles/process.pyx
graingert Jul 20, 2022
5926386
DRY os_dup call
graingert Jul 20, 2022
aeb5560
Merge branch 'master' into preexec-fn-double-close
graingert Aug 14, 2022
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
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -7,6 +7,7 @@
import subprocess
import sys
import tempfile
import textwrap
import time
import unittest

Expand DownExpand 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"

@graingertgraingertJul 19, 2022

Copy link
Copy Markdown
ContributorAuthor

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
View file
Open in desktop
Original file line numberDiff line numberDiff 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

@graingertgraingertAug 16, 2022

Copy link
Copy Markdown
ContributorAuthor

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
View file
Open in desktop
Original file line numberDiff line numberDiff 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 DownExpand 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)
Comment thread
graingert marked this conversation as resolved.
try:
os_set_inheritable(self._errpipe_write, True)

Expand DownExpand 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

@fantixfantixJul 19, 2022

Copy link
Copy Markdown
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?

@graingertgraingertJul 19, 2022

Copy link
Copy Markdown
ContributorAuthor

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

@graingertgraingertJul 19, 2022

Copy link
Copy Markdown
ContributorAuthor

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 DownExpand 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 DownExpand 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

@graingertgraingertJul 20, 2022

Copy link
Copy Markdown
ContributorAuthor

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

uvloop/uvloop/handles/process.pyx

Lines 398 to 402 in 5926386

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
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

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

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.

Sign up for GitHub

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

Merged
fantix merged 8 commits into from
Aug 23, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
b1f4f3d
Add test for preexec_fn fd double close issue
fantix Jul 17, 2022
757e39c
Update tests/test_process.py
fantix Jul 17, 2022
730dd1a
use a stack of self._fds_to_close to prevent double closes
graingert Jul 19, 2022
ddeb5e1
Merge branch 'master' into preexec-fn-double-close
graingert Jul 19, 2022
f4f57f6
Update process.pyx
graingert Jul 19, 2022
d5fe279
Update uvloop/handles/process.pyx
graingert Jul 20, 2022
5926386
DRY os_dup call
graingert Jul 20, 2022
aeb5560
Merge branch 'master' into preexec-fn-double-close
graingert Aug 14, 2022
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
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -7,6 +7,7 @@
import subprocess
import sys
import tempfile
import textwrap
import time
import unittest

Expand DownExpand 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"

@graingertgraingertJul 19, 2022

Copy link
Copy Markdown
ContributorAuthor

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
View file
Open in desktop
Original file line numberDiff line numberDiff 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

@graingertgraingertAug 16, 2022

Copy link
Copy Markdown
ContributorAuthor

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
View file
Open in desktop
Original file line numberDiff line numberDiff 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 DownExpand 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)
Comment thread
graingert marked this conversation as resolved.
try:
os_set_inheritable(self._errpipe_write, True)

Expand DownExpand 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

@fantixfantixJul 19, 2022

Copy link
Copy Markdown
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?

@graingertgraingertJul 19, 2022

Copy link
Copy Markdown
ContributorAuthor

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

@graingertgraingertJul 19, 2022

Copy link
Copy Markdown
ContributorAuthor

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 DownExpand 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 DownExpand 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

@graingertgraingertJul 20, 2022

Copy link
Copy Markdown
ContributorAuthor

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

uvloop/uvloop/handles/process.pyx

Lines 398 to 402 in 5926386

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
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

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

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.

Sign up for GitHub

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

Merged
fantix merged 8 commits into from
Aug 23, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
b1f4f3d
Add test for preexec_fn fd double close issue
fantix Jul 17, 2022
757e39c
Update tests/test_process.py
fantix Jul 17, 2022
730dd1a
use a stack of self._fds_to_close to prevent double closes
graingert Jul 19, 2022
ddeb5e1
Merge branch 'master' into preexec-fn-double-close
graingert Jul 19, 2022
f4f57f6
Update process.pyx
graingert Jul 19, 2022
d5fe279
Update uvloop/handles/process.pyx
graingert Jul 20, 2022
5926386
DRY os_dup call
graingert Jul 20, 2022
aeb5560
Merge branch 'master' into preexec-fn-double-close
graingert Aug 14, 2022
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
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -7,6 +7,7 @@
import subprocess
import sys
import tempfile
import textwrap
import time
import unittest

Expand DownExpand 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"

@graingertgraingertJul 19, 2022

Copy link
Copy Markdown
ContributorAuthor

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
View file
Open in desktop
Original file line numberDiff line numberDiff 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

@graingertgraingertAug 16, 2022

Copy link
Copy Markdown
ContributorAuthor

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
View file
Open in desktop
Original file line numberDiff line numberDiff 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 DownExpand 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)
Comment thread
graingert marked this conversation as resolved.
try:
os_set_inheritable(self._errpipe_write, True)

Expand DownExpand 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

@fantixfantixJul 19, 2022

Copy link
Copy Markdown
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?

@graingertgraingertJul 19, 2022

Copy link
Copy Markdown
ContributorAuthor

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

@graingertgraingertJul 19, 2022

Copy link
Copy Markdown
ContributorAuthor

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 DownExpand 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 DownExpand 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

@graingertgraingertJul 20, 2022

Copy link
Copy Markdown
ContributorAuthor

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

uvloop/uvloop/handles/process.pyx

Lines 398 to 402 in 5926386

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