launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/librarian-check-content-length into lp:launchpad.

Commit message:
Check Content-Length in librarian responses.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/librarian-check-content-length/+merge/296120

Check Content-Length in librarian responses.  httplib has a corner case where it doesn't do that for us in exactly the situation that's most common when streaming files from the librarian.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/librarian-check-content-length into lp:launchpad.

=== modified file 'lib/lp/services/daemons/tachandler.py'
--- lib/lp/services/daemons/tachandler.py	2014-04-25 12:31:15 +0000
+++ lib/lp/services/daemons/tachandler.py	2016-05-31 17:02:35 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2014 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2016 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Test harness for TAC (Twisted Application Configuration) files."""
@@ -44,7 +44,7 @@
         # readyservice.LOG_MAGIC string and return immediately, provoking
         # hard-to-diagnose race conditions. Delete the logfile to make sure
         # this does not happen.
-        remove_if_exists(self.logfile)
+        self.removeLog()
         with override_environ(LP_DEBUG_SQL=None):
             TacTestFixture.setUp(self,
                 python_path=sys.executable,
@@ -62,6 +62,9 @@
         else:
             return False
 
+    def removeLog(self):
+        remove_if_exists(self.logfile)
+
     def truncateLog(self):
         """Truncate the log file.
 

=== modified file 'lib/lp/services/librarian/client.py'
--- lib/lp/services/librarian/client.py	2015-12-10 00:37:05 +0000
+++ lib/lp/services/librarian/client.py	2016-05-31 17:02:35 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2016 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -14,6 +14,7 @@
 
 
 import hashlib
