Merge lp:~michael.nelson/launchpad/550049-overwriting-buildd-manager-files-2 into lp:launchpad

[Michael Nelson]

Adds basic test for buildStatus_OK, ensuring that paths outside the upload dir are not written.

This branch is just a merge of a fix that is already on production.

That (private) MP can be seen at:
https://code.edge.launchpad.net/~michael.nelson/launchpad/pd-550049-overwriting-buildd-manager-files/+merge/22932

[Julian Edwards (julian-edwards)]

rs=me, this is already landed in production

review: Approve

[Michael Nelson (michael.nelson)]

review: Approve (code)

=== modified file 'lib/lp/buildmaster/model/buildbase.py'
--- lib/lp/buildmaster/model/buildbase.py	2010-03-28 09:35:12 +0000
+++ lib/lp/buildmaster/model/buildbase.py	2010-04-14 14:24:28 +0000
@@ -43,7 +43,7 @@
     package.
 
     Note: this class does not implement IBuildBase as we currently duplicate
-    the properties defined on IBuildBase on the inheriting class tables. 
+    the properties defined on IBuildBase on the inheriting class tables.
     BuildBase cannot therefore implement IBuildBase itself, as storm requires
     that the corresponding __storm_table__ be defined for the class. Instead,
     the classes using the BuildBase mixin must ensure that they implement IBuildBase.
@@ -148,7 +148,6 @@
         directory, store build information and push them through the
         uploader.
         """
-        # XXX cprov 2007-07-11 bug=129487: untested code path.
         filemap = slave_status['filemap']
 
         logger.info("Processing successful build %s from builder %s" % (
@@ -182,33 +181,48 @@
         os.makedirs(upload_path)
 
         slave = removeSecurityProxy(self.buildqueue_record.builder.slave)
+        successful_copy_from_slave = True
         for filename in filemap:
             logger.info("Grabbing file: %s" % filename)
-            slave_file = slave.getFile(filemap[filename])
             out_file_name = os.path.join(upload_path, filename)
+            # If the evaluated output file name is not within our
+            # upload path, then we don't try to copy this or any
+            # subsequent files.
+            if not os.path.realpath(out_file_name).startswith(upload_path):
+                successful_copy_from_slave = False
+                logger.warning(
+                    "A slave tried to upload the file '%s' "
+                    "for the build %d." % (filename, self.id))
+                break
             out_file = open(out_file_name, "wb")
+            slave_file = slave.getFile(filemap[filename])
             copy_and_close(slave_file, out_file)
 
-        uploader_logfilename = os.path.join(upload_dir, UPLOAD_LOG_FILENAME)
-        uploader_command = self.getUploaderCommand(
-            upload_leaf, uploader_logfilename)
-        logger.debug("Saving uploader log at '%s'" % uploader_logfilename)
-
-        logger.info("Invoking uploader on %s" % root)
-        logger.info("%s" % uploader_command)
-
-        uploader_process = subprocess.Popen(
-            uploader_command, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
-
-        # Nothing should be written to the stdout/stderr.
-        upload_stdout, upload_stderr = uploader_process.communicate()
-
-        # XXX cprov 2007-04-17: we do not check uploader_result_code
-        # anywhere. We need to find out what will be best strategy
-        # when it failed HARD (there is a huge effort in process-upload
-        # to not return error, it only happen when the code is broken).
-        uploader_result_code = uploader_process.returncode
-        logger.info("Uploader returned %d" % uploader_result_code)
+        # We only attempt the upload if we successfully copied all the
+        # files from the slave.
+        if successful_copy_from_slave:
+            uploader_logfilename = os.path.join(
+                upload_dir, UPLOAD_LOG_FILENAME)
+            uploader_command = self.getUploaderCommand(
+                upload_leaf, uploader_logfilename)
+            logger.debug("Saving uploader log at '%s'" % uploader_logfilename)
+
+            logger.info("Invoking uploader on %s" % root)
+            logger.info("%s" % uploader_command)
+
+            uploader_process = subprocess.Popen(
+                uploader_command, stdout=subprocess.PIPE,
+                stderr=subprocess.PIPE)
+
+            # Nothing should be written to the stdout/stderr.
+            upload_stdout, upload_stderr = uploader_process.communicate()
+
+            # XXX cprov 2007-04-17: we do not check uploader_result_code
+            # anywhere. We need to find out what will be best strategy
+            # when it failed HARD (there is a huge effort in process-upload
+            # to not return error, it only happen when the code is broken).
+            uploader_result_code = uploader_process.returncode
+            logger.info("Uploader returned %d" % uploader_result_code)
 
         # Quick and dirty hack to carry on on process-upload failures
         if os.path.exists(upload_dir):
@@ -264,6 +278,7 @@
         # also contain the information required to manually reprocess the
         # binary upload when it was the case.
         if (self.buildstate != BuildStatus.FULLYBUILT or
+            not successful_copy_from_slave or
             not self.verifySuccessfulUpload()):
             logger.warning("Build %s upload failed." % self.id)
             self.buildstate = BuildStatus.FAILEDTOUPLOAD

=== modified file 'lib/lp/buildmaster/tests/test_buildbase.py'
--- lib/lp/buildmaster/tests/test_buildbase.py	2010-03-03 02:40:10 +0000
+++ lib/lp/buildmaster/tests/test_buildbase.py	2010-04-14 14:24:28 +0000
@@ -12,10 +12,16 @@
 import unittest
 
 from canonical.config import config
-from canonical.testing.layers import DatabaseFunctionalLayer
+from canonical.database.constants import UTC_NOW
+from canonical.testing.layers import (
+    DatabaseFunctionalLayer, LaunchpadZopelessLayer)
+from lp.buildmaster.interfaces.buildbase import BuildStatus
 from lp.buildmaster.model.buildbase import BuildBase
 from lp.registry.interfaces.pocket import pocketsuffix
+from lp.soyuz.tests.soyuzbuilddhelpers import WaitingSlave
+from lp.soyuz.tests.test_publishing import SoyuzTestPublisher
 from lp.testing import TestCase, TestCaseWithFactory
+from lp.testing.fakemethod import FakeMethod
 
 
 class TestBuildBase(TestCase):
@@ -48,11 +54,11 @@
     layer = DatabaseFunctionalLayer
 
     def test_getUploadLogContent_nolog(self):
-        """If there is no log file there, a string explaining that is returned.
+        """If there is no log file there, a string explanation is returned.
         """
         self.useTempDir()
         build_base = BuildBase()
