classification
Title: asyncio.base_events.create_connection doesn't handle scoped IPv6 addresses
Type: behavior Stage: patch review
Components: asyncio Versions: Python 3.8, Python 3.7
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: asvetlov, eamanu, lepaperwan, maxifree, twisteroid ambassador, yselivanov
Priority: normal Keywords: patch

Created on 2018-12-20 10:56 by maxifree, last changed 2019-01-04 08:20 by twisteroid ambassador.

Pull Requests
URL Status Linked Edit
PR 11271 open lepaperwan, 2018-12-20 23:21
PR 11403 open twisteroid ambassador, 2019-01-02 11:23
PR 11403 open twisteroid ambassador, 2019-01-02 11:23
PR 11403 open twisteroid ambassador, 2019-01-02 11:23
PR 11403 open twisteroid ambassador, 2019-01-02 11:23
Messages (8)
msg332211 - (view) Author: (maxifree) Date: 2018-12-20 10:56
loop.create_connection doesn't handle ipv6 RFC4007 addresses right since 3.7 


TEST CASE
# Set up listener on link-local address fe80::1%lo
sudo ip a add dev lo fe80::1

# 3.6 handles everything fine
socat file:/dev/null tcp6-listen:12345,REUSEADDR &
python3.6 -c 'import asyncio;loop=asyncio.get_event_loop();loop.run_until_complete(loop.create_connection(lambda:asyncio.Protocol(),host="fe80::1%lo",port="12345"))'

# 3.7 and later fails
socat file:/dev/null tcp6-listen:12345,REUSEADDR &
python3.7 -c 'import asyncio;loop=asyncio.get_event_loop();loop.run_until_complete(loop.create_connection(lambda:asyncio.Protocol(),host="fe80::1%lo",port="12345"))'

Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/usr/lib/python3.7/asyncio/base_events.py", line 576, in run_until_complete
    return future.result()
  File "/usr/lib/python3.7/asyncio/base_events.py", line 951, in create_connection
    raise exceptions[0]
  File "/usr/lib/python3.7/asyncio/base_events.py", line 938, in create_connection
    await self.sock_connect(sock, address)
  File "/usr/lib/python3.7/asyncio/selector_events.py", line 475, in sock_connect
    return await fut
  File "/usr/lib/python3.7/asyncio/selector_events.py", line 480, in _sock_connect
    sock.connect(address)
OSError: [Errno 22] Invalid argument


CAUSE

Upon asyncio.base_events.create_connection _ensure_resolved is called twice, first time here:
https://github.com/python/cpython/blob/3.7/Lib/asyncio/base_events.py#L908
then here through sock_connect:
https://github.com/python/cpython/blob/3.7/Lib/asyncio/base_events.py#L946
https://github.com/python/cpython/blob/3.7/Lib/asyncio/selector_events.py#L458

_ensure_resolved calls getaddrinfo, but in 3.7 implementation changed:

% python3.6 -c 'import socket;print(socket.getaddrinfo("fe80::1%lo",12345)[0][4])'
('fe80::1%lo', 12345, 0, 1)

% python3.7 -c 'import socket;print(socket.getaddrinfo("fe80::1%lo",12345)[0][4])'
('fe80::1', 12345, 0, 1)

_ensure_connect only considers host and port parts of the address tuple:
https://github.com/python/cpython/blob/3.7/Lib/asyncio/base_events.py#L1272

In case of 3.7 first call to _ensure_resolved returns
('fe80::1', 12345, 0, 1)
then second call returns
('fe80::1', 12345, 0, 0)
Notice that scope is now completely lost and is set to 0, thus actual call to socket.connect is wrong

In case of 3.6 both first and second call to _ensure_resolved return
('fe80::1%lo', 12345, 0, 1)
because in 3.6 case scope info is preserved in address and second call can derive correct address tuple
msg332275 - (view) Author: Erwan Le Pape (lepaperwan) * Date: 2018-12-20 23:08
While the 3.7+ getaddrinfo isn't the best human representation of an IPv6 address, I believe it does make the most sense to keep it that way.
In any case, this is a regression and changing return values of getaddrinfo for 3.7 isn't something that should be considered.

The issue stems from the refactoring of the underlying socketmodule.c handling of IPv4/IPv6 addresses with dedicated make_ipv4_addr and make_ipv6_addr functions which returns proper tuples of:
  str/int for IPv4: https://github.com/python/cpython/blob/master/Modules/socketmodule.c#L1270
  str/int/int/int for IPv6: https://github.com/python/cpython/blob/master/Modules/socketmodule.c#L1325

The actual issue is that _ensure_resolved naively assumes IPv4 and truncates the address to its first 2 members
https://github.com/python/cpython/blob/3.7/Lib/asyncio/base_events.py#L1269
and never redefines them again in the case where they were set.

I'd suggest passing the remaining elements of address in a packed *args or an optional flowinfo=0, scopeid=0 to _ipaddr_info since fundamentally, that's the method stripping valuable information.
msg332287 - (view) Author: twisteroid ambassador (twisteroid ambassador) * Date: 2018-12-21 11:04
I think the root cause of this bug is a bit of confusion.

The "customer-facing" asyncio API, create_connection(), takes two arguments: host and port. The lower-level API that actually deal with connecting sockets, socket.connect() and loop.sock_connect(), takes one argument: the address tuple. These are *not the same thing*, despite an IPv4 address tuple having two elements (host, port), and must not be mixed.

