-
-
Notifications
You must be signed in to change notification settings - Fork 33.2k
gh-107898: Do not ignore address and flags parameters in socket.{send,recv}_fds
#128882
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
base: main
Are you sure you want to change the base?
Changes from 12 commits
b9ae8e2
45b28f6
fbb2668
c58d2de
aefccca
6b58f4b
486e18c
0134f01
5cca1b4
6c226c4
b130108
bb1c5ad
02b0ddd
7368656
4f8d1ca
c423723
1dcd26f
5d3e896
d37ba07
f4c9d67
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 | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -3516,7 +3516,7 @@ def _testSendmsgTimeout(self): | |||||||||||||||
| # Linux supports MSG_DONTWAIT when sending, but in general, it | ||||||||||||||||
| # only works when receiving. Could add other platforms if they | ||||||||||||||||
| # support it too. | ||||||||||||||||
| @skipWithClientIf(sys.platform not in {"linux", "android"}, | ||||||||||||||||
| @skipWithClientIf(sys.platform not in {"linux", "android", "freebsd"}, | ||||||||||||||||
| "MSG_DONTWAIT not known to work on this platform when " | ||||||||||||||||
| "sending") | ||||||||||||||||
| def testSendmsgDontWait(self): | ||||||||||||||||
|
|
@@ -7327,19 +7327,22 @@ def test_dual_stack_client_v6(self): | |||||||||||||||
| @requireAttrs(socket, "recv_fds") | ||||||||||||||||
| @requireAttrs(socket, "AF_UNIX") | ||||||||||||||||
| class SendRecvFdsTests(unittest.TestCase): | ||||||||||||||||
| def testSendAndRecvFds(self): | ||||||||||||||||
| def close_pipes(pipes): | ||||||||||||||||
| for fd1, fd2 in pipes: | ||||||||||||||||
| os.close(fd1) | ||||||||||||||||
| os.close(fd2) | ||||||||||||||||
|
|
||||||||||||||||
| def _cleanup_fds(self, fds): | ||||||||||||||||
| def close_fds(fds): | ||||||||||||||||
| for fd in fds: | ||||||||||||||||
| os.close(fd) | ||||||||||||||||
| self.addCleanup(close_fds, fds) | ||||||||||||||||
|
|
||||||||||||||||
| def _test_pipe(self, rfd, wfd, msg): | ||||||||||||||||
| assert len(msg) < 512 | ||||||||||||||||
| os.write(wfd, msg) | ||||||||||||||||
| data = os.read(rfd, 512) | ||||||||||||||||
| self.assertEqual(data, msg) | ||||||||||||||||
|
||||||||||||||||
| assert len(msg) < 512 | |
| os.write(wfd, msg) | |
| data = os.read(rfd, 512) | |
| self.assertEqual(data, msg) | |
| os.write(wfd, msg) | |
| data = os.read(rfd, len(msg)) | |
| self.assertEqual(data, msg) |
I think we can do it like this right?
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.
man 7 pipe says:
POSIX.1 says that writes of less than PIPE_BUF bytes must be atomic: the output data is written to the pipe as a contiguous sequence. [...] POSIX.1 requires PIPE_BUF to be at least 512 bytes.
I hardcoded 512 here, which may look confusing. I've updated it to make it more clear.
GalaxySnail marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
GalaxySnail marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
GalaxySnail marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
GalaxySnail marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
picnixz marked this conversation as resolved.
Show resolved
Hide resolved
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.
I'm back here, but does it work on freebsd?
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.
Unfortunately no. On FreeBSD recvmsg with MSG_PEEK returns random bytes in the ancillary data, which looks like some kind of uninitialized memory.
GalaxySnail marked this conversation as resolved.
Show resolved
Hide resolved
picnixz marked this conversation as resolved.
Show resolved
Hide resolved
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.
Above, the test for sendmsg only enabled it for @skipWithClientIf(sys.platform not in {"linux", "android", "freebsd"}. I think it's better if we check this here as well because there might be more platforms than {"linux", "android", "freebsd", "darwin"} in the future.
Or: we change the test above with @skipWithClientIf(sys.platform not in ("darwin",) instead and add @requireAttrs(socket, "MSG_DONTWAIT")
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.
According to PEP 11, it should be safe to replace @skipWithClientIf(sys.platform not in {"linux", "android", "freebsd"}) with @requireAttrs(socket, "MSG_DONTWAIT") and @skipWithClientIf(sys.platform not in ("darwin",)) on all supported platforms. If this test fails on any other platform, we can update it as needed.
GalaxySnail marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
GalaxySnail marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
GalaxySnail marked this conversation as resolved.
Show resolved
Hide resolved
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| Fix ``flags`` and ``address`` parameters which were ignored in | ||
| :func:`socket.send_fds` and :func:`socket.recv_fds`. |
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.
Let's rename the result flags as
msg_flagsto be consistent with the signature (don't forget to updatereturn ...)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.
Done.