-        self.assertEquals('Could not find upload log file', 
+        self.assertEquals('Could not find upload log file',
             build_base.getUploadLogContent(os.getcwd(), "myleaf"))
 
     def test_getUploadLogContent_only_dir(self):
@@ -61,7 +67,7 @@
         self.useTempDir()
         os.makedirs("accepted/myleaf")
         build_base = BuildBase()
-        self.assertEquals('Could not find upload log file', 
+        self.assertEquals('Could not find upload log file',
             build_base.getUploadLogContent(os.getcwd(), "myleaf"))
 
     def test_getUploadLogContent_readsfile(self):
@@ -99,5 +105,69 @@
         self.assertEqual(config_args, uploader_command)
 
 
+class TestBuildBaseHandleStatus(TestCaseWithFactory):
+    """Tests for `IBuildBase`s handleStatus method."""
+
+    layer = LaunchpadZopelessLayer
+
+    def setUp(self):
+        super(TestBuildBaseHandleStatus, self).setUp()
+        test_publisher = SoyuzTestPublisher()
+        test_publisher.prepareBreezyAutotest()
+        binaries = test_publisher.getPubBinaries()
+        self.build = binaries[0].binarypackagerelease.build
+
+        # For the moment, we require a builder for the build so that
+        # handleStatus_OK can get a reference to the slave.
+        builder = self.factory.makeBuilder()
+        self.build.buildqueue_record.builder = builder
+        self.build.buildqueue_record.setDateStarted(UTC_NOW)
+        self.slave = WaitingSlave('BuildStatus.OK')
+        self.slave.valid_file_hashes.append('test_file_hash')
+        builder.setSlaveForTesting(self.slave)
+
+        # We overwrite the buildmaster root to use a temp directory.
+        tmp_dir = self.makeTemporaryDirectory()
+        tmp_builddmaster_root = """
+        [builddmaster]
+        root: %s
+        """ % tmp_dir
+        config.push('tmp_builddmaster_root', tmp_builddmaster_root)
+
+        # We stub out our builds getUploaderCommand() method so
+        # we can check whether it was called.
+        self.build.getUploaderCommand = FakeMethod(
+            result=['echo', 'noop'])
+
+    def test_handleStatus_OK_normal_file(self):
+        # A filemap with plain filenames should not cause a problem.
+        # The call to handleStatus will attempt to get the file from
+        # the slave resulting in a URL error in this test case.
+        self.build.handleStatus('OK', None, {
+                'filemap': { 'myfile.py': 'test_file_hash'},
+                })
+
+        self.assertEqual(BuildStatus.FULLYBUILT, self.build.buildstate)
+        self.assertEqual(1, self.build.getUploaderCommand.call_count)
+
+    def test_handleStatus_OK_absolute_filepath(self):
+        # A filemap that tries to write to files outside of
+        # the upload directory will result in a failed upload.
+        self.build.handleStatus('OK', None, {
+            'filemap': { '/tmp/myfile.py': 'test_file_hash'},
+            })
+        self.assertEqual(BuildStatus.FAILEDTOUPLOAD, self.build.buildstate)
+        self.assertEqual(0, self.build.getUploaderCommand.call_count)
+
+    def test_handleStatus_OK_relative_filepath(self):
+        # A filemap that tries to write to files outside of
+        # the upload directory will result in a failed upload.
+        self.build.handleStatus('OK', None, {
+            'filemap': { '../myfile.py': 'test_file_hash'},
+            })
+        self.assertEqual(BuildStatus.FAILEDTOUPLOAD, self.build.buildstate)
+        self.assertEqual(0, self.build.getUploaderCommand.call_count)
+
+
 def test_suite():
     return unittest.TestLoader().loadTestsFromName(__name__)

=== modified file 'lib/lp/soyuz/tests/soyuzbuilddhelpers.py'
--- lib/lp/soyuz/tests/soyuzbuilddhelpers.py	2010-04-12 05:52:01 +0000
+++ lib/lp/soyuz/tests/soyuzbuilddhelpers.py	2010-04-14 14:24:28 +0000
@@ -173,14 +173,19 @@
         self.dependencies = dependencies
         self.build_id = build_id
 
+        # By default, the slave only has a buildlog, but callsites
+        # can update this list as needed.
+        self.valid_file_hashes = ['buildlog']
+
     def status(self):
         return ('BuilderStatus.WAITING', self.state, self.build_id, {},
                 self.dependencies)
 
-    def getFile(self, sum):
-        if sum == "buildlog":
-            s = StringIO("This is a build log")
-            s.headers = {'content-length':19}
+    def getFile(self, hash):
+        if hash in self.valid_file_hashes:
+            content = "This is a %s" % hash
+            s = StringIO(content)
+            s.headers = {'content-length':len(content)}
             return s