From 25ebf3aa9e6e98974565ae1b70fc05b49423e17c Mon Sep 17 00:00:00 2001 From: Samuel Merritt Date: Thu, 20 Feb 2014 20:33:00 -0800 Subject: [PATCH] Raise error on long or short DLO The GET response for a DLO has a Content-Length that's computed from the container listing, but the response body's length is determined by the segments. If a segment grows or shrinks between when the headers are sent and when the segment is requested, this confuses clients. For example, if the DLO is longer than its Content-Length and the client sends another request on the same TCP connection, then it can get leftover object bytes instead of an HTTP status line. Alternately, if the headers it sends fill up the TCP buffers since Swift won't read them until the first response is done, then deadlock hilarity ensues. If the DLO is shorter than its Content-Length, you're pretty much guaranteed a deadlock: the client is waiting for the rest of the response, and the server is waiting for a new request. Now SegmentedIterable detects both these conditions and raises an exception so that the TCP connection gets torn down. It can't save this request, but it can stop the next one from getting hosed too. Change-Id: Icf79ba046ef7aaaab49ce6d0b33147332c967afc --- swift/common/request_helpers.py | 29 +++++++++- test/unit/common/middleware/test_dlo.py | 76 +++++++++++++++++++++++++ 2 files changed, 103 insertions(+), 2 deletions(-) diff --git a/swift/common/request_helpers.py b/swift/common/request_helpers.py index a191096206..a75c3452aa 100644 --- a/swift/common/request_helpers.py +++ b/swift/common/request_helpers.py @@ -239,7 +239,7 @@ class SegmentedIterable(object): :param ua_suffix: string to append to user-agent. :param name: name of manifest (used in logging only) :param response: optional response object for the response being sent - to the client. Only affects logs. + to the client. """ def __init__(self, req, app, listing_iter, max_get_time, logger, ua_suffix, swift_source, @@ -267,6 +267,12 @@ class SegmentedIterable(object): def __iter__(self): start_time = time.time() have_yielded_data = False + + if self.response and self.response.content_length: + bytes_left = int(self.response.content_length) + else: + bytes_left = None + try: for seg_path, seg_etag, seg_size, first_byte, last_byte \ in self.listing_iter: @@ -317,9 +323,28 @@ class SegmentedIterable(object): 's_size': seg_size}) for chunk in seg_resp.app_iter: - yield chunk have_yielded_data = True + if bytes_left is None: + yield chunk + elif bytes_left >= len(chunk): + yield chunk + bytes_left -= len(chunk) + else: + yield chunk[:bytes_left] + bytes_left -= len(chunk) + close_if_possible(seg_resp.app_iter) + raise SegmentError( + 'Too many bytes for %(name)s; truncating in ' + '%(seg)s with %(left)d bytes left' % + {'name': self.name, 'seg': seg_req.path, + 'left': bytes_left}) close_if_possible(seg_resp.app_iter) + + if bytes_left: + raise SegmentError( + 'Not enough bytes for %s; closing connection' % + self.name) + except ListingIterError as err: # I have to save this error because yielding the ' ' below clears # the exception from the current stack frame. diff --git a/test/unit/common/middleware/test_dlo.py b/test/unit/common/middleware/test_dlo.py index 52989fe278..08db16ebf3 100644 --- a/test/unit/common/middleware/test_dlo.py +++ b/test/unit/common/middleware/test_dlo.py @@ -682,6 +682,82 @@ class TestDloGetManifest(DloTestCase): self.assertEqual(body, 'aaaaabbbbbccccc') self.assertTrue(isinstance(exc, exceptions.SegmentError)) + def test_get_oversize_segment(self): + # If we send a Content-Length header to the client, it's based on the + # container listing. If a segment gets bigger by the time we get to it + # (like if a client uploads a bigger segment w/the same name), we need + # to not send anything beyond the length we promised. Also, we should + # probably raise an exception. + + # This is now longer than the original seg_03+seg_04+seg_05 combined + self.app.register( + 'GET', '/v1/AUTH_test/c/seg_03', + swob.HTTPOk, {'Content-Length': '20', 'Etag': 'seg03-etag'}, + 'cccccccccccccccccccc') + + req = swob.Request.blank( + '/v1/AUTH_test/mancon/manifest', + environ={'REQUEST_METHOD': 'GET'}) + status, headers, body, exc = self.call_dlo(req, expect_exception=True) + headers = swob.HeaderKeyDict(headers) + + self.assertEqual(status, '200 OK') # sanity check + self.assertEqual(headers.get('Content-Length'), '25') # sanity check + self.assertEqual(body, 'aaaaabbbbbccccccccccccccc') + self.assertTrue(isinstance(exc, exceptions.SegmentError)) + self.assertEqual( + self.app.calls, + [('GET', '/v1/AUTH_test/mancon/manifest'), + ('GET', '/v1/AUTH_test/c?format=json&prefix=seg'), + ('GET', '/v1/AUTH_test/c/seg_01'), + ('GET', '/v1/AUTH_test/c/seg_02'), + ('GET', '/v1/AUTH_test/c/seg_03')]) + + def test_get_undersize_segment(self): + # If we send a Content-Length header to the client, it's based on the + # container listing. If a segment gets smaller by the time we get to + # it (like if a client uploads a smaller segment w/the same name), we + # need to raise an exception so that the connection will be closed by + # the WSGI server. Otherwise, the WSGI server will be waiting for the + # next request, the client will still be waiting for the rest of the + # response, and nobody will be happy. + + # Shrink it by a single byte + self.app.register( + 'GET', '/v1/AUTH_test/c/seg_03', + swob.HTTPOk, {'Content-Length': '4', 'Etag': 'seg03-etag'}, + 'cccc') + + req = swob.Request.blank( + '/v1/AUTH_test/mancon/manifest', + environ={'REQUEST_METHOD': 'GET'}) + status, headers, body, exc = self.call_dlo(req, expect_exception=True) + headers = swob.HeaderKeyDict(headers) + + self.assertEqual(status, '200 OK') # sanity check + self.assertEqual(headers.get('Content-Length'), '25') # sanity check + self.assertEqual(body, 'aaaaabbbbbccccdddddeeeee') + self.assertTrue(isinstance(exc, exceptions.SegmentError)) + + def test_get_undersize_segment_range(self): + # Shrink it by a single byte + self.app.register( + 'GET', '/v1/AUTH_test/c/seg_03', + swob.HTTPOk, {'Content-Length': '4', 'Etag': 'seg03-etag'}, + 'cccc') + + req = swob.Request.blank( + '/v1/AUTH_test/mancon/manifest', + environ={'REQUEST_METHOD': 'GET'}, + headers={'Range': 'bytes=0-14'}) + status, headers, body, exc = self.call_dlo(req, expect_exception=True) + headers = swob.HeaderKeyDict(headers) + + self.assertEqual(status, '206 Partial Content') # sanity check + self.assertEqual(headers.get('Content-Length'), '15') # sanity check + self.assertEqual(body, 'aaaaabbbbbcccc') + self.assertTrue(isinstance(exc, exceptions.SegmentError)) + def fake_start_response(*args, **kwargs): pass