From 3ffe35af98b81cb44c8fcd699dc87690ce7238f8 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 25 Feb 2021 09:57:05 -0500 Subject: [PATCH 1/4] Update tests to not use setUp. --- tests/http/test_client.py | 74 +++++++++++++++++++++------------------ 1 file changed, 40 insertions(+), 34 deletions(-) diff --git a/tests/http/test_client.py b/tests/http/test_client.py index 2d9b733be0ff..1772347ddf94 100644 --- a/tests/http/test_client.py +++ b/tests/http/test_client.py @@ -26,77 +26,83 @@ class ReadBodyWithMaxSizeTests(TestCase): - def setUp(self): + def _build_response(self, length=UNKNOWN_LENGTH): """Start reading the body, returns the response, result and proto""" - response = Mock(length=UNKNOWN_LENGTH) - self.result = BytesIO() - self.deferred = read_body_with_max_size(response, self.result, 6) + response = Mock(length=length) + result = BytesIO() + deferred = read_body_with_max_size(response, result, 6) # Fish the protocol out of the response. - self.protocol = response.deliverBody.call_args[0][0] - self.protocol.transport = Mock() + protocol = response.deliverBody.call_args[0][0] + protocol.transport = Mock() - def _cleanup_error(self): + return result, deferred, protocol + + def _cleanup_error(self, deferred): """Ensure that the error in the Deferred is handled gracefully.""" called = [False] def errback(f): called[0] = True - self.deferred.addErrback(errback) + deferred.addErrback(errback) self.assertTrue(called[0]) def test_no_error(self): """A response that is NOT too large.""" + result, deferred, protocol = self._build_response() # Start sending data. - self.protocol.dataReceived(b"12345") + protocol.dataReceived(b"12345") # Close the connection. - self.protocol.connectionLost(Failure(ResponseDone())) + protocol.connectionLost(Failure(ResponseDone())) - self.assertEqual(self.result.getvalue(), b"12345") - self.assertEqual(self.deferred.result, 5) + self.assertEqual(result.getvalue(), b"12345") + self.assertEqual(deferred.result, 5) def test_too_large(self): """A response which is too large raises an exception.""" + result, deferred, protocol = self._build_response() # Start sending data. - self.protocol.dataReceived(b"1234567890") + protocol.dataReceived(b"1234567890") # Close the connection. - self.protocol.connectionLost(Failure(ResponseDone())) + protocol.connectionLost(Failure(ResponseDone())) - self.assertEqual(self.result.getvalue(), b"1234567890") - self.assertIsInstance(self.deferred.result, Failure) - self.assertIsInstance(self.deferred.result.value, BodyExceededMaxSize) - self._cleanup_error() + self.assertEqual(result.getvalue(), b"1234567890") + self.assertIsInstance(deferred.result, Failure) + self.assertIsInstance(deferred.result.value, BodyExceededMaxSize) + self._cleanup_error(deferred) def test_multiple_packets(self): - """Data should be accummulated through mutliple packets.""" + """Data should be accumulated through mutliple packets.""" + result, deferred, protocol = self._build_response() # Start sending data. - self.protocol.dataReceived(b"12") - self.protocol.dataReceived(b"34") + protocol.dataReceived(b"12") + protocol.dataReceived(b"34") # Close the connection. - self.protocol.connectionLost(Failure(ResponseDone())) + protocol.connectionLost(Failure(ResponseDone())) - self.assertEqual(self.result.getvalue(), b"1234") - self.assertEqual(self.deferred.result, 4) + self.assertEqual(result.getvalue(), b"1234") + self.assertEqual(deferred.result, 4) def test_additional_data(self): """A connection can receive data after being closed.""" + result, deferred, protocol = self._build_response() # Start sending data. - self.protocol.dataReceived(b"1234567890") - self.assertIsInstance(self.deferred.result, Failure) - self.assertIsInstance(self.deferred.result.value, BodyExceededMaxSize) - self.protocol.transport.abortConnection.assert_called_once() + protocol.dataReceived(b"1234567890") + self.assertIsInstance(deferred.result, Failure) + self.assertIsInstance(deferred.result.value, BodyExceededMaxSize) + protocol.transport.abortConnection.assert_called_once() # More data might have come in. - self.protocol.dataReceived(b"1234567890") + protocol.dataReceived(b"1234567890") # Close the connection. - self.protocol.connectionLost(Failure(ResponseDone())) + protocol.connectionLost(Failure(ResponseDone())) - self.assertEqual(self.result.getvalue(), b"1234567890") - self.assertIsInstance(self.deferred.result, Failure) - self.assertIsInstance(self.deferred.result.value, BodyExceededMaxSize) - self._cleanup_error() + self.assertEqual(result.getvalue(), b"1234567890") + self.assertIsInstance(deferred.result, Failure) + self.assertIsInstance(deferred.result.value, BodyExceededMaxSize) + self._cleanup_error(deferred) From 5f22de366aa591bbac7cc46c0240a8a599bb6a1b Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 25 Feb 2021 10:13:52 -0500 Subject: [PATCH 2/4] Abstract repeated code in tests. --- tests/http/test_client.py | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/tests/http/test_client.py b/tests/http/test_client.py index 1772347ddf94..ffff09819cdc 100644 --- a/tests/http/test_client.py +++ b/tests/http/test_client.py @@ -38,6 +38,12 @@ def _build_response(self, length=UNKNOWN_LENGTH): return result, deferred, protocol + def _assert_error(self, deferred, protocol): + """Ensure that the expected error is received.""" + self.assertIsInstance(deferred.result, Failure) + self.assertIsInstance(deferred.result.value, BodyExceededMaxSize) + protocol.transport.abortConnection.assert_called_once() + def _cleanup_error(self, deferred): """Ensure that the error in the Deferred is handled gracefully.""" called = [False] @@ -66,12 +72,9 @@ def test_too_large(self): # Start sending data. protocol.dataReceived(b"1234567890") - # Close the connection. - protocol.connectionLost(Failure(ResponseDone())) self.assertEqual(result.getvalue(), b"1234567890") - self.assertIsInstance(deferred.result, Failure) - self.assertIsInstance(deferred.result.value, BodyExceededMaxSize) + self._assert_error(deferred, protocol) self._cleanup_error(deferred) def test_multiple_packets(self): @@ -93,16 +96,11 @@ def test_additional_data(self): # Start sending data. protocol.dataReceived(b"1234567890") - self.assertIsInstance(deferred.result, Failure) - self.assertIsInstance(deferred.result.value, BodyExceededMaxSize) - protocol.transport.abortConnection.assert_called_once() + self._assert_error(deferred, protocol) # More data might have come in. protocol.dataReceived(b"1234567890") - # Close the connection. - protocol.connectionLost(Failure(ResponseDone())) self.assertEqual(result.getvalue(), b"1234567890") - self.assertIsInstance(deferred.result, Failure) - self.assertIsInstance(deferred.result.value, BodyExceededMaxSize) + self._assert_error(deferred, protocol) self._cleanup_error(deferred) From 72368b1b8b270fd0c632abcfe27db4cb2ac62dfa Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 25 Feb 2021 10:13:56 -0500 Subject: [PATCH 3/4] Close the connection when the maximum body size is exceeded due to headers. --- synapse/http/client.py | 31 +++++++++++++++++++++++++++++-- tests/http/test_client.py | 15 +++++++++++++++ 2 files changed, 44 insertions(+), 2 deletions(-) diff --git a/synapse/http/client.py b/synapse/http/client.py index e54d9bd213c9..d71b042638f5 100644 --- a/synapse/http/client.py +++ b/synapse/http/client.py @@ -750,7 +750,32 @@ class BodyExceededMaxSize(Exception): """The maximum allowed size of the HTTP body was exceeded.""" +class _DiscardBodyWithMaxSizeProtocol(protocol.Protocol): + """A protocol which immediately errors upon receiving data.""" + + def __init__(self, deferred: defer.Deferred): + self.deferred = deferred + + def _maybe_fail(self): + """ + Report a max size exceed error and disconnect the first time this is called. + """ + if not self.deferred.called: + self.deferred.errback(BodyExceededMaxSize()) + # Close the connection (forcefully) since all the data will get + # discarded anyway. + self.transport.abortConnection() + + def dataReceived(self, data: bytes) -> None: + self._maybe_fail() + + def connectionLost(self, reason: Failure) -> None: + self._maybe_fail() + + class _ReadBodyWithMaxSizeProtocol(protocol.Protocol): + """A protocol which reads body to a stream, erroring if the body exceeds a maximum size.""" + def __init__( self, stream: BinaryIO, deferred: defer.Deferred, max_size: Optional[int] ): @@ -807,13 +832,15 @@ def read_body_with_max_size( Returns: A Deferred which resolves to the length of the read body. """ + d = defer.Deferred() + # If the Content-Length header gives a size larger than the maximum allowed # size, do not bother downloading the body. if max_size is not None and response.length != UNKNOWN_LENGTH: if response.length > max_size: - return defer.fail(BodyExceededMaxSize()) + response.deliverBody(_DiscardBodyWithMaxSizeProtocol(d)) + return d - d = defer.Deferred() response.deliverBody(_ReadBodyWithMaxSizeProtocol(stream, d, max_size)) return d diff --git a/tests/http/test_client.py b/tests/http/test_client.py index ffff09819cdc..21ecb81c9926 100644 --- a/tests/http/test_client.py +++ b/tests/http/test_client.py @@ -104,3 +104,18 @@ def test_additional_data(self): self.assertEqual(result.getvalue(), b"1234567890") self._assert_error(deferred, protocol) self._cleanup_error(deferred) + + def test_content_length(self): + """The body shouldn't be read (at all) if the Content-Length header is too large.""" + result, deferred, protocol = self._build_response(length=10) + + # Deferred shouldn't be called yet. + self.assertFalse(deferred.called) + + # Start sending data. + protocol.dataReceived(b"12345") + self._assert_error(deferred, protocol) + self._cleanup_error(deferred) + + # The data is never consumed. + self.assertEqual(result.getvalue(), b"") From bf7ba5c49a008f452dedc480f6f4aa05338c4faf Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 25 Feb 2021 10:19:09 -0500 Subject: [PATCH 4/4] Newsfragment --- changelog.d/9497.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/9497.bugfix diff --git a/changelog.d/9497.bugfix b/changelog.d/9497.bugfix new file mode 100644 index 000000000000..598bcaab673b --- /dev/null +++ b/changelog.d/9497.bugfix @@ -0,0 +1 @@ +Fix a long-standing bug where the media repository could leak file descriptors while previewing media.