Commit 061cd5d8 authored by Kirill Smelkov's avatar Kirill Smelkov Committed by Julien Muchembled

ssl: Don't ignore non-ragged EOF

Testing NEO/go client wrt NEO/py server revealed a bug in NEO/py SSL
handling: proper non-ragged EOF from a peer is ignored, and so leads to
hang in infinite loop inside _SSL.receive with read_buf memory growing
indefinitely. Details are below:

NEO/py wraps raw sockets with

	ssl.wrap_socket(suppress_ragged_eofs=False)

which instructs SSL layer to convert unexpected EOF when receiving a TLS
record into SSLEOFError exception. However when remote peer properly
closes its side of the connection, socket.read() still returns b'' to
report non-ragged regular EOF:

https://github.com/python/cpython/blob/v2.7.18/Lib/ssl.py#L630-L650

The code was handling SSLEOFError but not b'' return from socket recv.
Thus after NEO/go client was disconnecting and properly closing its side
of the connection, the code started to loop indefinitely in _SSL.receive
under `while 1` with  b'' returned by self.socket.recv() appended to
read_buf again and again.

-> Fix it by detecting non-ragged EOF as well and, similarly to how
SSLEOFError is handled, converting them into self._error('recv', None).

See merge request nexedi/neoppod!17
parent 261dd4b4
...@@ -277,7 +277,12 @@ class _SSL: ...@@ -277,7 +277,12 @@ class _SSL:
def receive(self, read_buf): def receive(self, read_buf):
try: try:
while 1: while 1:
read_buf.feed(self.socket.recv(4096)) data = self.socket.recv(4096)
if not data:
# non-ragged EOF (peer properly closed its side of connection)
self._error('recv', None)
return
read_buf.feed(data)
except ssl.SSLWantReadError: except ssl.SSLWantReadError:
pass pass
except socket.error, e: except socket.error, e:
......
...@@ -42,7 +42,7 @@ class SSLTests(SSLMixin, test.Test): ...@@ -42,7 +42,7 @@ class SSLTests(SSLMixin, test.Test):
def testBasicStore(self): def testBasicStore(self):
super(SSLTests, self).testBasicStore(True) super(SSLTests, self).testBasicStore(True)
def testAbortConnection(self, after_handshake=1): def testAbortConnection(self, after_handshake=1, abortf=None):
with self.getLoopbackConnection() as conn: with self.getLoopbackConnection() as conn:
conn.ask(Packets.Ping()) conn.ask(Packets.Ping())
connector = conn.getConnector() connector = conn.getConnector()
...@@ -54,7 +54,9 @@ class SSLTests(SSLMixin, test.Test): ...@@ -54,7 +54,9 @@ class SSLTests(SSLMixin, test.Test):
if after_handshake: if after_handshake:
while not isinstance(connector, connector.SSLConnectorClass): while not isinstance(connector, connector.SSLConnectorClass):
conn.em.poll(1) conn.em.poll(1)
conn.abort() if abortf is None:
abortf = conn.__class__.abort
abortf(conn)
fd = connector.getDescriptor() fd = connector.getDescriptor()
while fd in conn.em.reader_set: while fd in conn.em.reader_set:
conn.em.poll(1) conn.em.poll(1)
...@@ -63,6 +65,15 @@ class SSLTests(SSLMixin, test.Test): ...@@ -63,6 +65,15 @@ class SSLTests(SSLMixin, test.Test):
def testAbortConnectionBeforeHandshake(self): def testAbortConnectionBeforeHandshake(self):
self.testAbortConnection(0) self.testAbortConnection(0)
def testReceiveVSEOF(self):
# verify that _SSL.receive correctly handles non-ragged EOF.
# ( it used to hang in infinite busy loop because _SSL was not checking
# return from socket.recv for b'' )
def _(conn):
conn.em.poll(0)
conn.connector.shutdown()
self.testAbortConnection(abortf=_)
def testSSLVsNoSSL(self): def testSSLVsNoSSL(self):
def __init__(orig, self, app, *args, **kw): def __init__(orig, self, app, *args, **kw):
with Patch(app, ssl=None): with Patch(app, ssl=None):
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment