Merge lp:~cjwatson/launchpad/limit-debdiff into lp:launchpad

[Colin Watson]

Kill debdiff after ten minutes or 1GiB of output by default, and make sure we clean up after it properly. Add a configurable blacklist.

Kill debdiff after ten minutes or 1GiB of output by default, and make sure we clean up after it properly. Add a configurable blacklist.

Some source packages that contain particularly convoluted symlink farms can confuse debdiff into producing exponentially large output, and we should guard ourselves against this possibility. Ten minutes seems to be a reasonable threshold, as it's larger than the time taken for 99.9% of all successful PackageDiffJobs in 2015 to complete, but I've made it configurable in case we need to tweak it in future. I've arranged to set TMPDIR because debdiff creates some of its own temporary files there and may not clean them up properly if it's killed.

[William Grant (wgrant)]

review: Approve (code)

[Robert Collins (lifeless)]

It might be a good idea to set a direct metric on the output work too - e.g. set a ulimit on memory, total cpu or when more than some N (e.g. 1M) of output is generated stop immediately.

=== modified file 'lib/lp/services/config/schema-lazr.conf'
--- lib/lp/services/config/schema-lazr.conf	2015-11-17 01:39:17 +0000
+++ lib/lp/services/config/schema-lazr.conf	2015-12-02 11:27:35 +0000
@@ -1387,6 +1387,22 @@
 port: 11217
 
 
+[packagediff]
+# The timeout in seconds for a debdiff between two packages.
+# datatype: integer
+debdiff_timeout: 600
+
+# The maximum size in bytes of any file created while debdiffing two
+# packages.
+# 10GiB
+# datatype: integer
+debdiff_max_size: 10737418240
+
+# Packages we never try to generate diffs for.
+# datatype: string
+blacklist:
+
+
 [person_notification]
 # User for person notification db access
 # datatype: string

=== modified file 'lib/lp/soyuz/doc/package-diff.txt'
--- lib/lp/soyuz/doc/package-diff.txt	2013-08-06 09:49:14 +0000
+++ lib/lp/soyuz/doc/package-diff.txt	2015-12-02 11:27:35 +0000
@@ -649,33 +649,6 @@
     None
 
 
-Problematic packages
---------------------
-
-XXX 2009-11-23 Julian bug=314436
-Because of bug 314436, diffs of udev can generate huge output which fills the
-disk very quickly.  For that reason, diffs of udev are created FAILED by
-default, which will stop the diff script from attempting to create the diff.
-
-    >>> from lp.soyuz.enums import PackagePublishingStatus
-    >>> from lp.soyuz.tests.test_publishing import SoyuzTestPublisher
-    >>> stp = SoyuzTestPublisher()
-    >>> discard = stp.setUpDefaultDistroSeries(hoary)
-    >>> udev_orig = stp.getPubSource(
-    ...     sourcename="udev", version="1.0",
-    ...     status=PackagePublishingStatus.PUBLISHED)
-    >>> udev_new = stp.getPubSource(
-    ...     sourcename="udev", version="1.1",
-    ...     status=PackagePublishingStatus.PENDING)
-
-    >>> udev_diff = udev_orig.sourcepackagerelease.requestDiffTo(
-    ...     requester=cprov,
-    ...     to_sourcepackagerelease=udev_new.sourcepackagerelease)
-
-    >>> print udev_diff.status.name
-    FAILED
-
-
 PackageDiff privacy
 -------------------
 

=== modified file 'lib/lp/soyuz/model/packagediff.py'
--- lib/lp/soyuz/model/packagediff.py	2015-07-08 16:05:11 +0000
+++ lib/lp/soyuz/model/packagediff.py	2015-12-02 11:27:35 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -7,10 +7,13 @@
     'PackageDiffSet',
     ]
 
+from functools import partial
 import gzip
 import itertools
 import os
+import resource
 import shutil
+import signal
 import subprocess
 import tempfile
 
@@ -20,6 +23,7 @@
 from zope.component import getUtility
 from zope.interface import implementer
 
+from lp.services.config import config
 from lp.services.database.bulk import load
 from lp.services.database.constants import UTC_NOW
 from lp.services.database.datetimecol import UtcDateTimeCol
@@ -43,6 +47,19 @@
     )
 
 
