From 9818142b1bd20243733a953fb8aa2c7be314c47c Mon Sep 17 00:00:00 2001 From: Yury Selivanov Date: Mon, 18 Dec 2017 20:02:54 -0500 Subject: bpo-32331: Fix socket.type when SOCK_NONBLOCK is available (#4877) --- Doc/library/socket.rst | 22 ++++++++++++ Doc/whatsnew/3.7.rst | 7 ++++ Lib/socket.py | 6 +--- Lib/test/test_asyncore.py | 10 ++---- Lib/test/test_socket.py | 41 ++++++++++++++++++---- .../2017-12-15-23-48-43.bpo-32331.fIg1Uc.rst | 4 +++ Modules/socketmodule.c | 21 +++++++---- 7 files changed, 87 insertions(+), 24 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 42fd7ea..db032ca 100644 --- a/Doc/library/socket.rst +++ b/Doc/library/socket.rst @@ -482,6 +482,20 @@ The following functions all create :ref:`socket objects `. .. versionchanged:: 3.7 The CAN_ISOTP protocol was added. + .. versionchanged:: 3.7 + 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 system `socket()` call. Therefore:: + + sock = socket.socket( + socket.AF_INET, + socket.SOCK_STREAM | socket.SOCK_NONBLOCK) + + 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]]]) Build a pair of connected socket objects using the given address family, socket @@ -1417,6 +1431,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`. + .. method:: socket.settimeout(value) @@ -1429,6 +1447,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`. + .. method:: socket.setsockopt(level, optname, value: int) .. method:: socket.setsockopt(level, optname, value: buffer) diff --git a/Doc/whatsnew/3.7.rst b/Doc/whatsnew/3.7.rst index 82f7cc0..e5523ff 100644 --- a/Doc/whatsnew/3.7.rst +++ b/Doc/whatsnew/3.7.rst @@ -892,6 +892,13 @@ Changes in the Python API recent to be more consistent with :mod:`traceback`. (Contributed by Jesse Bakker in :issue:`32121`.) +* 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`.) + .. _Unicode Technical Standard #18: https://unicode.org/reports/tr18/ * On Windows the default for the *close_fds* argument of diff --git a/Lib/socket.py b/Lib/socket.py index 1ada24d..2d8aee3 100644 --- a/Lib/socket.py +++ b/Lib/socket.py @@ -203,11 +203,7 @@ class socket(_socket.socket): 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 ee0c3b3..694ddff 100644 --- a/Lib/test/test_asyncore.py +++ b/Lib/test/test_asyncore.py @@ -726,14 +726,10 @@ class BaseTestAPI: 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 5b4c5f9..43688ea 100644 --- a/Lib/test/test_socket.py +++ b/Lib/test/test_socket.py @@ -1577,6 +1577,22 @@ class GeneralModuleTests(unittest.TestCase): 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,18 @@ class GeneralModuleTests(unittest.TestCase): # 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( + 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) + self.assertEqual(s.type, unknown_type) @unittest.skipUnless(hasattr(os, 'sendfile'), 'test needs os.sendfile()') def test__sendfile_use_sendfile(self): @@ -5084,7 +5109,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 +5174,15 @@ class InheritanceTest(unittest.TestCase): 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 0000000..53a262c --- /dev/null +++ b/Misc/NEWS.d/next/Library/2017-12-15-23-48-43.bpo-32331.fIg1Uc.rst @@ -0,0 +1,4 @@ +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 91c879f..d52d9db 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; + s->sock_type = type; + + /* It's possible to pass SOCK_NONBLOCK and SOCK_CLOEXEC bit flags + 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. + */ +#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; s->errorhandler = &set_error; -- cgit v0.12