Fix connect_read_pipe/connect_write_pipe double-close of the underlying fd - #764
Open
jaideeppyne wants to merge 1 commit into
Open
jaideeppyne wants to merge 1 commit into
jaideeppyne wants to merge 1 commit into
Conversation
connect_read_pipe and connect_write_pipe passed the Python file object's fd to libuv, which takes ownership and closes it. Closing the file object later (or GC) then closed the same fd again, which can steal a recycled descriptor. Dup the fd before uv_pipe_open so libuv and the file object each own a distinct descriptor.
Member
|
Thanks for the PR! But I'm not sure @1st1 previously explained that duplication can make uvloop exhaust FD limits sooner than standard asyncio. 318e593 subsequently removed duplication, including from these two APIs. I'd like to preserve your regression tests and resolve the ownership conflict without reintroducing duplication. That means we shall either patch libuv to avoid closing the FD conditionally, or replicate |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
connect_read_pipeandconnect_write_pipepassed the Python file object's fd touv_pipe_open. libuv takes ownership and closes that fd inuv_close. The attached file object later calledclose()on the same number. If the kernel had reused the fd, this closed an unrelated descriptor.This is the pipe equivalent of the socket double-close fixed in d5195d7. The EBADF on non-socket
close()is not always benign.The same race exists if the transport is GC'd instead of explicitly closed.
Fix
os.dup()the fd beforeuv_pipe_openso libuv and the Python file object each own a distinct descriptor.uv_close()closes the dup;fileobj.close()(or GC) closes the original. Either order is safe.Tests
tests/test_pipes.pyadds a probe (from the issue) that opens a freshos.pipe()inside the file object'sclose()and fails if those fds are stolen. Covered for both read and write pipes.Fixes #763