Repository: aio-libs/aiosmtpd

Issue:
CancelledError and misbehaving clients handling
I tried to write a RCPT handler that accepts connection from various embedded devices and forwards the email address to an external system. However, as the devices are not proper SMTP clients and their quality varies I need the solution to be pretty robust. While developing it I discovered a problem with the way aoismtpd deals with clients who send data and ignore the response. To reproduce:

sleep.py (simple handler class simulating my handler):

```
import asyncio
from aiosmtpd.handlers import Debugging

class RcptWebhook(Debugging):
    async def handle_RCPT(self, server, session, envelope, address, rcpt_options):
        await asyncio.sleep(10)

        envelope.rcpt_tos.append(address)
        return '250 OK'
```

Run aiosmtpd

    python3 -m aiosmtpd -n -c sleep.RcptWebhook

smtp.txt (sample SMTP session)

```
HELO there
MAIL FROM: somebody@example.com
RCPT TO: anybody@example.com
DATA
test
.
```
(note the is no QUIT in the data)

Send it witch netcat

    nc localhost 8025 < smtp.txt

It sends the content of the file and EOF which causes unhandled CanceledError in aiosmtpd while it is waiting for my sleep handler to finish:
```

INFO:mail.log:Server listening on localhost:8025
INFO:mail.log:Starting asyncio loop
INFO:mail.log:Peer: ('127.0.0.1', 39306)
INFO:mail.log:('127.0.0.1', 39306) handling connection
INFO:mail.log:('127.0.0.1', 39306) Data: b'HELO there'
INFO:mail.log:('127.0.0.1', 39306) Data: b'MAIL FROM: somebody@example.com'
INFO:mail.log:('127.0.0.1', 39306) sender: somebody@example.com
INFO:mail.log:('127.0.0.1', 39306) Data: b'RCPT TO: anybody@example.com'
INFO:mail.log:('127.0.0.1', 39306) EOF received
ERROR:mail.log:SMTP session exception
Traceback (most recent call last):
  File "/usr/local/lib/python3.5/dist-packages/aiosmtpd/smtp.py", line 276, in _handle_client
    yield from method(arg)
  File "/usr/local/lib/python3.5/dist-packages/aiosmtpd/smtp.py", line 582, in smtp_RCPT
    'RCPT', address, rcpt_options)
  File "/usr/local/lib/python3.5/dist-packages/aiosmtpd/smtp.py", line 116, in _call_handler_hook
    status = yield from hook(self, self.session, self.envelope, *args)
  File "/home/vaclav/src/smtp-events-receiver/sleep.py", line 6, in handle_RCPT
    await asyncio.sleep(10)
  File "/usr/lib/python3.5/asyncio/tasks.py", line 516, in sleep
    return (yield from future)
  File "/usr/lib/python3.5/asyncio/futures.py", line 361, in __iter__
    yield self  # This tells Task to wait for completion.
  File "/usr/lib/python3.5/asyncio/tasks.py", line 296, in _wakeup
    future.result()
  File "/usr/lib/python3.5/asyncio/futures.py", line 266, in result
    raise CancelledError
concurrent.futures._base.CancelledError
INFO:mail.log:('127.0.0.1', 39306) Data: b'DATA'
INFO:mail.log:('127.0.0.1', 39306) Data: b'test'
INFO:mail.log:('127.0.0.1', 39306) Data: b'.'
INFO:mail.log:('127.0.0.1', 39306) Data: b''
INFO:mail.log:('127.0.0.1', 39306) Data: b''
INFO:mail.log:('127.0.0.1', 39306) Data: b''
INFO:mail.log:('127.0.0.1', 39306) Data: b''
...
```
aiosmtpd is now 100% busy on CPU and keeps printing "500 Error: bad syntax" to the netcat. It ignores KeyboardInterrupt and to quit it I need to terminate netcat with ^C which stops aiosmtp with "Connection lost".

I think aiosmtpd should handle this more gracefully as you don't want your server to get stuck on CPU due to misbehaving client. I see in RFC 5321 that the server must not close the connection until QUIT from the client so I'm not sure what is the proper way of handling EOF though.

Discussion:

Patch:
diff --git a/MANIFEST.in b/MANIFEST.in
index 1b19070..6c44541 100644
--- a/MANIFEST.in
+++ b/MANIFEST.in
@@ -1,4 +1,4 @@
 include *.py MANIFEST.in
 global-include *.txt *.rst *.ini *.yml *.cfg *.crt *.key
-exclude .gitignore
+global-exclude .gitignore
 prune build
diff --git a/aiosmtpd/smtp.py b/aiosmtpd/smtp.py
index b790785..dfdbcb5 100644
--- a/aiosmtpd/smtp.py
+++ b/aiosmtpd/smtp.py
@@ -201,12 +201,6 @@ class SMTP(asyncio.StreamReaderProtocol):
             try:
                 line = await self._reader.readline()
                 log.debug('_handle_client readline: %s', line)
-            except (ConnectionResetError, asyncio.CancelledError) as error:
-                # The connection got reset during the DATA command.
-                log.info('Connection lost during _handle_client()')
-                self.connection_lost(error)
-                return
-            try:
                 # XXX this rstrip may not completely preserve old behavior.
                 line = line.rstrip(b'\r\n')
                 log.info('%r Data: %s', self.session.peer, line)
@@ -266,6 +260,13 @@ class SMTP(asyncio.StreamReaderProtocol):
                         '500 Error: command "%s" not recognized' % command)
                     continue
                 await method(arg)
+            except (ConnectionResetError, asyncio.CancelledError) as error:
+                # The connection got reset during the DATA command.
+                # XXX If handler method raises ConnectionResetError, we should
+                # verify that it was actually self._reader that was reset.
+                log.info('Connection lost during _handle_client()')
+                self.connection_lost(error)
+                return
             except Exception as error:
                 try:
                     status = await self.handle_exception(error)


Test patch:
diff --git a/aiosmtpd/tests/test_smtp.py b/aiosmtpd/tests/test_smtp.py
index 8a7ba7d..ab52cf6 100644
--- a/aiosmtpd/tests/test_smtp.py
+++ b/aiosmtpd/tests/test_smtp.py
@@ -124,6 +124,13 @@ class ErrorController(Controller):
         return ErrorSMTP(self.handler)
 
 
+class SleepingHeloHandler:
+    async def handle_HELO(self, server, session, envelope, hostname):
+        await asyncio.sleep(0.01)
+        session.host_name = hostname
+        return '250 {}'.format(server.hostname)
+
+
 class TestProtocol(unittest.TestCase):
     def setUp(self):
         self.transport = Mock()
@@ -1135,3 +1142,17 @@ Testing\xFF
 """)
             self.assertEqual(cm.exception.smtp_code, 500)
             self.assertIn(b'Error: strict ASCII mode', cm.exception.smtp_error)
+
+
+class TestSleepingHandler(unittest.TestCase):
+    def setUp(self):
+        controller = NoDecodeController(SleepingHeloHandler())
+        controller.start()
+        self.addCleanup(controller.stop)
+        self.address = (controller.hostname, controller.port)
+
+    def test_close_after_helo(self):
+        with SMTP(*self.address) as client:
+            client.send('HELO example.com\r\n')
+            client.sock.shutdown(socket.SHUT_WR)
+            self.assertRaises(SMTPServerDisconnected, client.getreply)