MagicStack / uvloop

Ultra fast asyncio event loop.
Apache License 2.0
10.27k stars 538 forks source link

Silently closes UDP socket #338

Open spumer opened 4 years ago

spumer commented 4 years ago

uvloop silently closes UDP socket when sending data to incorrect destination.

Test program:

import asyncio
import uvloop

# uncomment this line to check it with uvloop
#uvloop.install()

async def main():
    loop = asyncio.get_event_loop()
    transport, proto = await loop.create_datagram_endpoint(
        asyncio.DatagramProtocol,
        local_addr=("192.168.0.2", 0),
    )
    print(transport.get_extra_info('sockname'))
    print("Before sending the message with None destination")
    transport.sendto(b"deadbeef")
    print("After sending the message with None destination")

    await asyncio.sleep(0.1)  # let asyncio closes internal socket

    print("Before sending the message with incorrect port destination")
    transport.sendto(b"deadbeef", ("45.83.128.251", 0))
    print("After sending the message with incorrect port destination")
    transport.close()

asyncio.get_event_loop().run_until_complete(main())

asyncio output:

'192.168.0.2', 59471) Before sending the message with None destination Fatal write error on datagram transport protocol: transport: <_SelectorDatagramTransport fd=6 read=polling write=> Traceback (most recent call last): File "/root/.pyenv/versions/3.8.2/lib/python3.8/asyncio/selector_events.py", line 1046, in sendto self._sock.sendto(data, addr) TypeError: sendto(): AF_INET address must be tuple, not NoneType After sending the message with None destination Before sending the message with incorrect port destination Traceback (most recent call last): File "/root/.pyenv/versions/3.8.2/lib/python3.8/asyncio/selector_events.py", line 1046, in sendto self._sock.sendto(data, addr) AttributeError: 'NoneType' object has no attribute 'sendto' During handling of the above exception, another exception occurred: Traceback (most recent call last): File "", line 1, in File "/root/.pyenv/versions/3.8.2/lib/python3.8/asyncio/base_events.py", line 616, in run_until_complete return future.result() File "", line 14, in main File "/root/.pyenv/versions/3.8.2/lib/python3.8/asyncio/selector_events.py", line 1056, in sendto self._fatal_error( File "/root/.pyenv/versions/3.8.2/lib/python3.8/asyncio/selector_events.py", line 703, in _fatal_error self._loop.call_exception_handler({ AttributeError: 'NoneType' object has no attribute 'call_exception_handler'

uvloop output:

('192.168.0.2', 33515) Before sending the message with None destination After sending the message with None destination Before sending the message with incorrect port destination After sending the message with incorrect port destination

Then we can cut off first block with sending to None destination. And here we have NEW ONE differece in handling ip:port.

async def main():
    loop = asyncio.get_event_loop()
    transport, proto = await loop.create_datagram_endpoint(
        asyncio.DatagramProtocol,
        local_addr=("192.168.0.2", 0),
    )
    print(transport.get_extra_info('sockname'))
    print("Please use `netstat -au` to see this socket really listening. I will wait 10 sec")
    await asyncio.sleep(10)

    print("Before sending the message with incorrect port destination")
    transport.sendto(b"deadbeef", ("45.83.128.251", 0))
    print("After sending the message with incorrect port destination")

    print("Please use `netstat -au` to see this socket STILL really listening. I will wait 10 sec")
    await asyncio.sleep(10)

    transport.close()

When i run it with asyncio event loop all looks fine, socket still present.

When i run it with uvloop it silently closes, and also i can go into infinity wait on recv() call


As i know RFC describe that case as (https://tools.ietf.org/html/rfc8085#section-5.1):

A UDP sender SHOULD NOT use a source port value of zero.  A source
   port number that cannot be easily determined from the address or
   payload type provides protection at the receiver from data injection
   attacks by off-path devices.  A UDP receiver SHOULD NOT bind to port
   zero.

But i got real messages from the internet with that port. I will drop it in my application, but i think uvloop should not silently closes socket. It's very unexpected and differ from asyncio event loop.

Proof from Sentry (https://github.com/spumer/source-query-proxy):

next iteration of recv packet was failed, cause in previous we send response to zero port изображение

fantix commented 3 years ago

You're actually talking about multiple issues.

  1. UDPTransport should raise errors if used after connection_lost() is called.
  2. UDPTransport.sendto(DATA, (IP, 0)) would silently close the transport.
  3. I'm not sure about your quote of the RFC or your Sentry report, but I guess you were saying we shouldn't allow local_addr=("192.168.0.2", 0)?

My answers:

  1. You should not use the transport after connection_lost() is called. However uvloop can be more user-friendly to raise errors if UDPTransport is used after connection_lost() as asyncio does (even though asyncio's error is not friendly enough). On the other hand, it's also worth checking why connection_lost() is triggered and if that's necessary.
  2. On Linux, the transport is NOT silently closed - connection_lost() is called, you should listen to that event. However, asyncio won't close the transport on Linux - this is something to double-check. Not reproduced on macOS.
  3. Python allows you to bind a socket to a random free port, like socket.socket(socket.AF_INET, socket.SOCK_DGRAM).bind(('127.0.0.1', 0)).
spumer commented 3 years ago

I'm not sure about your quote of the RFC or your Sentry report, but I guess you were saying we shouldn't allow local_addr=("192.168.0.2", 0)?

No, RFC references for UDPTransport.sendto(DATA, (IP, 0)) would silently close the transport. Sentry screenshot just proof for "i'm really got request with zero src port"

r00ta commented 1 year ago

my2c

A UDP sender SHOULD NOT use a source port value of zero.  
A UDP receiver SHOULD NOT bind to zero.

suggests that who writes the software should not bind that port. Such framework should not prevent users from shooting themselves in the foot