ssl: Fix recv_into blocking when reading chunks of datassl-recv-methods

The comment in the test should explain the issue sufficiently.

Turns out that request/urllib3 use recv_into() instad of recv() on
Python 3 so:

Closes https://github.com/eventlet/eventlet/issues/315.

This patch is contributed by Smarkets Limited.

2 files changed, 88 insertions, 10 deletions

diff --git a/eventlet/green/ssl.py b/eventlet/green/ssl.py
index 828f9bf..2e0fef6 100644
--- a/eventlet/green/ssl.py
+++ b/eventlet/green/ssl.py
@@ -191,18 +191,39 @@ class GreenSSLSocket(_original_sslsocket):
                     raise
 
     def recv(self, buflen=1024, flags=0):
+        return self._base_recv(buflen, flags, into=False)
+
+    def recv_into(self, buffer, nbytes=None, flags=0):
+        # Copied verbatim from CPython
+        if buffer and nbytes is None:
+            nbytes = len(buffer)
+        elif nbytes is None:
+            nbytes = 1024
+        # end of CPython code
+
+        return self._base_recv(nbytes, flags, into=True, buffer_=buffer)
+
+    def _base_recv(self, nbytes, flags, into, buffer_=None):
+        if into:
+            plain_socket_function = socket.recv_into
+        else:
+            plain_socket_function = socket.recv
+
         # *NOTE: gross, copied code from ssl.py becase it's not factored well enough to be used as-is
         if self._sslobj:
             if flags != 0:
                 raise ValueError(
-                    "non-zero flags not allowed in calls to recv() on %s" %
-                    self.__class__)
-            read = self.read(buflen)
+                    "non-zero flags not allowed in calls to %s() on %s" %
+                    plain_socket_function.__name__, self.__class__)
+            read = self.read(nbytes, buffer_)
             return read
         else:
             while True:
                 try:
-                    return socket.recv(self, buflen, flags)
+                    args = [self, nbytes, flags]
+                    if into:
+                        args.insert(1, buffer_)
+                    return plain_socket_function(*args)
                 except orig_socket.error as e:
                     if self.act_non_blocking:
                         raise
@@ -218,12 +239,6 @@ class GreenSSLSocket(_original_sslsocket):
                         return b''
                     raise
 
-    def recv_into(self, buffer, nbytes=None, flags=0):
-        if not self.act_non_blocking:
-            trampoline(self, read=True, timeout=self.gettimeout(),
-                       timeout_exc=timeout_exc('timed out'))
-        return super(GreenSSLSocket, self).recv_into(buffer, nbytes, flags)
-
     def recvfrom(self, addr, buflen=1024, flags=0):
         if not self.act_non_blocking:
             trampoline(self, read=True, timeout=self.gettimeout(),
diff --git a/tests/ssl_test.py b/tests/ssl_test.py
index 193d9df..8fc784f 100644
--- a/tests/ssl_test.py
+++ b/tests/ssl_test.py
@@ -1,12 +1,15 @@
+import contextlib
 import socket
 import warnings
 
 import eventlet
 from eventlet import greenio
+from eventlet.green import socket
 try:
     from eventlet.green import ssl
 except ImportError:
     __test__ = False
+from eventlet.support import six
 import tests
 
 
@@ -205,3 +208,63 @@ class SSLTest(tests.LimitedTestCase):
         listener.close()
         loopt.wait()
         eventlet.sleep(0)
+
+    def test_receiving_doesnt_block_if_there_is_already_decrypted_buffered_data(self):
+        # Here's what could (and would) happen before the relevant bug was fixed (assuming method
+        # M was trampolining unconditionally before actually reading):
+        # 1. One side sends n bytes, leaves connection open (important)
+        # 2. The other side uses method M to read m (where m < n) bytes, the underlying SSL
+        #    implementation reads everything from the underlying socket, decrypts all n bytes,
+        #    returns m of them and buffers n-m to be read later.
+        # 3. The other side tries to read the remainder of the data (n-m bytes), this blocks
+        #    because M trampolines uncoditionally and trampoline will hang because reading from
+        #    the underlying socket would block. It would block because there's no data to be read
+        #    and the connection is still open; leaving the connection open /mentioned in 1./ is
+        #    important because otherwise trampoline would return immediately and the test would pass
+        #    even with the bug still present in the code).
+        #
+        # The solution is to first request data from the underlying SSL implementation and only
+        # trampoline if we actually need to read some data from the underlying socket.
+        #
+        # GreenSSLSocket.recv() wasn't broken but I've added code to test it as well for
+        # completeness.
+        content = b'xy'
+
+        def recv(sock, expected):
+            assert sock.recv(len(expected)) == expected
+
+        def recv_into(sock, expected):
+            buf = bytearray(len(expected))
+            assert sock.recv_into(buf, len(expected)) == len(expected)
+            assert buf == expected
+
+        for read_function in [recv, recv_into]:
+            print('Trying %s...' % (read_function,))
+            listener = listen_ssl_socket()
+
+            def accept(listener):
+                sock, addr = listener.accept()
+                sock.sendall(content)
+                return sock
+
+            accepter = eventlet.spawn(accept, listener)
+
+            client_to_server = None
+            try:
+                client_to_server = ssl.wrap_socket(eventlet.connect(listener.getsockname()))
+                for character in six.iterbytes(content):
+                    character = six.int2byte(character)
+                    print('We have %d already decrypted bytes pending, expecting: %s' % (
+                        client_to_server.pending(), character))
+                    read_function(client_to_server, character)
+            finally:
+                if client_to_server is not None:
+                    client_to_server.close()
+                server_to_client = accepter.wait()
+
+                # Very important: we only want to close the socket *after* the other side has
+                # read the data it wanted already, otherwise this would defeat the purpose of the
+                # test (see the comment at the top of this test).
+                server_to_client.close()
+
+                listener.close()