From 3d9ca1f73c03d09bd293d5dfe8a5c960c44e8967 Mon Sep 17 00:00:00 2001 From: Yury Selivanov Date: Fri, 15 Dec 2017 23:48:52 -0500 Subject: [PATCH 1/8] bpo-32331: Don't obstruct socket.type with SOCK_NONBLOCK and SOCK_CLOEXEC on Linux --- Doc/library/socket.rst | 13 +++++++ Lib/socket.py | 10 +++--- Lib/test/test_asyncore.py | 10 ++---- Lib/test/test_socket.py | 35 +++++++++++++++---- .../2017-12-15-23-48-43.bpo-32331.fIg1Uc.rst | 3 ++ Modules/socketmodule.c | 21 +++++++---- 6 files changed, 67 insertions(+), 25 deletions(-) create mode 100644 Misc/NEWS.d/next/Library/2017-12-15-23-48-43.bpo-32331.fIg1Uc.rst diff --git a/Doc/library/socket.rst b/Doc/library/socket.rst index 42fd7ea0f0bd044..0bf909f494498b5 100644 --- a/Doc/library/socket.rst +++ b/Doc/library/socket.rst @@ -482,6 +482,11 @@ The following functions all create :ref:`socket objects `. .. versionchanged:: 3.7 The CAN_ISOTP protocol was added. + .. versionchanged:: 3.7 + On Linux: when :const:`SOCK_NONBLOCK` or :const:`SOCK_CLOEXEC` bit + flags are applied to *type* they are cleared, and :attr:`socket.type` + will not reflect them. + .. function:: socketpair([family[, type[, proto]]]) Build a pair of connected socket objects using the given address family, socket @@ -1417,6 +1422,10 @@ to sockets. * ``sock.setblocking(False)`` is equivalent to ``sock.settimeout(0.0)`` + .. versionchanged:: 3.7 + The method no longer applies :const:`SOCK_NONBLOCK` flag on + :attr:`socket.type` on Linux. + .. method:: socket.settimeout(value) @@ -1429,6 +1438,10 @@ to sockets. For further information, please consult the :ref:`notes on socket timeouts `. + .. versionchanged:: 3.7 + The method no longer toggles :const:`SOCK_NONBLOCK` flag on + :attr:`socket.type` on Linux. + .. method:: socket.setsockopt(level, optname, value: int) .. method:: socket.setsockopt(level, optname, value: buffer) diff --git a/Lib/socket.py b/Lib/socket.py index 1ada24d33264225..e4d46b3070f2bac 100644 --- a/Lib/socket.py +++ b/Lib/socket.py @@ -78,7 +78,9 @@ IntEnum._convert( 'SocketKind', __name__, - lambda C: C.isupper() and C.startswith('SOCK_')) + lambda C: (C.isupper() and + C.startswith('SOCK_') and + C not in {'SOCK_NONBLOCK', 'SOCK_CLOEXEC'})) IntFlag._convert( 'MsgFlag', @@ -203,11 +205,7 @@ def accept(self): For IP sockets, the address info is a pair (hostaddr, port). """ fd, addr = self._accept() - # If our type has the SOCK_NONBLOCK flag, we shouldn't pass it onto the - # new socket. We do not currently allow passing SOCK_NONBLOCK to - # accept4, so the returned socket is always blocking. - type = self.type & ~globals().get("SOCK_NONBLOCK", 0) - sock = socket(self.family, type, self.proto, fileno=fd) + sock = socket(self.family, self.type, self.proto, fileno=fd) # Issue #7995: if no default timeout is set and the listening # socket had a (non-zero) timeout, force the new socket in blocking # mode to override platform-specific socket flags inheritance. diff --git a/Lib/test/test_asyncore.py b/Lib/test/test_asyncore.py index ee0c3b371f8be01..694ddffd6879ebb 100644 --- a/Lib/test/test_asyncore.py +++ b/Lib/test/test_asyncore.py @@ -726,14 +726,10 @@ def test_connection_attributes(self): def test_create_socket(self): s = asyncore.dispatcher() s.create_socket(self.family) + self.assertEqual(s.socket.type, socket.SOCK_STREAM) self.assertEqual(s.socket.family, self.family) - SOCK_NONBLOCK = getattr(socket, 'SOCK_NONBLOCK', 0) - sock_type = socket.SOCK_STREAM | SOCK_NONBLOCK - if hasattr(socket, 'SOCK_CLOEXEC'): - self.assertIn(s.socket.type, - (sock_type | socket.SOCK_CLOEXEC, sock_type)) - else: - self.assertEqual(s.socket.type, sock_type) + self.assertEqual(s.socket.gettimeout(), 0) + self.assertFalse(s.socket.get_inheritable()) def test_bind(self): if HAS_UNIX_SOCKETS and self.family == socket.AF_UNIX: diff --git a/Lib/test/test_socket.py b/Lib/test/test_socket.py index 5b4c5f9f8ca61ba..2744ee392585d34 100644 --- a/Lib/test/test_socket.py +++ b/Lib/test/test_socket.py @@ -1577,6 +1577,22 @@ def test_str_for_enums(self): self.assertEqual(str(s.family), 'AddressFamily.AF_INET') self.assertEqual(str(s.type), 'SocketKind.SOCK_STREAM') + def test_socket_consistent_sock_type(self): + SOCK_NONBLOCK = getattr(socket, 'SOCK_NONBLOCK', 0) + SOCK_CLOEXEC = getattr(socket, 'SOCK_CLOEXEC', 0) + sock_type = socket.SOCK_STREAM | SOCK_NONBLOCK | SOCK_CLOEXEC + + with socket.socket(socket.AF_INET, sock_type) as s: + self.assertEqual(s.type, socket.SOCK_STREAM) + s.settimeout(1) + self.assertEqual(s.type, socket.SOCK_STREAM) + s.settimeout(0) + self.assertEqual(s.type, socket.SOCK_STREAM) + s.setblocking(True) + self.assertEqual(s.type, socket.SOCK_STREAM) + s.setblocking(False) + self.assertEqual(s.type, socket.SOCK_STREAM) + @unittest.skipIf(os.name == 'nt', 'Will not work on Windows') def test_uknown_socket_family_repr(self): # Test that when created with a family that's not one of the known @@ -1589,9 +1605,12 @@ def test_uknown_socket_family_repr(self): # On Windows this trick won't work, so the test is skipped. fd, path = tempfile.mkstemp() self.addCleanup(os.unlink, path) - with socket.socket(family=42424, type=13331, fileno=fd) as s: - self.assertEqual(s.family, 42424) - self.assertEqual(s.type, 13331) + unknown_family = max(socket.AddressFamily.__members__.values()) + 1 + unknown_type = max(socket.SocketKind.__members__.values()) + 1 + with socket.socket( + family=unknown_family, type=unknown_type, fileno=fd) as s: + self.assertEqual(s.family, unknown_family) + self.assertEqual(s.type, unknown_type) @unittest.skipUnless(hasattr(os, 'sendfile'), 'test needs os.sendfile()') def test__sendfile_use_sendfile(self): @@ -5084,7 +5103,7 @@ class InheritanceTest(unittest.TestCase): def test_SOCK_CLOEXEC(self): with socket.socket(socket.AF_INET, socket.SOCK_STREAM | socket.SOCK_CLOEXEC) as s: - self.assertTrue(s.type & socket.SOCK_CLOEXEC) + self.assertEqual(s.type, socket.SOCK_STREAM) self.assertFalse(s.get_inheritable()) def test_default_inheritable(self): @@ -5149,11 +5168,15 @@ def test_socketpair(self): class NonblockConstantTest(unittest.TestCase): def checkNonblock(self, s, nonblock=True, timeout=0.0): if nonblock: - self.assertTrue(s.type & socket.SOCK_NONBLOCK) + self.assertEqual(s.type, socket.SOCK_STREAM) self.assertEqual(s.gettimeout(), timeout) + self.assertTrue( + fcntl.fcntl(s, fcntl.F_GETFL, os.O_NONBLOCK) & os.O_NONBLOCK) else: - self.assertFalse(s.type & socket.SOCK_NONBLOCK) + self.assertEqual(s.type, socket.SOCK_STREAM) self.assertEqual(s.gettimeout(), None) + self.assertFalse( + fcntl.fcntl(s, fcntl.F_GETFL, os.O_NONBLOCK) & os.O_NONBLOCK) @support.requires_linux_version(2, 6, 28) def test_SOCK_NONBLOCK(self): diff --git a/Misc/NEWS.d/next/Library/2017-12-15-23-48-43.bpo-32331.fIg1Uc.rst b/Misc/NEWS.d/next/Library/2017-12-15-23-48-43.bpo-32331.fIg1Uc.rst new file mode 100644 index 000000000000000..c74f0d4294a1019 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2017-12-15-23-48-43.bpo-32331.fIg1Uc.rst @@ -0,0 +1,3 @@ +Fix socket.settimeout() and socket.setblocking() to not change socket.type +on Linux. Fix socket.socket() constructor to reset any bit flags applied to +socket's type. This change only affects Linux. diff --git a/Modules/socketmodule.c b/Modules/socketmodule.c index 91c879f9d6355fb..e815d3489d02e70 100644 --- a/Modules/socketmodule.c +++ b/Modules/socketmodule.c @@ -582,12 +582,6 @@ internal_setblocking(PySocketSockObject *s, int block) && !((defined(HAVE_SYS_IOCTL_H) && defined(FIONBIO))) int delay_flag, new_delay_flag; #endif -#ifdef SOCK_NONBLOCK - if (block) - s->sock_type &= (~SOCK_NONBLOCK); - else - s->sock_type |= SOCK_NONBLOCK; -#endif Py_BEGIN_ALLOW_THREADS #ifndef MS_WINDOWS @@ -876,7 +870,22 @@ init_sockobject(PySocketSockObject *s, { s->sock_fd = fd; s->sock_family = family; + +#ifdef __linux__ + /* It's possible to pass SOCK_NONBLOCK and SOCK_CLOEXEC bit flags + on Linux as part of socket.type. We want to reset them here, + to make socket.type be set to the same value on all platforms. + Otherwise, simple code like 'if sock.type == SOCK_STREAM' is + not portable. + + 0xF is SOCK_TYPE_MASK in include/linux/net.h which on Linux + is used to mask off SOCK_NONBLOCK and SOCK_CLOEXEC. + */ + s->sock_type = type & 0xF; +#else s->sock_type = type; +#endif + s->sock_proto = proto; s->errorhandler = &set_error; From dae478425fde4977bab1258629d86424ac9f261a Mon Sep 17 00:00:00 2001 From: Yury Selivanov Date: Mon, 18 Dec 2017 13:13:55 -0500 Subject: [PATCH 2/8] Add a comment; fix docs --- Doc/library/socket.rst | 14 +++++++++++--- Lib/socket.py | 2 ++ 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/Doc/library/socket.rst b/Doc/library/socket.rst index 0bf909f494498b5..db4c77dff999a74 100644 --- a/Doc/library/socket.rst +++ b/Doc/library/socket.rst @@ -483,9 +483,17 @@ The following functions all create :ref:`socket objects `. The CAN_ISOTP protocol was added. .. versionchanged:: 3.7 - On Linux: when :const:`SOCK_NONBLOCK` or :const:`SOCK_CLOEXEC` bit - flags are applied to *type* they are cleared, and :attr:`socket.type` - will not reflect them. + Linux-specific: when :const:`SOCK_NONBLOCK` or :const:`SOCK_CLOEXEC` + bit flags are applied to *type* they are cleared, and + :attr:`socket.type` will not reflect them. They are still passed + to the underlying Linux' `socket()` call, therefore:: + + sock = socket.socket( + socket.AF_INET, + socket.SOCK_STREAM | socket.SOCK_NONBLOCK) + + will still create a non-blocking socket on Linux, but + ``sock.type`` will be set to ``socket.SOCK_STREAM``. .. function:: socketpair([family[, type[, proto]]]) diff --git a/Lib/socket.py b/Lib/socket.py index e4d46b3070f2bac..0ea4d6c230872f1 100644 --- a/Lib/socket.py +++ b/Lib/socket.py @@ -80,6 +80,8 @@ __name__, lambda C: (C.isupper() and C.startswith('SOCK_') and + # SOCK_NONBLOCK and SOCK_CLOEXEC are not socket types, + # they are Linux-specific bit flags. C not in {'SOCK_NONBLOCK', 'SOCK_CLOEXEC'})) IntFlag._convert( From ed40d775436b276c67ac4b1c82ae2859f329def1 Mon Sep 17 00:00:00 2001 From: Yury Selivanov Date: Mon, 18 Dec 2017 17:24:30 -0500 Subject: [PATCH 3/8] Address Victor's feedback --- Doc/whatsnew/3.7.rst | 6 ++++++ Modules/socketmodule.c | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/Doc/whatsnew/3.7.rst b/Doc/whatsnew/3.7.rst index 3574b53ad180095..ab1b84d168ded37 100644 --- a/Doc/whatsnew/3.7.rst +++ b/Doc/whatsnew/3.7.rst @@ -881,6 +881,12 @@ Changes in the Python API recent to be more consistent with :mod:`traceback`. (Contributed by Jesse Bakker in :issue:`32121`.) +* On Linux, the :attr:`socket.type ` attribute no + longer has :const:`socket.SOCK_NONBLOCK` flag applied, so checks like + ``if sock.type == socket.SOCK_STREAM`` work as expected on all + platforms. + (Contributed by Yury Selivanov in :issue:`32331`.) + .. _Unicode Technical Standard #18: https://unicode.org/reports/tr18/ diff --git a/Modules/socketmodule.c b/Modules/socketmodule.c index e815d3489d02e70..1c321c4f2f8f822 100644 --- a/Modules/socketmodule.c +++ b/Modules/socketmodule.c @@ -881,7 +881,7 @@ init_sockobject(PySocketSockObject *s, 0xF is SOCK_TYPE_MASK in include/linux/net.h which on Linux is used to mask off SOCK_NONBLOCK and SOCK_CLOEXEC. */ - s->sock_type = type & 0xF; + s->sock_type = type & ~(SOCK_NONBLOCK | SOCK_CLOEXEC); #else s->sock_type = type; #endif From ee6a0240d27cb129563e7e5a419803d83727937a Mon Sep 17 00:00:00 2001 From: Yury Selivanov Date: Mon, 18 Dec 2017 17:27:01 -0500 Subject: [PATCH 4/8] Another small update of what's new --- Doc/whatsnew/3.7.rst | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Doc/whatsnew/3.7.rst b/Doc/whatsnew/3.7.rst index ab1b84d168ded37..5ac666b8a06ba5d 100644 --- a/Doc/whatsnew/3.7.rst +++ b/Doc/whatsnew/3.7.rst @@ -882,9 +882,9 @@ Changes in the Python API (Contributed by Jesse Bakker in :issue:`32121`.) * On Linux, the :attr:`socket.type ` attribute no - longer has :const:`socket.SOCK_NONBLOCK` flag applied, so checks like - ``if sock.type == socket.SOCK_STREAM`` work as expected on all - platforms. + longer has :const:`socket.SOCK_NONBLOCK` or :const:`socket.SOCK_CLOEXEC` + flags applied, so checks like ``if sock.type == socket.SOCK_STREAM`` + work as expected on all platforms. (Contributed by Yury Selivanov in :issue:`32331`.) .. _Unicode Technical Standard #18: https://unicode.org/reports/tr18/ From 8e5bd2b16519c9e1b3db35b31be5c6e3fa030946 Mon Sep 17 00:00:00 2001 From: Yury Selivanov Date: Mon, 18 Dec 2017 17:40:57 -0500 Subject: [PATCH 5/8] Don't use ifdef __linux__; reset SOCK_* flags explicitly --- Modules/socketmodule.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/Modules/socketmodule.c b/Modules/socketmodule.c index 1c321c4f2f8f822..becdb83c001d868 100644 --- a/Modules/socketmodule.c +++ b/Modules/socketmodule.c @@ -871,7 +871,8 @@ init_sockobject(PySocketSockObject *s, s->sock_fd = fd; s->sock_family = family; -#ifdef __linux__ + s->sock_type = type; + /* It's possible to pass SOCK_NONBLOCK and SOCK_CLOEXEC bit flags on Linux as part of socket.type. We want to reset them here, to make socket.type be set to the same value on all platforms. @@ -881,9 +882,11 @@ init_sockobject(PySocketSockObject *s, 0xF is SOCK_TYPE_MASK in include/linux/net.h which on Linux is used to mask off SOCK_NONBLOCK and SOCK_CLOEXEC. */ - s->sock_type = type & ~(SOCK_NONBLOCK | SOCK_CLOEXEC); -#else - s->sock_type = type; +#ifdef SOCK_NONBLOCK + s->sock_type = s->sock_type & ~SOCK_NONBLOCK; +#endif +#ifdef SOCK_CLOEXEC + s->sock_type = s->sock_type & ~SOCK_CLOEXEC; #endif s->sock_proto = proto; From 7e9eb5339be196366ffe45f6ec1d8ea4239dfc6e Mon Sep 17 00:00:00 2001 From: Yury Selivanov Date: Mon, 18 Dec 2017 19:04:24 -0500 Subject: [PATCH 6/8] More fixes per Victor's review. --- Doc/library/socket.rst | 13 +++++++------ Doc/whatsnew/3.7.rst | 7 ++++--- .../2017-12-15-23-48-43.bpo-32331.fIg1Uc.rst | 7 ++++--- Modules/socketmodule.c | 5 +---- 4 files changed, 16 insertions(+), 16 deletions(-) diff --git a/Doc/library/socket.rst b/Doc/library/socket.rst index db4c77dff999a74..db032ca7f1f2c34 100644 --- a/Doc/library/socket.rst +++ b/Doc/library/socket.rst @@ -483,17 +483,18 @@ The following functions all create :ref:`socket objects `. The CAN_ISOTP protocol was added. .. versionchanged:: 3.7 - Linux-specific: when :const:`SOCK_NONBLOCK` or :const:`SOCK_CLOEXEC` + When :const:`SOCK_NONBLOCK` or :const:`SOCK_CLOEXEC` bit flags are applied to *type* they are cleared, and :attr:`socket.type` will not reflect them. They are still passed - to the underlying Linux' `socket()` call, therefore:: + to the underlying system `socket()` call. Therefore:: sock = socket.socket( socket.AF_INET, socket.SOCK_STREAM | socket.SOCK_NONBLOCK) - will still create a non-blocking socket on Linux, but - ``sock.type`` will be set to ``socket.SOCK_STREAM``. + will still create a non-blocking socket on OSes that support + ``SOCK_NONBLOCK``, but ``sock.type`` will be set to + ``socket.SOCK_STREAM``. .. function:: socketpair([family[, type[, proto]]]) @@ -1432,7 +1433,7 @@ to sockets. .. versionchanged:: 3.7 The method no longer applies :const:`SOCK_NONBLOCK` flag on - :attr:`socket.type` on Linux. + :attr:`socket.type`. .. method:: socket.settimeout(value) @@ -1448,7 +1449,7 @@ to sockets. .. versionchanged:: 3.7 The method no longer toggles :const:`SOCK_NONBLOCK` flag on - :attr:`socket.type` on Linux. + :attr:`socket.type`. .. method:: socket.setsockopt(level, optname, value: int) diff --git a/Doc/whatsnew/3.7.rst b/Doc/whatsnew/3.7.rst index 5ac666b8a06ba5d..3766e4677b59ece 100644 --- a/Doc/whatsnew/3.7.rst +++ b/Doc/whatsnew/3.7.rst @@ -881,9 +881,10 @@ Changes in the Python API recent to be more consistent with :mod:`traceback`. (Contributed by Jesse Bakker in :issue:`32121`.) -* On Linux, the :attr:`socket.type ` attribute no - longer has :const:`socket.SOCK_NONBLOCK` or :const:`socket.SOCK_CLOEXEC` - flags applied, so checks like ``if sock.type == socket.SOCK_STREAM`` +* On OSes that support :const:`socket.SOCK_NONBLOCK` or + :const:`socket.SOCK_CLOEXEC` bit flags, the + :attr:`socket.type ` no longer has them applied. + Therefore, checks like ``if sock.type == socket.SOCK_STREAM`` work as expected on all platforms. (Contributed by Yury Selivanov in :issue:`32331`.) diff --git a/Misc/NEWS.d/next/Library/2017-12-15-23-48-43.bpo-32331.fIg1Uc.rst b/Misc/NEWS.d/next/Library/2017-12-15-23-48-43.bpo-32331.fIg1Uc.rst index c74f0d4294a1019..53a262cf2207939 100644 --- a/Misc/NEWS.d/next/Library/2017-12-15-23-48-43.bpo-32331.fIg1Uc.rst +++ b/Misc/NEWS.d/next/Library/2017-12-15-23-48-43.bpo-32331.fIg1Uc.rst @@ -1,3 +1,4 @@ -Fix socket.settimeout() and socket.setblocking() to not change socket.type -on Linux. Fix socket.socket() constructor to reset any bit flags applied to -socket's type. This change only affects Linux. +Fix socket.settimeout() and socket.setblocking() to keep socket.type +as is. Fix socket.socket() constructor to reset any bit flags applied to +socket's type. This change only affects OSes that have SOCK_NONBLOCK +and/or SOCK_CLOEXEC. diff --git a/Modules/socketmodule.c b/Modules/socketmodule.c index becdb83c001d868..d52d9db743a67ff 100644 --- a/Modules/socketmodule.c +++ b/Modules/socketmodule.c @@ -874,13 +874,10 @@ init_sockobject(PySocketSockObject *s, s->sock_type = type; /* It's possible to pass SOCK_NONBLOCK and SOCK_CLOEXEC bit flags - on Linux as part of socket.type. We want to reset them here, + on some OSes as part of socket.type. We want to reset them here, to make socket.type be set to the same value on all platforms. Otherwise, simple code like 'if sock.type == SOCK_STREAM' is not portable. - - 0xF is SOCK_TYPE_MASK in include/linux/net.h which on Linux - is used to mask off SOCK_NONBLOCK and SOCK_CLOEXEC. */ #ifdef SOCK_NONBLOCK s->sock_type = s->sock_type & ~SOCK_NONBLOCK; From 7b628035b8778b3f5e8edbdd7fac4d719b24396e Mon Sep 17 00:00:00 2001 From: Yury Selivanov Date: Mon, 18 Dec 2017 19:22:12 -0500 Subject: [PATCH 7/8] Don't cleanup SocketKind; we'll do it in a separate PR --- Lib/socket.py | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/Lib/socket.py b/Lib/socket.py index 0ea4d6c230872f1..2d8aee3e9047369 100644 --- a/Lib/socket.py +++ b/Lib/socket.py @@ -78,11 +78,7 @@ IntEnum._convert( 'SocketKind', __name__, - lambda C: (C.isupper() and - C.startswith('SOCK_') and - # SOCK_NONBLOCK and SOCK_CLOEXEC are not socket types, - # they are Linux-specific bit flags. - C not in {'SOCK_NONBLOCK', 'SOCK_CLOEXEC'})) + lambda C: C.isupper() and C.startswith('SOCK_')) IntFlag._convert( 'MsgFlag', From ad998e428d5ee1cca94b95fe447d5456a30ed1d0 Mon Sep 17 00:00:00 2001 From: Yury Selivanov Date: Mon, 18 Dec 2017 19:46:51 -0500 Subject: [PATCH 8/8] Fix test_uknown_socket_family_repr test --- Lib/test/test_socket.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/Lib/test/test_socket.py b/Lib/test/test_socket.py index 2744ee392585d34..43688eacf06a81c 100644 --- a/Lib/test/test_socket.py +++ b/Lib/test/test_socket.py @@ -1606,7 +1606,13 @@ def test_uknown_socket_family_repr(self): fd, path = tempfile.mkstemp() self.addCleanup(os.unlink, path) unknown_family = max(socket.AddressFamily.__members__.values()) + 1 - unknown_type = max(socket.SocketKind.__members__.values()) + 1 + + unknown_type = max( + kind + for name, kind in socket.SocketKind.__members__.items() + if name not in {'SOCK_NONBLOCK', 'SOCK_CLOEXEC'} + ) + 1 + with socket.socket( family=unknown_family, type=unknown_type, fileno=fd) as s: self.assertEqual(s.family, unknown_family)