_ensure_resolved() is the function responsible for turning host + port into an address tuple, and it does the right thing, turning host="fe80::1%lo",port=12345 into ('fe80::1', 12345, 0, 1) correctly. The mistake is taking the address tuple and passing it through _ensure_resolved() again, since that's not the correct input type for it: the only correct input type is host + port.

So I think the appropriate fix for this bug is to make sure _ensure_resolved is only called once. In particular, BaseSelectorEventLoop.sock_connect() https://github.com/python/cpython/blob/3bc0ebab17bf5a2c29d2214743c82034f82e6573/Lib/asyncio/selector_events.py#L458 should not call _ensure_resolved(). It might be a good idea to add some comments clarifying that sock_connect() takes an address tuple argument, not host + port, and likewise for sock_connect() on each event loop implementation.
msg332288 - (view) Author: twisteroid ambassador (twisteroid ambassador) * Date: 2018-12-21 11:10
Also I believe it's a good idea to change the arguments of _ensure_resolved() from (address, *, ...) to (host, port, *, ...), and go through all its usages, making sure we're not mixing host + port with address tuples everywhere in asyncio.
msg332871 - (view) Author: Emmanuel Arias (eamanu) * Date: 2019-01-02 12:48
Hi!, 

I was reading the PR.  Just a little comment. I am not sure about have a difference for IPv4 and IPv6, in the sense of use a tuple for IPv4 and separate parameters for IPv6

Regards
msg332962 - (view) Author: twisteroid ambassador (twisteroid ambassador) * Date: 2019-01-04 07:17
Hi Emmanuel,

Are you referring to my PR 11403? I don't see where IPv6 uses separate parameters.
msg332963 - (view) Author: twisteroid ambassador (twisteroid ambassador) * Date: 2019-01-04 08:01
I just noticed that in the socket module, an AF_INET address tuple is allowed to have an unresolved host name. Quote:

A pair (host, port) is used for the AF_INET address family, where host is a string representing either a hostname in Internet domain notation like 'daring.cwi.nl' or an IPv4 address like '100.50.200.5', and port is an integer.

https://docs.python.org/3/library/socket.html#socket-families

Passing a tuple of (hostname, port) to socket.connect() successfully connects the socket (tested on Windows). Since the C struct sockaddr_in does not support hostnames, socket.connect obviously does resolution at some point, but its implementation is in C, and I haven't looked into it.

BaseSelectorEventLoop.sock_connect() calls socket.connect() directly, therefore it also supports passing in a tuple of (hostname, port). I just tested ProactorEventLoop.sock_connect() on 3.7.1 on Windows, and it does not support hostnames, raising OSError: [WinError 10022] An invalid argument was supplied.

I personally believe it's not a good idea to allow hostnames in address tuples and in sock.connect(). However, the socket module tries pretty hard to basically accept any (host, port) tuple as address tuples, whether host is an IPv4 address, IPv6 address or host name, so that's probably not going to change.
msg332964 - (view) Author: twisteroid ambassador (twisteroid ambassador) * Date: 2019-01-04 08:20
Oh wait, there's also this in asyncio docs for loop.sock_connect:

Changed in version 3.5.2: address no longer needs to be resolved. sock_connect will try to check if the address is already resolved by calling socket.inet_pton(). If not, loop.getaddrinfo() will be used to resolve the address.

https://docs.python.org/3/library/asyncio-eventloop.html#asyncio.loop.sock_connect

So this is where the current bug comes from! My PR 11403 basically undid this change.

My proposal, as is probably obvious, is to undo this change and insist on passing only resolved address tuples to loop.sock_connect(). My argument is that this feature never worked properly: 

* As mentioned in the previous message, this does not work on ProactorEventLoop.
* On SelectorEventLoop, the resolution done by loop.sock_connect() is pretty weak anyways: it only takes the first resolved address, unlike loop.create_connection() that actually tries all the resolved addresses until one of them successfully connects.

Users should use create_connection() or open_connection() if they want to avoid the complexities of address resolution. If they are reaching for low_level APIs like loop.sock_connect(), they should also handle loop.getaddrinfo() themselves.
History
Date User Action Args
2019-01-04 08:20:32twisteroid ambassadorsetmessages: + msg332964
2019-01-04 08:01:32twisteroid ambassadorsetmessages: + msg332963
2019-01-04 07:17:52twisteroid ambassadorsetmessages: + msg332962
2019-01-02 12:48:35eamanusetnosy: + eamanu
messages: + msg332871
2019-01-02 11:23:49twisteroid ambassadorsetpull_requests: + pull_request10789
2019-01-02 11:23:40twisteroid ambassadorsetpull_requests: + pull_request10788
2019-01-02 11:23:30twisteroid ambassadorsetpull_requests: + pull_request10787
2019-01-02 11:23:21twisteroid ambassadorsetpull_requests: + pull_request10786
2018-12-21 11:10:28twisteroid ambassadorsetmessages: + msg332288
2018-12-21 11:04:46twisteroid ambassadorsetnosy: + twisteroid ambassador
messages: + msg332287
2018-12-20 23:21:45lepaperwansetkeywords: + patch
stage: patch review
pull_requests: + pull_request10507
2018-12-20 23:08:59lepaperwansetnosy: + lepaperwan
messages: + msg332275
2018-12-20 10:56:50maxifreecreate