-
-
Notifications
You must be signed in to change notification settings - Fork 32.1k
bpo-28724: Add methods send_fds, recv_fds to the socket module #12889
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 13 commits
fd7b901
2558c5d
dfdeb76
bc58424
cc6973c
5050e59
fe8ff1a
e98918f
c3fcd6f
d497c15
035845a
e6d22e0
f84d6db
07b05dc
8b96493
7c46e7a
2c32da1
75bffbc
ba29ca2
83e422d
910461b
b134b48
ee950f8
36e157d
0f03c42
bf67bd8
1ac7fbf
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 |
---|---|---|
|
@@ -669,6 +669,30 @@ The following functions all create :ref:`socket objects <socket-objects>`. | |
.. versionadded:: 3.3 | ||
|
||
|
||
.. function:: socket.send_fds(sock, buffers, fds[, flags[, address]]) | ||
|
||
:func:`socket.send_fds` sends the list of file descriptors *fds* | ||
over an :const:`AF_UNIX` socket, on systems which support the | ||
:const:`SCM_RIGHTS` mechanism. | ||
|
||
.. availability:: most Unix platforms, possibly others. | ||
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. I suggest "availability: Unix supporting :meth: |
||
|
||
.. versionadded:: 3.8 | ||
|
||
|
||
.. function:: socket.recv_fds(sock, bufsize, maxfds[, flags]) | ||
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. What is bufsize? How should I use it? What is flags? Maybe suggest to read recvmsg() for the documentation of flags? |
||
|
||
On systems which support the :const:`SCM_RIGHTS` mechanism, | ||
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. I suggest "availability: Unix supporting :meth: |
||
:func:`socket.recv_fds` will receive up to *maxfds* file descriptors, | ||
returning the message data and a list containing the descriptors | ||
(while ignoring unexpected conditions such as unrelated control | ||
messages being received). | ||
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. I suggest: Receive up to maxfds file descriptors. It seems like it returns up to 4 items. I don't understand "(while ignoring unexpected conditions such as unrelated control messages being received)." What is the behavior in case of "unrelated control messages being received"? What is the return format? |
||
|
||
.. availability:: most Unix platforms, possibly others. | ||
|
||
.. versionadded:: 3.8 | ||
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. It's too late for 3.8, you should now target 3.9. |
||
|
||
|
||
.. data:: SocketType | ||
|
||
This is a Python type object that represents the socket object type. It is the | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -12,6 +12,8 @@ | |||||
socket() -- create a new socket object | ||||||
socketpair() -- create a pair of new socket objects [*] | ||||||
fromfd() -- create a socket object from an open file descriptor [*] | ||||||
send_fds() -- Send file descriptor to the socket. | ||||||
recv_fds() -- Recieve file descriptor from the socket. | ||||||
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. I suggest to replace "file descriptor" with "file descriptors" for both functions here (add an S), as in their name, since they can send and receive more than one file descriptor ;-) |
||||||
fromshare() -- create a socket object from data received from socket.share() [*] | ||||||
gethostname() -- return the current hostname | ||||||
gethostbyname() -- map a hostname to its IP number | ||||||
|
@@ -49,7 +51,7 @@ | |||||
import _socket | ||||||
from _socket import * | ||||||
|
||||||
import os, sys, io, selectors | ||||||
import os, sys, io, selectors, array | ||||||
nanjekyejoannah marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
from enum import IntEnum, IntFlag | ||||||
|
||||||
try: | ||||||
|
@@ -463,6 +465,34 @@ def fromfd(fd, family, type, proto=0): | |||||
nfd = dup(fd) | ||||||
return socket(family, type, proto, nfd) | ||||||
|
||||||
if hasattr(_socket.socket, "sendmsg"): | ||||||
def send_fds(sock, buffers, fds, flags=0, address=None): | ||||||
""" send_fds(sock, buffers, fds[, flags[, address]]) -> socket object | ||||||
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. It seems like sendmsg() returns an integer, not a socket object. |
||||||
|
||||||
Send the list of file descriptors fds over an AF_UNIX socket. | ||||||
""" | ||||||
return sock.sendmsg(buffers, [(_socket.SOL_SOCKET, | ||||||
_socket.SCM_RIGHTS, array.array("i", fds))]) | ||||||
__all__.append("send_fds") | ||||||
|
||||||
if hasattr(_socket.socket, "recvmsg"): | ||||||
def recv_fds(sock, bufsize, maxfds, flags=0): | ||||||
""" recv_fds(sock, bufsize, maxfds[, flags]) -> (socket object, socket object) | ||||||
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. The doc of the return value looks wrong. The code uses a tuple of 4 times: |
||||||
|
||||||
receive up to maxfds file descriptors returning the message | ||||||
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. Upper case for the first word of the doc?
Suggested change
|
||||||
data and a list containing the descriptors. | ||||||
""" | ||||||
# Array of ints | ||||||
fds = array.array("i") | ||||||
msg, ancdata, flags, addr = sock.recvmsg(bufsize, | ||||||
_socket.CMSG_LEN(maxfds * fds.itemsize)) | ||||||
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 will likely fail on FreeBSD as RFC 3542 requires portable applications to use CMSG_SPACE if I am not mistaken. 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. We should check that this works on FreeBSD to avoid broken buildbots with obscure errors. 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. @pablogsal You mean at test level ? 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. I mean that FreeBSD systems running that code can fail and that will likely appear when running the tests that go over this code path. This is an example of similar scenarios of CMSG_LEN vs CMSG_SPACE: I could be wrong with my argument, that's why I think we should check directly on a FreeBSD system. 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. I dont have a freeBSD system to confirm this atm. When I do find, I will confirm but someone else can help confirm this. Do you have a FreeBSD to help check this? 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. Here are the custom builds: https://buildbot.python.org/all/#/builders/175/builds/39 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. It seems that my suspicions were correct:
but it could be something also on top of CMSG_LEN 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. @koobs Could you please take a look? The new feature is failing on FreeBSD. 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. I can confirm that is the same error as in OSX so it may be easier to investigate there. 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. According to the FreeBSD/OSX man page:
I am not sure if they mean that is done automatically or that you should do that. |
||||||
for cmsg_level, cmsg_type, cmsg_data in ancdata: | ||||||
if (cmsg_level == _socket.SOL_SOCKET and cmsg_type == _socket.SCM_RIGHTS): | ||||||
# Append data, ignoring any truncated integers at the end. | ||||||
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. Should we document this in the recv_fds() doc? 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. Ok, but IMHO it's worth it to mention that "any truncated integers at the end are ignored". 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. Well, another solution is to raise an hard error rather than silently ignoring a potential bug. 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. @nanjekyejoannah: Would you mind to document this behavior in recv_fds() documention? |
||||||
fds.fromstring(cmsg_data[:len(cmsg_data) - (len(cmsg_data) % fds.itemsize)]) | ||||||
return msg, list(fds), flags, addr | ||||||
vstinner marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
__all__.append("recv_fds") | ||||||
|
||||||
if hasattr(_socket.socket, "share"): | ||||||
def fromshare(info): | ||||||
""" fromshare(info) -> socket object | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2275,6 +2275,31 @@ def testFromFd(self): | |
def _testFromFd(self): | ||
self.serv_conn.send(MSG) | ||
|
||
@requireAttrs(socket.socket, "sendmsg") | ||
@requireAttrs(socket, "AF_UNIX") | ||
@unittest.skipIf(sys.platform == 'darwin', 'test not working for MacOSX') | ||
@unittest.skipIf(sys.platform.startswith(('freebsd', 'netbsd', 'gnukfreebsd')), 'test not working for freebsd') | ||
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. Wait, why are we ignoring the test in FreeBSD? We should fix the code/test instead, right? 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. Fixing means I remove those parameters that freebsd complains about in the function itself. Is that the right fix? and will require changing the docs too to say so. My assumption is that freebsd doesn't support those attributes we are calling that is why it is failing. 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. FREEBSD supports those functions ( https://www.freebsd.org/cgi/man.cgi?query=recvmsg&sektion=2&manpath=FreeBSD+6.0-RELEASE As well as MacOSX: I don't think we should ignore the test in those systems. We should understand why both systems complain with the current implementation. Maybe I am missing something, though. 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. The error returned is general pointing to invalid attributes. It may not be SCM_RIGHTS but may be it is SOL_SOCKET that is also used in the definition of send_fds() in this case. 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.
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. I agree finding a remedy is better. I may have time next week maybe to simulate this freebsd thing to troubleshoot this but If anyone wants to jump in now, no problem :) 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. I will try to find some time this week to help you in investigating this :) |
||
def testSendAndRecvFds(self): | ||
fds = [] | ||
# create two new file descriptors. | ||
for i in range(2): | ||
fd, path = tempfile.mkstemp() | ||
self.addCleanup(os.unlink, path) | ||
self.addCleanup(os.close, fd) | ||
os.write(fd, str(i).encode()) | ||
fds.append(fd) | ||
f = self.cli_conn.detach() | ||
sock = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM, fileno=f) | ||
self.addCleanup(sock.close) | ||
with sock: | ||
socket.send_fds(sock, [MSG], fds) | ||
msg, fds_list, flags, addr = socket.recv_fds(sock, len(MSG), 1024) | ||
self.assertEqual(msg, MSG) | ||
|
||
@testSendAndRecvFds.client_skip | ||
def _testSendAndRecvFds(self): | ||
nanjekyejoannah marked this conversation as resolved.
Show resolved
Hide resolved
|
||
self.serv_conn.send(MSG) | ||
|
||
def testDup(self): | ||
# Testing dup() | ||
sock = self.cli_conn.dup() | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
The socket module now has the :func:`socket.send_fds()` and :func:`socket.recv.fds()` functions. | ||
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. It's a method, not a function, no? You should omit parenthesis. The :class: |
||
Contributed by Joannah Nanjekye and Shinya Okano . |
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.
What are buffers, flags and address? Maybe suggest to read sendmsg() for the documentation of these parameters?