launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/remove-nul-from-logtail into lp:launchpad.

Commit message:
Filter ASCII NUL characters out of build logtails.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1831500 in Launchpad itself: "buildd-manager crashes on ASCII NUL in build logtail"
  https://bugs.launchpad.net/launchpad/+bug/1831500

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/remove-nul-from-logtail/+merge/368308
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/remove-nul-from-logtail into lp:launchpad.

=== modified file 'lib/lp/buildmaster/model/buildqueue.py'
--- lib/lp/buildmaster/model/buildqueue.py	2017-04-27 15:54:54 +0000
+++ lib/lp/buildmaster/model/buildqueue.py	2019-06-04 08:41:47 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2017 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2019 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -192,6 +192,10 @@
             # it into Unicode as best we can.
             self.logtail = str(
                 slave_status.get("logtail")).decode("UTF-8", errors="replace")
+            # PostgreSQL text columns can't contain \0 characters, and since
+            # we only use this for web UI display purposes there's no point
+            # in going through contortions to store them.
+            self.logtail = self.logtail.replace("\0", "")
 
     def suspend(self):
         """See `IBuildQueue`."""

=== modified file 'lib/lp/buildmaster/tests/test_manager.py'
--- lib/lp/buildmaster/tests/test_manager.py	2018-06-06 08:47:46 +0000
+++ lib/lp/buildmaster/tests/test_manager.py	2019-06-04 08:41:47 +0000
@@ -2,7 +2,7 @@
 # NOTE: The first line above must stay first; do not move the copyright
 # notice to the top.  See http://www.python.org/dev/peps/pep-0263/.
 #
-# Copyright 2009-2018 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2019 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for the renovated slave scanner aka BuilddManager."""
@@ -347,6 +347,34 @@
         d.addCallback(
             self._checkJobUpdated, builder, job, logtail=u"\uFFFD\uFFFD──")
 
+    def test_scan_of_logtail_containing_nul(self):
+        # PostgreSQL text columns can't store ASCII NUL (\0) characters, so
+        # we make sure to filter those out of the logtail.
+        class NULSlave(BuildingSlave):
+            @defer.inlineCallbacks
+            def status(self):
+                status = yield super(NULSlave, self).status()
+                status["logtail"] = xmlrpclib.Binary(b"foo\0bar\0baz")
+                defer.returnValue(status)
+
+        builder = getUtility(IBuilderSet)[BOB_THE_BUILDER_NAME]
+        login('foo.bar@xxxxxxxxxxxxx')
+        builder.builderok = True
+        self.patch(
+            BuilderSlave, 'makeBuilderSlave',
+            FakeMethod(NULSlave(build_id='PACKAGEBUILD-8')))
+        transaction.commit()
+        login(ANONYMOUS)
+
+        job = builder.currentjob
+        self.assertBuildingJob(job, builder)
+
+        switch_dbuser(config.builddmaster.dbuser)
+        scanner = self._getScanner()
+        d = defer.maybeDeferred(scanner.scan)
+        d.addCallback(
+            self._checkJobUpdated, builder, job, logtail=u"foobarbaz")
+
     @defer.inlineCallbacks
     def test_scan_calls_builder_factory_prescanUpdate(self):
         # SlaveScanner.scan() starts by calling