+import httplib
 from select import select
 import socket
 from socket import (
@@ -285,6 +286,18 @@
     def __init__(self, file, url):
         self.file = file
         self.url = url
+        headers = file.info()
+        chunked = (headers.get("Transfer-Encoding") == "chunked")
+        if not chunked and "Content-Length" in headers:
+            try:
+                self.length = int(headers["Content-Length"])
+            except ValueError:
+                self.length = None
+            else:
+                if self.length < 0:
+                    self.length = None
+        else:
+            self.length = None
 
     def read(self, chunksize=None):
         request = get_current_browser_request()
@@ -292,9 +305,20 @@
         action = timeline.start("librarian-read", self.url)
         try:
             if chunksize is None:
-                return self.file.read()
+                s = self.file.read()
             else:
-                return self.file.read(chunksize)
+                if self.length is not None:
+                    chunksize = min(chunksize, self.length)
+                s = self.file.read(chunksize)
+            if self.length is not None:
+                self.length -= len(s)
+                # httplib doesn't quite do all the checking we need: in
+                # particular, it won't fail on a short bounded-size read
+                # from a non-chunked-transfer-coding resource.  Check this
+                # manually.
+                if not s and chunksize != 0 and self.length:
+                    raise httplib.IncompleteRead(s)
+            return s
         finally:
             action.finish()
 

=== added file 'lib/lp/services/librarian/tests/fakeserver.tac'
--- lib/lp/services/librarian/tests/fakeserver.tac	1970-01-01 00:00:00 +0000
+++ lib/lp/services/librarian/tests/fakeserver.tac	2016-05-31 17:02:35 +0000
@@ -0,0 +1,56 @@
+# Copyright 2016 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""A fake server that can lie about Content-Length."""
+
+__metaclass__ = type
+
+import os
+
+from twisted.application import (
+    service,
+    strports,
+    )
+from twisted.web import (
+    resource,
+    server,
+    )
+
+from lp.services.daemons import readyservice
+
+
+class FakeResource(resource.Resource):
+
+    isLeaf = True
+
+    def render_GET(self, request):
+        return b"abcdef"
+
+
+class FakeRequest(server.Request):
+    """A Request that can send a Content-Length larger than the content."""
+
+    def __init__(self, *args, **kwargs):
+        server.Request.__init__(self, *args, **kwargs)
+        if "LP_EXTRA_CONTENT_LENGTH" in os.environ:
+            self.extra_content_length = int(
+                os.environ["LP_EXTRA_CONTENT_LENGTH"])
+        else:
+            self.extra_content_length = None
+
+    def setHeader(self, name, value):
+        if (name.lower() == b"content-length" and
+                self.extra_content_length is not None):
+            value = str(int(value) + self.extra_content_length).encode("UTF-8")
+        server.Request.setHeader(self, name, value)
+
+
+application = service.Application("FakeServer")
+services = service.IServiceCollection(application)
+
+readyservice.ReadyService().setServiceParent(services)
+
+site = server.Site(FakeResource())
+site.requestFactory = FakeRequest
+site.displayTracebacks = False
+strports.service("tcp:0", site).setServiceParent(services)

=== modified file 'lib/lp/services/librarian/tests/test_client.py'
--- lib/lp/services/librarian/tests/test_client.py	2014-01-30 15:04:06 +0000
+++ lib/lp/services/librarian/tests/test_client.py	2016-05-31 17:02:35 +0000
@@ -1,30 +1,41 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2016 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 from cStringIO import StringIO
 import hashlib
 import httplib
+import os
+import re
 import textwrap
 import unittest
 from urllib2 import (
     HTTPError,
     URLError,
+    urlopen,
     )
 
+from fixtures import (
+    EnvironmentVariable,
+    TempDir,
+    )
 import transaction
 
 from lp.services.config import config
+from lp.services.daemons.tachandler import TacTestSetup
 from lp.services.database.interfaces import ISlaveStore
 from lp.services.database.policy import SlaveDatabasePolicy
 from lp.services.database.sqlbase import block_implicit_flushes
 from lp.services.librarian import client as client_module
 from lp.services.librarian.client import (
+    _File,
     LibrarianClient,
     LibrarianServerError,
     RestrictedLibrarianClient,
     )
 from lp.services.librarian.interfaces.client import UploadFailed
 from lp.services.librarian.model import LibraryFileAlias
+from lp.services.propertycache import cachedproperty
+from lp.testing import TestCase
 from lp.testing.layers import (
     DatabaseLayer,
     FunctionalLayer,
@@ -79,6 +90,80 @@
     return mock_file
 
 
+class FakeServerTestSetup(TacTestSetup):
+
+    def setUp(self):
+        self.port = None
+        super(FakeServerTestSetup, self).setUp()
+
+    def setUpRoot(self):
+        pass
+
+    @cachedproperty
+    def root(self):
+        return self.useFixture(TempDir()).path
+
+    @property
+    def tacfile(self):
+        return os.path.join(os.path.dirname(__file__), "fakeserver.tac")
+
+    @property
+    def pidfile(self):
+        return os.path.join(self.root, "fakeserver.pid")
+
+    @property
+    def logfile(self):
+        return os.path.join(self.root, "fakeserver.log")
+
+    def removeLog(self):
+        pass
+
+    def _hasDaemonStarted(self):
+        if super(FakeServerTestSetup, self)._hasDaemonStarted():
+            with open(self.logfile) as logfile:
+                self.port = int(re.search(
+                    r"Site starting on (\d+)", logfile.read()).group(1))
+            return True
+        else:
+            return False
+
+
+class LibrarianFileWrapperTestCase(TestCase):
+    """Test behaviour of the _File wrapper used by the librarian client."""
+
+    def makeFile(self, extra_content_length=None):
+        extra_content_length_str = (
+            str(extra_content_length) if extra_content_length is not None
+            else None)
+        self.useFixture(EnvironmentVariable(
+            "LP_EXTRA_CONTENT_LENGTH", extra_content_length_str))
+        port = self.useFixture(FakeServerTestSetup()).port
+        url = "http://localhost:%d/"; % port
+        return _File(urlopen(url), url)
+
+    def test_unbounded_read_correct_length(self):
+        file = self.makeFile()
+        self.assertEqual(b"abcdef", file.read())
+        self.assertEqual(b"", file.read())
+
+    def test_unbounded_read_incorrect_length(self):
+        file = self.makeFile(extra_content_length=1)
+        self.assertEqual(b"abcdef", file.read())
+        self.assertRaises(httplib.IncompleteRead, file.read)
+
+    def test_bounded_read_correct_length(self):
+        file = self.makeFile()
+        self.assertEqual(b"abcd", file.read(chunksize=4))
+        self.assertEqual(b"ef", file.read(chunksize=4))
+        self.assertEqual(b"", file.read(chunksize=4))
+
+    def test_bounded_read_incorrect_length(self):
+        file = self.makeFile(extra_content_length=1)
+        self.assertEqual(b"abcd", file.read(chunksize=4))
+        self.assertEqual(b"ef", file.read(chunksize=4))
+        self.assertRaises(httplib.IncompleteRead, file.read, chunksize=4)
+
+
 class LibrarianClientTestCase(unittest.TestCase):
     layer = LaunchpadFunctionalLayer