From 81b280e7b4a01b81614e8533427a202a782f0c9a Mon Sep 17 00:00:00 2001 From: Stefano Miccoli Date: Mon, 21 Aug 2023 13:42:58 +0200 Subject: [PATCH 1/9] Refactor to check also optional parameters of recv_fds and send_fds Class SendRecvFdsTests is refactored to test also the optional parameters of recv_fds and send_fds. Methods - testSendAndRecvFds: base check, semantically equivalent to legacy version, changes are only for legibility and code reuse in following tests. - testRecvFlags: new test in which the socket.MSG_PEEK flag is passed to recv_fds. socket.MSG_PEEK semantics is checked. - testSendAddress: new test in which the address parameter of send_fds is tested on a connectionless socket of type SOCK_DGRAM There are no tests for the flags of send_fds, because testing MSG_OOB would be way too complicated. --- Lib/test/test_socket.py | 124 ++++++++++++++++++++++++++++++++-------- 1 file changed, 99 insertions(+), 25 deletions(-) diff --git a/Lib/test/test_socket.py b/Lib/test/test_socket.py index 0eaf64257c3b81..fcb4e50f44f749 100644 --- a/Lib/test/test_socket.py +++ b/Lib/test/test_socket.py @@ -6781,42 +6781,116 @@ 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 close_fds(fds): - for fd in fds: - os.close(fd) + def setUp(self): + # make 13 pipes, reading side in rds, writing side in wds + rds, wds = zip(*(os.pipe() for _ in range(13))) + # checks to verify that test setup is OK + self.assertEqual(len(rds), len(wds)) + self.check_pipes_connected(wds, rds) + + self.rds, self.wds = rds, wds - # send 10 file descriptors - pipes = [os.pipe() for _ in range(10)] - self.addCleanup(close_pipes, pipes) - fds = [rfd for rfd, wfd in pipes] + def tearDown(self): + self.close_fds(self.rds + self.wds) + + @staticmethod + def close_fds(fds): + for fd in fds: + os.close(fd) + def check_pipes_connected(self, wds, rds): + for index, fd in enumerate(wds): + os.write(fd, str(index).encode()) + + for index, fd in enumerate(rds): + os.set_blocking(fd, False) + data = os.read(fd, 1024) + self.assertEqual(data, str(index).encode()) + + def testSendAndRecvFds(self): # use a UNIX socket pair to exchange file descriptors locally sock1, sock2 = socket.socketpair(socket.AF_UNIX, socket.SOCK_STREAM) with sock1, sock2: - 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) + # send the reading fds over sock1 + socket.send_fds(sock1, [MSG], self.rds) + # receive them on sock2 + # allocate space for more data and file descriptors than expected + msg, rds2, flags, addr = socket.recv_fds( + sock2, len(MSG) * 2, len(self.rds) * 2 + ) + self.addCleanup(self.close_fds, rds2) self.assertEqual(msg, MSG) - self.assertEqual(len(fds2), len(fds)) + self.assertEqual(len(rds2), len(self.rds)) self.assertEqual(flags, 0) - # don't test addr + # addr contains no useful info and is not checked + self.check_pipes_connected(self.wds, rds2) - # test that file descriptors are connected - for index, fds in enumerate(pipes): - rfd, wfd = fds - os.write(wfd, str(index).encode()) + def testRecvFlags(self): + # use a UNIX socket pair to exchange file descriptors locally + # set to non blocking to avoid hangs on errors + with socket_setdefaulttimeout(0): + sock1, sock2 = socket.socketpair(socket.AF_UNIX, socket.SOCK_STREAM) + with sock1, sock2: + # send the reading fds over sock1 + socket.send_fds(sock1, [MSG], self.rds) + # receive them on sock2, with socket.MSG_PEEK + # allocate space for more data and file descriptors than expected + msg, rds2, flags, addr = socket.recv_fds( + sock2, len(MSG) * 2, len(self.rds) * 2, socket.MSG_PEEK + ) + + self.assertEqual(msg, MSG) + self.assertEqual(len(rds2), len(self.rds)) + self.assertEqual(flags, socket.MSG_PEEK) + # don't test addr for anonymous socket pair + # when PEEKing, rds2 are not open fds and should be all 0s + self.assertEqual(rds2, [0]*len(rds2)) + + # receive again without socket.MSG_PEEK + msg, rds2, flags, addr = socket.recv_fds( + sock2, len(MSG) * 2, len(self.rds) * 2 + ) + self.addCleanup(self.close_fds, rds2) + + self.assertEqual(msg, MSG) + self.assertEqual(len(rds2), len(self.rds)) + self.assertEqual(flags, 0) + # addr contains no useful info and is not checked + self.check_pipes_connected(self.wds, rds2) + + # check that there is no more data waiting + with self.assertRaises(BlockingIOError): + msg, rds2, flags, addr = socket.recv_fds( + sock2, len(MSG) * 2, len(self.rds) * 2 + ) + + def testSendAddress(self): + # create a bound socket and use a second unbound socket to test + # the address parameter of send_fds + with ( + tempfile.TemporaryDirectory() as tmpdir, + socket.socket(family=socket.AF_UNIX, type=socket.SOCK_DGRAM) as sock1, + socket.socket(family=socket.AF_UNIX, type=socket.SOCK_DGRAM) as sock2 + ): + # bind sock1 + sockpth = os.path.join(tmpdir, "SOCK") + sock1.bind(sockpth) + # send from sock2 + socket.send_fds(sock2, [MSG], self.rds, address=sockpth) + # receive on sock1 + msg, rds2, flags, addr = socket.recv_fds( + sock1, len(MSG) * 2, len(self.rds) * 2 + ) + self.addCleanup(self.close_fds, rds2) + + self.assertEqual(msg, MSG) + self.assertEqual(len(rds2), len(self.rds)) + self.assertEqual(flags, 0) + # addr contains no useful info and is not checked + self.check_pipes_connected(self.wds, rds2) - for index, rfd in enumerate(fds2): - data = os.read(rfd, 100) - self.assertEqual(data, str(index).encode()) def setUpModule(): From 56539551504733543bf97269ec0fd5fd8e5b7c69 Mon Sep 17 00:00:00 2001 From: Stefano Miccoli Date: Mon, 21 Aug 2023 13:46:48 +0200 Subject: [PATCH 2/9] Fix gh-107898 by honouring the optional flags of send_fds and recv_fds Optional parameters (flags and adress) are passed to undelying sendmsg and recvmsg functions. Minor code formatting for legibility. --- Lib/socket.py | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/Lib/socket.py b/Lib/socket.py index 321fcda51505c1..fa7afc8defb842 100644 --- a/Lib/socket.py +++ b/Lib/socket.py @@ -555,8 +555,12 @@ def send_fds(sock, buffers, fds, flags=0, address=None): 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))]) + return sock.sendmsg( + buffers, + [(_socket.SOL_SOCKET, _socket.SCM_RIGHTS, array.array("i", fds))], + flags, + address, + ) __all__.append("send_fds") if hasattr(_socket.socket, "recvmsg"): @@ -571,14 +575,18 @@ 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)) + msg, ancdata, msg_flags, addr = sock.recvmsg( + bufsize, _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[: - len(cmsg_data) - (len(cmsg_data) % fds.itemsize)]) - - return msg, list(fds), flags, addr + if (cmsg_level == _socket.SOL_SOCKET + and cmsg_type == _socket.SCM_RIGHTS): + # Append data, ignoring any truncated integers at the end. + fds.frombytes( + cmsg_data[:len(cmsg_data) - (len(cmsg_data) % fds.itemsize)] + ) + + return msg, list(fds), msg_flags, addr __all__.append("recv_fds") if hasattr(_socket.socket, "share"): From 92d72651a9df07fd41f0ba640a35a2b4c19b7450 Mon Sep 17 00:00:00 2001 From: Stefano Miccoli Date: Mon, 21 Aug 2023 14:18:11 +0200 Subject: [PATCH 3/9] Docs: drop meaningless note in socket.recv_fds description An inconclusive sentence is dropped: the code should be able to handle truncated integers at the end of cmsg_data, no need to alert the user. --- Doc/library/socket.rst | 4 ---- 1 file changed, 4 deletions(-) diff --git a/Doc/library/socket.rst b/Doc/library/socket.rst index 4f220e8a098979..ef17e2c8f1cfa7 100644 --- a/Doc/library/socket.rst +++ b/Doc/library/socket.rst @@ -1347,10 +1347,6 @@ The :mod:`socket` module also offers various network-related services: .. versionadded:: 3.9 - .. note:: - - Any truncated integers at the end of the list of file descriptors. - .. _socket-objects: From 8561b4848e2c9a7556c927e70cdacfb475017e11 Mon Sep 17 00:00:00 2001 From: Stefano Miccoli Date: Tue, 22 Aug 2023 01:10:46 +0200 Subject: [PATCH 4/9] Fix flags error on Linux platform --- Lib/test/test_socket.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Lib/test/test_socket.py b/Lib/test/test_socket.py index fcb4e50f44f749..0d3e4e49c07d35 100644 --- a/Lib/test/test_socket.py +++ b/Lib/test/test_socket.py @@ -6843,7 +6843,8 @@ def testRecvFlags(self): self.assertEqual(msg, MSG) self.assertEqual(len(rds2), len(self.rds)) - self.assertEqual(flags, socket.MSG_PEEK) + # flags can be 0 or socket.MSG_PEEK: + self.assertEqual(flags & ~socket.MSG_PEEK, 0) # don't test addr for anonymous socket pair # when PEEKing, rds2 are not open fds and should be all 0s self.assertEqual(rds2, [0]*len(rds2)) From 4e172c05efbf7668b5325349151cfc182d2d3947 Mon Sep 17 00:00:00 2001 From: Stefano Miccoli Date: Tue, 22 Aug 2023 01:11:06 +0200 Subject: [PATCH 5/9] Add blurb --- .../next/Library/2023-08-22-01-08-12.gh-issue-107898.eH3Y4r.rst | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 Misc/NEWS.d/next/Library/2023-08-22-01-08-12.gh-issue-107898.eH3Y4r.rst diff --git a/Misc/NEWS.d/next/Library/2023-08-22-01-08-12.gh-issue-107898.eH3Y4r.rst b/Misc/NEWS.d/next/Library/2023-08-22-01-08-12.gh-issue-107898.eH3Y4r.rst new file mode 100644 index 00000000000000..a10ec08546b28f --- /dev/null +++ b/Misc/NEWS.d/next/Library/2023-08-22-01-08-12.gh-issue-107898.eH3Y4r.rst @@ -0,0 +1,2 @@ +Make :func:`socket.send_fds` and func:`socket.recv_fds` honour optional +arguments. Patch by Stefano Miccoli. From 9ce8093d8cc63b83ceaf71099dbeff1668084d79 Mon Sep 17 00:00:00 2001 From: Stefano Miccoli Date: Tue, 22 Aug 2023 01:20:48 +0200 Subject: [PATCH 6/9] Fix rst blurb --- .../next/Library/2023-08-22-01-08-12.gh-issue-107898.eH3Y4r.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Misc/NEWS.d/next/Library/2023-08-22-01-08-12.gh-issue-107898.eH3Y4r.rst b/Misc/NEWS.d/next/Library/2023-08-22-01-08-12.gh-issue-107898.eH3Y4r.rst index a10ec08546b28f..c616f717bad38d 100644 --- a/Misc/NEWS.d/next/Library/2023-08-22-01-08-12.gh-issue-107898.eH3Y4r.rst +++ b/Misc/NEWS.d/next/Library/2023-08-22-01-08-12.gh-issue-107898.eH3Y4r.rst @@ -1,2 +1,2 @@ -Make :func:`socket.send_fds` and func:`socket.recv_fds` honour optional +Make :func:`socket.send_fds` and :func:`socket.recv_fds` honour optional arguments. Patch by Stefano Miccoli. From 59c31b38b0db6553fc21d7473c637920cb535f3b Mon Sep 17 00:00:00 2001 From: Stefano Miccoli Date: Tue, 22 Aug 2023 12:00:36 +0200 Subject: [PATCH 7/9] Fix msg_flags in recv_fds tests When receiving fds with the MSG_PEEK flag, two cases may arise. 1. The incoming msg has msg_flags == 0 and ancillary data contains open fd's (seen on linux); 2. msg_flags == MSG_PEEK and ancillary data contains a null array (seen on darwin). --- Lib/test/test_socket.py | 33 ++++++++++++++++++++------------- 1 file changed, 20 insertions(+), 13 deletions(-) diff --git a/Lib/test/test_socket.py b/Lib/test/test_socket.py index 0d3e4e49c07d35..317a1600ee861a 100644 --- a/Lib/test/test_socket.py +++ b/Lib/test/test_socket.py @@ -6816,14 +6816,14 @@ def testSendAndRecvFds(self): socket.send_fds(sock1, [MSG], self.rds) # receive them on sock2 # allocate space for more data and file descriptors than expected - msg, rds2, flags, addr = socket.recv_fds( + msg, rds2, msg_flags, addr = socket.recv_fds( sock2, len(MSG) * 2, len(self.rds) * 2 ) self.addCleanup(self.close_fds, rds2) self.assertEqual(msg, MSG) self.assertEqual(len(rds2), len(self.rds)) - self.assertEqual(flags, 0) + self.assertEqual(msg_flags, 0) # addr contains no useful info and is not checked self.check_pipes_connected(self.wds, rds2) @@ -6837,33 +6837,40 @@ def testRecvFlags(self): socket.send_fds(sock1, [MSG], self.rds) # receive them on sock2, with socket.MSG_PEEK # allocate space for more data and file descriptors than expected - msg, rds2, flags, addr = socket.recv_fds( + msg, rds2, msg_flags, addr = socket.recv_fds( sock2, len(MSG) * 2, len(self.rds) * 2, socket.MSG_PEEK ) self.assertEqual(msg, MSG) self.assertEqual(len(rds2), len(self.rds)) - # flags can be 0 or socket.MSG_PEEK: - self.assertEqual(flags & ~socket.MSG_PEEK, 0) - # don't test addr for anonymous socket pair - # when PEEKing, rds2 are not open fds and should be all 0s - self.assertEqual(rds2, [0]*len(rds2)) + # msg_flags can be 0 or socket.MSG_PEEK: + self.assertEqual(msg_flags & ~socket.MSG_PEEK, 0) + # addr contains no useful info and is not checked + if msg_flags == 0: + # rds2 are open and connected + self.addCleanup(self.close_fds, rds2) + self.check_pipes_connected(self.wds, rds2) + elif msg_flags == socket.MSG_PEEK: + # rds2 are not open fds and should be all 0s + self.assertEqual(rds2, [0]*len(rds2)) + else: + assert False, "message msg_flags has unexpected value" # receive again without socket.MSG_PEEK - msg, rds2, flags, addr = socket.recv_fds( + msg, rds2, msg_flags, addr = socket.recv_fds( sock2, len(MSG) * 2, len(self.rds) * 2 ) self.addCleanup(self.close_fds, rds2) self.assertEqual(msg, MSG) self.assertEqual(len(rds2), len(self.rds)) - self.assertEqual(flags, 0) + self.assertEqual(msg_flags, 0) # addr contains no useful info and is not checked self.check_pipes_connected(self.wds, rds2) # check that there is no more data waiting with self.assertRaises(BlockingIOError): - msg, rds2, flags, addr = socket.recv_fds( + msg, rds2, msg_flags, addr = socket.recv_fds( sock2, len(MSG) * 2, len(self.rds) * 2 ) @@ -6881,14 +6888,14 @@ def testSendAddress(self): # send from sock2 socket.send_fds(sock2, [MSG], self.rds, address=sockpth) # receive on sock1 - msg, rds2, flags, addr = socket.recv_fds( + msg, rds2, msg_flags, addr = socket.recv_fds( sock1, len(MSG) * 2, len(self.rds) * 2 ) self.addCleanup(self.close_fds, rds2) self.assertEqual(msg, MSG) self.assertEqual(len(rds2), len(self.rds)) - self.assertEqual(flags, 0) + self.assertEqual(msg_flags, 0) # addr contains no useful info and is not checked self.check_pipes_connected(self.wds, rds2) From a563a78fb4304bce55efbb1b37b544307a8c7072 Mon Sep 17 00:00:00 2001 From: Stefano Miccoli Date: Tue, 22 Aug 2023 12:21:00 +0200 Subject: [PATCH 8/9] Add docs to unit testing --- Lib/test/test_socket.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/Lib/test/test_socket.py b/Lib/test/test_socket.py index 317a1600ee861a..4061293c44efaf 100644 --- a/Lib/test/test_socket.py +++ b/Lib/test/test_socket.py @@ -6809,6 +6809,8 @@ def check_pipes_connected(self, wds, rds): self.assertEqual(data, str(index).encode()) def testSendAndRecvFds(self): + """basic test over a local socket pair""" + # use a UNIX socket pair to exchange file descriptors locally sock1, sock2 = socket.socketpair(socket.AF_UNIX, socket.SOCK_STREAM) with sock1, sock2: @@ -6828,6 +6830,8 @@ def testSendAndRecvFds(self): self.check_pipes_connected(self.wds, rds2) def testRecvFlags(self): + """same as testSendAndRecvFds but set recv_fds flags to MSG_PEEK""" + # use a UNIX socket pair to exchange file descriptors locally # set to non blocking to avoid hangs on errors with socket_setdefaulttimeout(0): @@ -6875,6 +6879,9 @@ def testRecvFlags(self): ) def testSendAddress(self): + """test receiving a DGRAM over a bound socket, sending with the + 'address' argument of send_fds""" + # create a bound socket and use a second unbound socket to test # the address parameter of send_fds with ( From 044c1f18bfbd6768e9c3f6b4c12e3204c1abbe65 Mon Sep 17 00:00:00 2001 From: Stefano Miccoli Date: Wed, 2 Oct 2024 17:18:05 +0200 Subject: [PATCH 9/9] Reduce changeset with respect to main --- Lib/socket.py | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/Lib/socket.py b/Lib/socket.py index 7edb99b56eee64..2935c20bb0849f 100644 --- a/Lib/socket.py +++ b/Lib/socket.py @@ -563,8 +563,7 @@ 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))], - flags, address) + _socket.SCM_RIGHTS, array.array("i", fds))], flags, address) __all__.append("send_fds") if hasattr(_socket.socket, "recvmsg"): @@ -579,16 +578,13 @@ def recv_fds(sock, bufsize, maxfds, flags=0): # Array of ints fds = array.array("i") - msg, ancdata, msg_flags, addr = sock.recvmsg( - bufsize, _socket.CMSG_LEN(maxfds * fds.itemsize), flags - ) + msg, ancdata, msg_flags, addr = sock.recvmsg(bufsize, + _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): + if (cmsg_level == _socket.SOL_SOCKET and cmsg_type == _socket.SCM_RIGHTS): # Append data, ignoring any truncated integers at the end. - fds.frombytes( - cmsg_data[:len(cmsg_data) - (len(cmsg_data) % fds.itemsize)] - ) + fds.frombytes(cmsg_data[: + len(cmsg_data) - (len(cmsg_data) % fds.itemsize)]) return msg, list(fds), msg_flags, addr __all__.append("recv_fds")