Repository navigation
Synchronize mmap buffer exports with close and resize #9015
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -39,3 +39,41 @@ | |
| mapped.find(b"") | ||
| with assert_raises(ValueError): | ||
| mapped.rfind(b"") | ||
|
|
||
|
|
||
| # Buffer acquisition must reject a closed mapping before returning a view. | ||
| with assert_raises(ValueError): | ||
| memoryview(mapped) | ||
|
Comment on lines
+45
to
+46
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This block adds new test setup, assertions, and data to a test file, but the repository's critical test policy permits test-file edits only for adding or removing expected-failure markers (and their associated TODO comments). The added snippet therefore needs to be removed or the repository policy changed before landing. AGENTS.md reference: AGENTS.md:L273-L279 Useful? React with 👍 / 👎. |
||
|
|
||
| mapped = mmap.mmap(-1, 4) | ||
| mapped[:] = b"data" | ||
| view = memoryview(mapped) | ||
| child = view[1:] | ||
| view.release() | ||
| with assert_raises(BufferError): | ||
| mapped.close() | ||
| assert not mapped.closed | ||
| assert child.tobytes() == b"ata" | ||
| child.release() | ||
| mapped.close() | ||
| mapped.close() | ||
|
|
||
| # On Windows resize must neither invalidate an export nor revive a closed map. | ||
| import sys | ||
|
|
||
| if sys.platform == "win32": | ||
| mapped = mmap.mmap(-1, 4) | ||
| mapped[:] = b"data" | ||
| with memoryview(mapped) as view: | ||
| with assert_raises(BufferError): | ||
| mapped.resize(8) | ||
| with assert_raises(BufferError): | ||
| mapped.resize(0) | ||
| assert view.tobytes() == b"data" | ||
| mapped.resize(8) | ||
| with memoryview(mapped) as view: | ||
| assert view.tobytes() == b"data" + b"\0" * 4 | ||
| mapped.close() | ||
| with assert_raises(ValueError): | ||
| mapped.resize(4) | ||
| assert mapped.closed | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When one thread passes this mmap to
_SSLSocket.write,crates/stdlib/src/openssl.rs:3073-3104holds the mmap guard viadata.borrow_buf()across a potentially unboundedallow_threadscall. A concurrentmapped.close()now blocks here instead of observing the already-nonzero export count and raisingBufferError; because this mutex wait remains attached to the interpreter, it can also participate in a stop-the-world deadlock. Keep an unlocked export-count fast path before this acquisition and recheck under the lock to preserve the race fix;check_resizeableneeds the same ordering.Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, you're right. Fixed in aa7ac19.
close()andcheck_resizeable()now reject existing exports before acquiring the mapping mutex, while retaining the check under the lock to prevent invalidating a concurrent export.Both new regressions hold an actual borrowed mmap buffer across the operation. They failed before this change and now pass on Linux and Windows.