Make loop TCP APIs accept only TCP streaming sockets #453

1st1 · 2016-11-06T21:45:00Z

Currently, loop.create_server, loop.create_connection, and loop.connect_accepted_socket accept any kind of socket, although they are documented (and used by asyncio users) as TCP API.

Allowing those methods to accept any kinds of sockets leads to hard to debug bugs. For instance, aiohttp gunicorn worker simply passes all sockets to create_server (including UNIX sockets). We even have a unittest in asyncio that does that: test_create_datagram_endpoint_sock passes a UDP socket to create_connection.

TCP, UDP and UNIX sockets are different from each other and have some subtle API differences. We shouldn't allow to blindly pass any of them to create_server and other TCP APIs.

gvanrossum · 2016-11-07T00:47:36Z

tests/test_events.py

+        if (sys.platform == 'win32' and
+                isinstance(self.loop, proactor_events.BaseProactorEventLoop)):
+            raise unittest.SkipTest(
+                'UCP not supported with proactor event loops')


gvanrossum · 2016-11-07T00:49:12Z

tests/test_events.py

@@ -1394,7 +1399,7 @@ def test_create_datagram_endpoint_sock(self):
        else:
            assert False, 'Can not create socket.'

-        f = self.loop.create_connection(
+        f = self.loop.create_datagram_endpoint(


I'm worried here that this test doesn't test much...

Yeah, you're right, it tests only that the method doesn't crash. Still it was testing a wrong method.

gvanrossum

(OT: connect_accepted_socket seems undocumented.)

gvanrossum · 2016-11-07T19:29:58Z

asyncio/base_events.py

@@ -1027,6 +1031,9 @@ def create_server(self, protocol_factory, host=None, port=None,
        else:
            if sock is None:
                raise ValueError('Neither host/port nor sock were specified')
+            if sock.family not in {socket.AF_INET, socket.AF_INET6}:


Careful, check line 972/976. AF_INET6 may not always exist in the socket module (apparently).

Good point. Will fix this.

gvanrossum · 2016-11-07T19:38:19Z

asyncio/base_events.py

@@ -1048,6 +1055,13 @@ def connect_accepted_socket(self, protocol_factory, sock, *, ssl=None):
        This method is a coroutine.  When completed, the coroutine
        returns a (transport, protocol) pair.
        """
+        socket_families = {socket.AF_INET, socket.AF_INET6}
+        if hasattr(socket, 'AF_UNIX'):
+            socket_families.add(socket.AF_UNIX)


If a UNIX socket is fine here, why not in create_connection()? They both end up calling _create_connection_transport().

Yes, in asyncio both create_connection and connect_accepted_socket use similar internal API. It's not the same in uvloop.

The problem is that we have create_connection and create_unix_connection, that are designed for TCP and UNIX sockets. uvloop implements this spec, using appropriate low-level libuv primitives for each case. It doesn't make a lot of sense to add "if" checks to accept Unix sockets in create_connection (as we already have dedicated APIs for that).

connect_accepted_socket on the other hand doesn't specify the kind of socket it accepts. And it doesn't make a lot of sense to add connect_accepted_unix_socket.

So maybe connect_accepted_socket() should only check whether it's a stream socket, not whether it's any particular address family?

That means that users will be able to pass AF_BLUETOOTH sockets or something. Those aren't supported in uvloop, and I'm not sure that even asyncio supports them properly.

In any case, I think you're right, let's only check for SOCK_STREAM here. I'll also add SOCK_STREAM checks in create_connection, create_server, as they simply won't work with anything else. AF_INET6 has to be properly handled too. Let me update the PR.

I have some additional worry now. You've mentioned twice (I think) "but uvloop doesn't support that". I don't think that's a valid argument. (Haven't we discussed this before?) I think it's fine if uvloop doesn't support e.g. bluetooth sockets but pure-Python asyncio does. That should be documented with uvloop; but I don't think "uvloop can't do this" is enough to conclude "hence asyncio shouldn't do this either". (Especially since I remember seeing a bug about code that would crash when passing a bluetooth address; IIRC it was a getaddrinfo() issue.)

Re uvloop - I agree. The updated PR only makes sure that:

create_datagram_endpoint only accepts SOCK_DGRAM sockets (it doesn't make sense to accept anything else).

create_connection and create_server only accept AF_INET, AF_INET6 and SOCK_STREAM sockets. We have separate asyncio API for AF_UNIX sockets.

connect_accepted_socket only accepts SOCK_STREAM sockets (no restrictions on family, event AF_BLUETOOTH will be accepted just fine. Even though uvloop won't work with the latter).

All in all, this PR is not about uvloop, it only ensures that asyncio APIs are used correctly. It doesn't limit asyncio in any way.

1st1 · 2016-11-07T20:38:03Z

(OT: connect_accepted_socket seems undocumented.)

I've committed Jim's documentation patch: http://bugs.python.org/issue27392. I've also updated this PR to address your comments.

gvanrossum · 2016-11-09T05:54:18Z

Thanks, LGTM now!

1st1 · 2016-11-09T15:57:11Z

I've updated the patch with more unittests and more robust checks of socket.type. Will merge it later.

gvanrossum · 2016-11-09T16:45:17Z

asyncio/base_events.py

-    _SOCKET_TYPE_MASK |= socket.SOCK_NONBLOCK
-if hasattr(socket, 'SOCK_CLOEXEC'):
-    _SOCKET_TYPE_MASK |= socket.SOCK_CLOEXEC
+def _is_stream_socket(sock_type):


I would rename this to _is_stream_socket_type(), and similar for the following functions (using _family as the suffix for _is_ip_socket) to emphasize which field of the socket is supposed to be passed into these functions.

Since _is_ip_socket no longer uses those methods, I refactored is_stream_socket to accept socket objects directly, which made the code simpler overall. See the update.

Even better!

More specifically: * loop.create_connection() and loop.create_server() can accept AF_INET or AF_INET6 SOCK_STREAM sockets; * loop.create_datagram_endpoint() can accept only SOCK_DGRAM sockets; * loop.connect_accepted_socket() can accept only SOCK_STREAM sockets; * fixed a bug in create_unix_server() and create_unix_connection() to properly check for SOCK_STREAM sockets on Linux; * fixed static DNS resolution to decline socket types that aren't strictly equal to SOCK_STREAM or SOCK_DGRAM. On Linux socket type can be a bit mask, and we should let system getaddrinfo() to deal with it.

1st1 · 2016-11-09T20:38:03Z

Thanks for the review, Guido. I've implemented the same set of features in uvloop, and will watch closely if they cause any problems (I highly doubt that).

gvanrossum · 2016-11-09T21:02:20Z

Whee!

PR #453.

1st1 · 2016-11-21T22:44:10Z

I rolled back the restriction of passing AF_UNIX to create_connection and create_server. Turns out too many people rely on that, so I guess we can just support that (it's easy to fix uvloop to support this as well).

1st1 mentioned this pull request Nov 6, 2016

aiohttp.worker.GunicornUVLoopWebWorker + unix sockets Nginx - error MagicStack/uvloop#59

Closed

1st1 force-pushed the tcp_strict branch from 5b3b6b6 to dc3b2af Compare November 6, 2016 21:59

gvanrossum reviewed Nov 7, 2016

View reviewed changes

1st1 force-pushed the tcp_strict branch 3 times, most recently from b04c696 to 2abb370 Compare November 7, 2016 19:14

gvanrossum reviewed Nov 7, 2016

View reviewed changes

1st1 force-pushed the tcp_strict branch 3 times, most recently from fb40156 to 2608734 Compare November 9, 2016 15:54

gvanrossum reviewed Nov 9, 2016

View reviewed changes

1st1 force-pushed the tcp_strict branch from 779b6a3 to 00f94fa Compare November 9, 2016 20:32

1st1 force-pushed the tcp_strict branch from 00f94fa to eb4c3ff Compare November 9, 2016 20:34

1st1 merged commit eb4c3ff into python:master Nov 9, 2016

1st1 added a commit that referenced this pull request Nov 21, 2016

Allow AF_UNIX in loop.create_connection and loop.create_server.

ab113e4

PR #453.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make loop TCP APIs accept only TCP streaming sockets #453

Make loop TCP APIs accept only TCP streaming sockets #453

1st1 commented Nov 6, 2016

gvanrossum Nov 7, 2016

1st1 Nov 7, 2016

gvanrossum Nov 7, 2016

1st1 Nov 7, 2016

gvanrossum left a comment

gvanrossum Nov 7, 2016

1st1 Nov 7, 2016

gvanrossum Nov 7, 2016

1st1 Nov 7, 2016

gvanrossum Nov 7, 2016

1st1 Nov 7, 2016

gvanrossum Nov 7, 2016

1st1 Nov 7, 2016

1st1 commented Nov 7, 2016

gvanrossum commented Nov 9, 2016

1st1 commented Nov 9, 2016

gvanrossum Nov 9, 2016

1st1 Nov 9, 2016

gvanrossum Nov 9, 2016

1st1 commented Nov 9, 2016

gvanrossum commented Nov 9, 2016 via email

1st1 commented Nov 21, 2016

Make loop TCP APIs accept only TCP streaming sockets #453

Make loop TCP APIs accept only TCP streaming sockets #453

Conversation

1st1 commented Nov 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gvanrossum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1st1 commented Nov 7, 2016

gvanrossum commented Nov 9, 2016

1st1 commented Nov 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1st1 commented Nov 9, 2016

gvanrossum commented Nov 9, 2016 via email

1st1 commented Nov 21, 2016