launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/limit-debdiff into lp:launchpad.

Commit message:
Kill debdiff after ten minutes by default, and make sure we clean up after it properly.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #314436 in Launchpad itself: "package-diff can generate infinite output"
  https://bugs.launchpad.net/launchpad/+bug/314436

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/limit-debdiff/+merge/278187

Kill debdiff after ten minutes by default, and make sure we clean up after it properly.

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.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/limit-debdiff into lp:launchpad.

=== 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-11-20 18:05:16 +0000
@@ -541,6 +541,10 @@
 # The maximum number of lines to format using the format_diff tal formatter.
 max_format_lines: 5000
 
+# The timeout in seconds for a debdiff between two packages.
+# datatype: integer
+debdiff_time_limit: 600
+
 
 [distributionmirrorprober]
 # The database user which will be used by this process.

=== 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-11-20 18:05:16 +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,12 @@
     'PackageDiffSet',
     ]
 
+from functools import partial
 import gzip
 import itertools
 import os
 import shutil
+import signal
 import subprocess
 import tempfile
 
@@ -20,6 +22,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
@@ -67,13 +70,17 @@
     [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(signal.alarm, config.diff.debdiff_time_limit),
+            cwd=tmp_dir, env=env)
         stdout, stderr = process.communicate()
     finally:
         if out_file is not None:

=== 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-11-20 18:05:16 +0000
@@ -1,13 +1,16 @@
-# 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 os.path
 
+from fixtures import EnvironmentVariableFixture
 import transaction
 from zope.security.proxy import removeSecurityProxy
 
@@ -156,3 +159,18 @@
         [job] = IStore(Job).find(
             Job, Job.base_job_type == JobType.GENERATE_PACKAGE_DIFF)
         self.assertIsNot(None, job)
+
+    def test_packagediff_time_limit(self):
+        # debdiff is killed after the time limit expires.
+        self.pushConfig("diff", debdiff_time_limit=1)
+        temp_dir = self.makeTemporaryDirectory()
+        mock_debdiff_path = os.path.join(temp_dir, "debdiff")
+        with open(mock_debdiff_path, "w") as mock_debdiff:
+            print("#! /bin/sh", file=mock_debdiff)
+            print("sleep 5", 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)