Skip to content
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

gh-128881: Do not ignore address and flags parameters in socket.{send,recv}_fds #128882

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 3 additions & 2 deletions Lib/socket.py
Original file line number Diff line number Diff line change
Expand Up @@ -563,7 +563,8 @@ def send_fds(sock, buffers, fds, flags=0, address=None):
import array

return sock.sendmsg(buffers, [(_socket.SOL_SOCKET,
_socket.SCM_RIGHTS, array.array("i", fds))])
_socket.SCM_RIGHTS, array.array("i", fds))],
flags, address)
__all__.append("send_fds")

if hasattr(_socket.socket, "recvmsg"):
Expand All @@ -579,7 +580,7 @@ def recv_fds(sock, bufsize, maxfds, flags=0):
# Array of ints
fds = array.array("i")
msg, ancdata, flags, addr = sock.recvmsg(bufsize,
_socket.CMSG_LEN(maxfds * fds.itemsize))
_socket.CMSG_LEN(maxfds * fds.itemsize), flags)
for cmsg_level, cmsg_type, cmsg_data in ancdata:
if (cmsg_level == _socket.SOL_SOCKET and cmsg_type == _socket.SCM_RIGHTS):
fds.frombytes(cmsg_data[:
Expand Down
105 changes: 91 additions & 14 deletions Lib/test/test_socket.py
Original file line number Diff line number Diff line change
Expand Up @@ -7037,19 +7037,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)

def testSendAndRecvFds(self):
# send 10 file descriptors
pipes = [os.pipe() for _ in range(10)]
self.addCleanup(close_pipes, pipes)
self._cleanup_fds(fd for pair in pipes for fd in pair)
fds = [rfd for rfd, wfd in pipes]

# use a UNIX socket pair to exchange file descriptors locally
Expand All @@ -7058,21 +7061,95 @@ def close_fds(fds):
socket.send_fds(sock1, [MSG], fds)
# request more data and file descriptors than expected
msg, fds2, flags, addr = socket.recv_fds(sock2, len(MSG) * 2, len(fds) * 2)
self.addCleanup(close_fds, fds2)
self._cleanup_fds(fds2)

self.assertEqual(msg, MSG)
self.assertEqual(len(fds2), len(fds))
self.assertEqual(flags, 0)
# don't test addr

# test that file descriptors are connected
for index, fds in enumerate(pipes):
rfd, wfd = fds
os.write(wfd, str(index).encode())
for index, ((_, wfd), rfd) in enumerate(zip(pipes, fds2)):
self._test_pipe(rfd, wfd, str(index).encode())

@unittest.skipUnless(sys.platform in ("linux", "android", "darwin"),
"works on Linux and macOS")
def test_send_recv_fds_with_addrs(self):
sock1 = socket.socket(socket.AF_UNIX, socket.SOCK_DGRAM)
sock2 = socket.socket(socket.AF_UNIX, socket.SOCK_DGRAM)
rfd, wfd = os.pipe()
self.addCleanup(os.close, rfd)
self.addCleanup(os.close, wfd)

with tempfile.TemporaryDirectory() as tmpdir, sock1, sock2:
sock1_addr = os.path.join(tmpdir, "sock1")
sock2_addr = os.path.join(tmpdir, "sock2")
sock1.bind(sock1_addr)
sock2.bind(sock2_addr)
sock2.setblocking(False)

socket.send_fds(sock1, [MSG], [rfd], address=sock2_addr)
msg, fds, flags, addr = socket.recv_fds(sock2, len(MSG), 1)
self._cleanup_fds(fds)

self.assertEqual(msg, MSG)
self.assertEqual(len(fds), 1)
self.assertEqual(addr, sock1_addr)

self._test_pipe(fds[0], wfd, MSG)

for index, rfd in enumerate(fds2):
data = os.read(rfd, 100)
self.assertEqual(data, str(index).encode())
@requireAttrs(socket, "MSG_PEEK")
@unittest.skipUnless(sys.platform in ("linux", "android"), "works on Linux")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to have a flag test on macOS or not at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about it, but I'm not sure how to implement it. Is it good practice to write a _test_recv_fds_peek(self, skip_fd_check=False) method and 2 wrappers with different arguments?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A good practice is to check if there's a flag that is supported both on Unix and Windows. The reason is that the availability of recv_fds is marked as Unix and Windows but not WASI. So we shouldn't have this kind of skip in the first place.

Now, if there is not a common flag that is supported on all platforms, just writing three different tests for three different flags is also fine (each of which protected by a skipUnless)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason is that the availability of recv_fds is marked as Unix and Windows but not WASI. So we shouldn't have this kind of skip in the first place.

recv_fds requires recvmsg, which is only available on Unix but not WASI, so I think it may be a documentation error.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok actually we have #122153 which wants to implement it but we never implemented it. Good to know that it's a doc issue. We can open a PR (using 122153 as the GH issue number) to amend the docs. For now, can you add some comment saying that it's not available on Windows yet but can be (and add some "see gh-122153)? thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now, can you add some comment saying that it's not available on Windows yet but can be (and add some "see gh-122153)? thanks

Of course!

However sendmsg/recvmsg aren't the only requirements, CPython doesn't support AF_UNIX on Windows, and Windows doesn't support SCM_RIGHTS at all, so I think it's not possible to implement send_fds/recv_fds on Windows.

ref: #77589 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doc issue was introduced in e3b6ff1

def test_recv_fds_peek(self):
rfd, wfd = os.pipe()
self.addCleanup(os.close, rfd)
self.addCleanup(os.close, wfd)

sock1, sock2 = socket.socketpair(socket.AF_UNIX, socket.SOCK_DGRAM)
with sock1, sock2:
socket.send_fds(sock1, [MSG], [rfd])
sock2.setblocking(False)

# peek message on sock2
peek_len = len(MSG) // 2
msg, fds, flags, addr = socket.recv_fds(sock2, peek_len, 1,
flags=socket.MSG_PEEK)
self._cleanup_fds(fds)

self.assertEqual(len(msg), peek_len)
self.assertEqual(msg, MSG[:peek_len])
self.assertEqual(flags & socket.MSG_TRUNC, socket.MSG_TRUNC)
self.assertEqual(len(fds), 1)
self._test_pipe(fds[0], wfd, MSG)

# will raise BlockingIOError if MSG_PEEK didn't work
msg, fds, flags, addr = socket.recv_fds(sock2, len(MSG), 1)
self._cleanup_fds(fds)

self.assertEqual(msg, MSG)
self.assertEqual(len(fds), 1)
self._test_pipe(fds[0], wfd, MSG)

@requireAttrs(socket, "MSG_DONTWAIT")
@unittest.skipUnless(sys.platform in ("linux", "android"), "Linux specific test")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI shows that sendmsg raises OSError: [Errno 55] No buffer space available on macOS. I'm not familiar with macOS, so I don't know if it's the expected behavior.

def test_send_fds_dontwait(self):
rfd, wfd = os.pipe()
self.addCleanup(os.close, rfd)
self.addCleanup(os.close, wfd)

sock1, sock2 = socket.socketpair(socket.AF_UNIX, socket.SOCK_DGRAM)
with sock1, sock2:
sock1.setblocking(True)
with self.assertRaises(BlockingIOError):
for _ in range(64 * 1024):
socket.send_fds(sock1, [MSG], [rfd], socket.MSG_DONTWAIT)

msg, fds, flags, addr = socket.recv_fds(sock2, len(MSG), 1)
self._cleanup_fds(fds)

self.assertEqual(msg, MSG)
self.assertEqual(len(fds), 1)
self._test_pipe(fds[0], wfd, MSG)


class FreeThreadingTests(unittest.TestCase):
Expand Down
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`.
Loading