launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad-buildd:image-targets-fix into launchpad-buildd:master.

Commit message:
Fix environment variable quoting in chroot backend

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1884936 in launchpad-buildd: "chroot backend failing to build multiple image-targets"
  https://bugs.launchpad.net/launchpad-buildd/+bug/1884936

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad-buildd/+git/launchpad-buildd/+merge/387221

The arguments to "env" are assembled as an argv list, so they shouldn't be internally-quoted.

Reported by Dimitri John Ledkov.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad-buildd:image-targets-fix into launchpad-buildd:master.

diff --git a/debian/changelog b/debian/changelog
index 9e45fea..3530cff 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -30,6 +30,7 @@ launchpad-buildd (190) UNRELEASED; urgency=medium
   * lpbuildd/binarypackage.py: Use "apt-get indextargets" and "apt-helper
     cat-file" where they exist to read Packages files, rather than looking
     in /var/lib/apt/lists/ directly.
+  * Fix environment variable quoting in chroot backend (LP: #1884936).
 
   [ Dimitri John Ledkov ]
   * lxd: Add riscv64 to arch table.
diff --git a/lpbuildd/target/chroot.py b/lpbuildd/target/chroot.py
index 203142c..436e21b 100644
--- a/lpbuildd/target/chroot.py
+++ b/lpbuildd/target/chroot.py
@@ -62,8 +62,7 @@ class Chroot(Backend):
         """See `Backend`."""
         if env:
             args = ["env"] + [
-                "%s=%s" % (key, shell_escape(value))
-                for key, value in env.items()] + args
+                "%s=%s" % (key, value) for key, value in env.items()] + args
         if self.arch is not None:
             args = set_personality(args, self.arch, series=self.series)
         if cwd is not None:
diff --git a/lpbuildd/target/tests/test_chroot.py b/lpbuildd/target/tests/test_chroot.py
index 88e3137..8004fa9 100644
--- a/lpbuildd/target/tests/test_chroot.py
+++ b/lpbuildd/target/tests/test_chroot.py
@@ -139,6 +139,22 @@ class TestChroot(TestCase):
             expected_args,
             [proc._args["args"] for proc in processes_fixture.procs])
 
+    def test_run_env_shell_metacharacters(self):
+        self.useFixture(EnvironmentVariable("HOME", "/expected/home"))
+        processes_fixture = self.useFixture(FakeProcesses())
+        processes_fixture.add(lambda _: {}, name="sudo")
+        Chroot("1", "xenial", "amd64").run(
+            ["echo", "hello"], env={"OBJECT": "{'foo': 'bar'}"})
+
+        expected_args = [
+            ["sudo", "/usr/sbin/chroot",
+             "/expected/home/build-1/chroot-autobuild",
+             "linux64", "env", "OBJECT={'foo': 'bar'}", "echo", "hello"],
+            ]
+        self.assertEqual(
+            expected_args,
+            [proc._args["args"] for proc in processes_fixture.procs])
+
     def test_copy_in(self):
         self.useFixture(EnvironmentVariable("HOME", "/expected/home"))
         source_dir = self.useFixture(TempDir()).path
diff --git a/lpbuildd/target/tests/test_lxd.py b/lpbuildd/target/tests/test_lxd.py
index 145630b..38d3337 100644
--- a/lpbuildd/target/tests/test_lxd.py
+++ b/lpbuildd/target/tests/test_lxd.py
@@ -649,6 +649,21 @@ class TestLXD(TestCase):
             expected_args,
             [proc._args["args"] for proc in processes_fixture.procs])
 
+    def test_run_env_shell_metacharacters(self):
+        processes_fixture = self.useFixture(FakeProcesses())
+        processes_fixture.add(lambda _: {}, name="lxc")
+        LXD("1", "xenial", "amd64").run(
+            ["echo", "hello"], env={"OBJECT": "{'foo': 'bar'}"})
+
+        expected_args = [
+            ["lxc", "exec", "lp-xenial-amd64",
+             "--env", "OBJECT={'foo': 'bar'}", "--",
+             "linux64", "echo", "hello"],
+            ]
+        self.assertEqual(
+            expected_args,
+            [proc._args["args"] for proc in processes_fixture.procs])
+
     def test_copy_in(self):
         source_dir = self.useFixture(TempDir()).path
         self.useFixture(MockPatch("pylxd.Client"))