+def limit_deb_diff(timeout, max_size):
+    """Pre-exec function to apply resource limits to debdiff.
+
+    :param timeout: Time limit in seconds.
+    :param max_size: Maximum output file size in bytes.
+    """
+    signal.alarm(timeout)
+    _, hard_fsize = resource.getrlimit(resource.RLIMIT_FSIZE)
+    if hard_fsize != resource.RLIM_INFINITY and hard_fsize < max_size:
+        max_size = hard_fsize
+    resource.setrlimit(resource.RLIMIT_FSIZE, (max_size, hard_fsize))
+
+
 def perform_deb_diff(tmp_dir, out_filename, from_files, to_files):
     """Perform a (deb)diff on two packages.
 
@@ -67,13 +84,20 @@
     [to_dsc] = [name for name in to_files
                 if name.lower().endswith('.dsc')]
     args = ['debdiff', from_dsc, to_dsc]
+    env = os.environ.copy()
+    env['TMPDIR'] = tmp_dir
 
     full_path = os.path.join(tmp_dir, out_filename)
     out_file = None
     try:
         out_file = open(full_path, 'w')
         process = subprocess.Popen(
-            args, stdout=out_file, stderr=subprocess.PIPE, cwd=tmp_dir)
+            args, stdout=out_file, stderr=subprocess.PIPE,
+            preexec_fn=partial(
+                limit_deb_diff,
+                config.packagediff.debdiff_timeout,
+                config.packagediff.debdiff_max_size),
+            cwd=tmp_dir, env=env)
         stdout, stderr = process.communicate()
     finally:
         if out_file is not None:
@@ -173,6 +197,11 @@
             self.status = PackageDiffStatus.FAILED
             return
 
+        blacklist = config.packagediff.blacklist.split()
+        if self.from_source.sourcepackagename.name in blacklist:
+            self.status = PackageDiffStatus.FAILED
+            return
+
         # Create the temporary directory where the files will be
         # downloaded to and where the debdiff will be performed.
         tmp_dir = tempfile.mkdtemp()

=== modified file 'lib/lp/soyuz/model/sourcepackagerelease.py'
--- lib/lp/soyuz/model/sourcepackagerelease.py	2015-07-08 16:05:11 +0000
+++ lib/lp/soyuz/model/sourcepackagerelease.py	2015-12-02 11:27:35 +0000
@@ -60,7 +60,6 @@
     cachedproperty,
     get_property_cache,
     )
-from lp.soyuz.enums import PackageDiffStatus
 from lp.soyuz.interfaces.archive import MAIN_ARCHIVE_PURPOSES
 from lp.soyuz.interfaces.packagediff import PackageDiffAlreadyRequested
 from lp.soyuz.interfaces.packagediffjob import IPackageDiffJobSource
@@ -395,21 +394,12 @@
             raise PackageDiffAlreadyRequested(
                 "%s has already been requested" % candidate.title)
 
-        if self.sourcepackagename.name == 'udev':
-            # XXX 2009-11-23 Julian bug=314436
-            # Currently diff output for udev will fill disks.  It's
-            # disabled until diffutils is fixed in that bug.
-            status = PackageDiffStatus.FAILED
-        else:
-            status = PackageDiffStatus.PENDING
-
         Store.of(to_sourcepackagerelease).flush()
         del get_property_cache(to_sourcepackagerelease).package_diffs
         packagediff = PackageDiff(
             from_source=self, to_source=to_sourcepackagerelease,
-            requester=requester, status=status)
-        if status == PackageDiffStatus.PENDING:
-            getUtility(IPackageDiffJobSource).create(packagediff)
+            requester=requester)
+        getUtility(IPackageDiffJobSource).create(packagediff)
         return packagediff
 
     def aggregate_changelog(self, since_version):

=== modified file 'lib/lp/soyuz/tests/test_packagediff.py'
--- lib/lp/soyuz/tests/test_packagediff.py	2013-07-31 00:37:32 +0000
+++ lib/lp/soyuz/tests/test_packagediff.py	2015-12-02 11:27:35 +0000
@@ -1,13 +1,18 @@
-# Copyright 2010-2013 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2015 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Test source package diffs."""
 
+from __future__ import print_function
+
 __metaclass__ = type
 
 from datetime import datetime
+import errno
 import os.path
+from textwrap import dedent
 
+from fixtures import EnvironmentVariableFixture
 import transaction
 from zope.security.proxy import removeSecurityProxy
 
@@ -23,10 +28,12 @@
 from lp.testing.layers import LaunchpadZopelessLayer
 
 
-def create_proper_job(factory):
+def create_proper_job(factory, sourcepackagename=None):
     archive = factory.makeArchive()
-    foo_dash1 = factory.makeSourcePackageRelease(archive=archive)
-    foo_dash15 = factory.makeSourcePackageRelease(archive=archive)
+    foo_dash1 = factory.makeSourcePackageRelease(
+        archive=archive, sourcepackagename=sourcepackagename)
+    foo_dash15 = factory.makeSourcePackageRelease(
+        archive=archive, sourcepackagename=sourcepackagename)
     suite_dir = 'lib/lp/archiveuploader/tests/data/suite'
     files = {
         '%s/foo_1.0-1/foo_1.0-1.diff.gz' % suite_dir: None,
@@ -156,3 +163,61 @@
         [job] = IStore(Job).find(
             Job, Job.base_job_type == JobType.GENERATE_PACKAGE_DIFF)
         self.assertIsNot(None, job)
+
+    def test_packagediff_timeout(self):
+        # debdiff is killed after the time limit expires.
+        self.pushConfig("packagediff", debdiff_timeout=1)
+        temp_dir = self.makeTemporaryDirectory()
+        mock_debdiff_path = os.path.join(temp_dir, "debdiff")
+        marker_path = os.path.join(temp_dir, "marker")
+        with open(mock_debdiff_path, "w") as mock_debdiff:
+            print(dedent("""\
+                #! /bin/sh
+                (echo "$$"; echo "$TMPDIR") >%s
+                sleep 5
+                """ % marker_path), end="", file=mock_debdiff)
+        os.chmod(mock_debdiff_path, 0o755)
+        mock_path = "%s:%s" % (temp_dir, os.environ["PATH"])
+        diff = create_proper_job(self.factory)
+        with EnvironmentVariableFixture("PATH", mock_path):
+            diff.performDiff()
+        self.assertEqual(PackageDiffStatus.FAILED, diff.status)
+        with open(marker_path) as marker:
+            debdiff_pid = int(marker.readline())
+            debdiff_tmpdir = marker.readline().rstrip("\n")
+            err = self.assertRaises(OSError, os.kill, debdiff_pid, 0)
+            self.assertEqual(errno.ESRCH, err.errno)
+            self.assertFalse(os.path.exists(debdiff_tmpdir))
+
+    def test_packagediff_max_size(self):
+        # debdiff is killed if it generates more than the size limit.
+        self.pushConfig("packagediff", debdiff_max_size=1024)
+        temp_dir = self.makeTemporaryDirectory()
+        mock_debdiff_path = os.path.join(temp_dir, "debdiff")
+        marker_path = os.path.join(temp_dir, "marker")
+        with open(mock_debdiff_path, "w") as mock_debdiff:
+            print(dedent("""\
+                #! /bin/sh
+                (echo "$$"; echo "$TMPDIR") >%s
+                yes | head -n2048 || exit 2
+                sleep 5
+                """ % marker_path), end="", file=mock_debdiff)
+        os.chmod(mock_debdiff_path, 0o755)
+        mock_path = "%s:%s" % (temp_dir, os.environ["PATH"])
+        diff = create_proper_job(self.factory)
+        with EnvironmentVariableFixture("PATH", mock_path):
+            diff.performDiff()
+        self.assertEqual(PackageDiffStatus.FAILED, diff.status)
+        with open(marker_path) as marker:
+            debdiff_pid = int(marker.readline())
+            debdiff_tmpdir = marker.readline().rstrip("\n")
+            err = self.assertRaises(OSError, os.kill, debdiff_pid, 0)
+            self.assertEqual(errno.ESRCH, err.errno)
+            self.assertFalse(os.path.exists(debdiff_tmpdir))
+
+    def test_packagediff_blacklist(self):
+        # Package diff jobs for blacklisted package names do nothing.
+        self.pushConfig("packagediff", blacklist="udev cordova-cli")
+        diff = create_proper_job(self.factory, sourcepackagename="cordova-cli")
+        diff.performDiff()
+        self.assertEqual(PackageDiffStatus.FAILED, diff.status)