Merge branch 'richardmaw/distinguish-sandboxing-build-fail' into 'master'

Distinguish between bubblewrap sandboxing failure and command failure

Closes #286

See merge request BuildStream/buildstream!868

8 files changed, 136 insertions, 42 deletions

diff --git a/buildstream/_platform/linux.py b/buildstream/_platform/linux.py
index afdf81c79..33f3966c1 100644
--- a/buildstream/_platform/linux.py
+++ b/buildstream/_platform/linux.py
@@ -18,9 +18,9 @@
 #        Tristan Maat <tristan.maat@codethink.co.uk>
 
 import os
-import shutil
 import subprocess
 
+from .. import _site
 from .. import utils
 from ..sandbox import SandboxDummy
 
@@ -38,16 +38,18 @@ class Linux(Platform):
 
         self._have_fuse = os.path.exists("/dev/fuse")
 
-        bwrap_version = self._get_bwrap_version()
+        bwrap_version = _site.get_bwrap_version()
 
         if bwrap_version is None:
             self._bwrap_exists = False
             self._have_good_bwrap = False
             self._die_with_parent_available = False
+            self._json_status_available = False
         else:
             self._bwrap_exists = True
             self._have_good_bwrap = (0, 1, 2) <= bwrap_version
             self._die_with_parent_available = (0, 1, 8) <= bwrap_version
+            self._json_status_available = (0, 3, 2) <= bwrap_version
 
         self._local_sandbox_available = self._have_fuse and self._have_good_bwrap
 
@@ -97,6 +99,7 @@ class Linux(Platform):
         # Inform the bubblewrap sandbox as to whether it can use user namespaces or not
         kwargs['user_ns_available'] = self._user_ns_available
         kwargs['die_with_parent_available'] = self._die_with_parent_available
+        kwargs['json_status_available'] = self._json_status_available
         return SandboxBwrap(*args, **kwargs)
 
     def _check_user_ns_available(self):
@@ -119,21 +122,3 @@ class Linux(Platform):
             output = ''
 
         return output == 'root'
-
-    def _get_bwrap_version(self):
-        # Get the current bwrap version
-        #
-        # returns None if no bwrap was found
-        # otherwise returns a tuple of 3 int: major, minor, patch
-        bwrap_path = shutil.which('bwrap')
-
-        if not bwrap_path:
-            return None
-
-        cmd = [bwrap_path, "--version"]
-        try:
-            version = str(subprocess.check_output(cmd).split()[1], "utf-8")
-        except subprocess.CalledProcessError:
-            return None
-
-        return tuple(int(x) for x in version.split("."))
diff --git a/buildstream/_site.py b/buildstream/_site.py
index d64390b5d..8940fa34a 100644
--- a/buildstream/_site.py
+++ b/buildstream/_site.py
@@ -18,6 +18,8 @@
 #        Tristan Van Berkom <tristan.vanberkom@codethink.co.uk>
 
 import os
+import shutil
+import subprocess
 
 #
 # Private module declaring some info about where the buildstream
@@ -44,3 +46,22 @@ build_all_template = os.path.join(root, 'data', 'build-all.sh.in')
 
 # Module building script template
 build_module_template = os.path.join(root, 'data', 'build-module.sh.in')
+
+
+def get_bwrap_version():
+    # Get the current bwrap version
+    #
+    # returns None if no bwrap was found
+    # otherwise returns a tuple of 3 int: major, minor, patch
+    bwrap_path = shutil.which('bwrap')
+
+    if not bwrap_path:
+        return None
+
+    cmd = [bwrap_path, "--version"]
+    try:
+        version = str(subprocess.check_output(cmd).split()[1], "utf-8")
+    except subprocess.CalledProcessError:
+        return None
+
+    return tuple(int(x) for x in version.split("."))
diff --git a/buildstream/sandbox/_sandboxbwrap.py b/buildstream/sandbox/_sandboxbwrap.py
index 066932f5c..839780f95 100644
--- a/buildstream/sandbox/_sandboxbwrap.py
+++ b/buildstream/sandbox/_sandboxbwrap.py
@@ -17,6 +17,8 @@
 #  Authors:
 #        Andrew Leeming <andrew.leeming@codethink.co.uk>
 #        Tristan Van Berkom <tristan.vanberkom@codethink.co.uk>
+import collections
+import json
 import os
 import sys
 import time
@@ -24,7 +26,8 @@ import errno
 import signal
 import subprocess
 import shutil
-from contextlib import ExitStack
+from contextlib import ExitStack, suppress
+from tempfile import TemporaryFile
 
 import psutil
 
@@ -53,6 +56,7 @@ class SandboxBwrap(Sandbox):
         super().__init__(*args, **kwargs)
         self.user_ns_available = kwargs['user_ns_available']
         self.die_with_parent_available = kwargs['die_with_parent_available']
+        self.json_status_available = kwargs['json_status_available']
 
     def run(self, command, flags, *, cwd=None, env=None):
         stdout, stderr = self._get_output()
@@ -160,24 +164,31 @@ class SandboxBwrap(Sandbox):
                 gid = self._get_config().build_gid
                 bwrap_command += ['--uid', str(uid), '--gid', str(gid)]
 
-        # Add the command
-        bwrap_command += command
-
-        # bwrap might create some directories while being suid
-        # and may give them to root gid, if it does, we'll want
-        # to clean them up after, so record what we already had
-        # there just in case so that we can safely cleanup the debris.
-        #
-        existing_basedirs = {
-            directory: os.path.exists(os.path.join(root_directory, directory))
-            for directory in ['tmp', 'dev', 'proc']
-        }
-
-        # Use the MountMap context manager to ensure that any redirected
-        # mounts through fuse layers are in context and ready for bwrap
-        # to mount them from.
-        #
         with ExitStack() as stack:
+            pass_fds = ()
+            # Improve error reporting with json-status if available
+            if self.json_status_available:
+                json_status_file = stack.enter_context(TemporaryFile())
+                pass_fds = (json_status_file.fileno(),)
+                bwrap_command += ['--json-status-fd', str(json_status_file.fileno())]
+
+            # Add the command
+            bwrap_command += command
+
+            # bwrap might create some directories while being suid
+            # and may give them to root gid, if it does, we'll want
+            # to clean them up after, so record what we already had
+            # there just in case so that we can safely cleanup the debris.
+            #
+            existing_basedirs = {
+                directory: os.path.exists(os.path.join(root_directory, directory))
+                for directory in ['tmp', 'dev', 'proc']
+            }
+
+            # Use the MountMap context manager to ensure that any redirected
+            # mounts through fuse layers are in context and ready for bwrap
+            # to mount them from.
+            #
             stack.enter_context(mount_map.mounted(self))
 
             # If we're interactive, we want to inherit our stdin,
@@ -190,7 +201,7 @@ class SandboxBwrap(Sandbox):
 
             # Run bubblewrap !
             exit_code = self.run_bwrap(bwrap_command, stdin, stdout, stderr,
-                                       (flags & SandboxFlags.INTERACTIVE))
+                                       (flags & SandboxFlags.INTERACTIVE), pass_fds)
 
             # Cleanup things which bwrap might have left behind, while
             # everything is still mounted because bwrap can be creating
@@ -238,10 +249,27 @@ class SandboxBwrap(Sandbox):
                         # a bug, bwrap mounted a tempfs here and when it exits, that better be empty.
                         pass
 
+            if self.json_status_available:
+                json_status_file.seek(0, 0)
+                child_exit_code = None
+                # The JSON status file's output is a JSON object per line
+                # with the keys present identifying the type of message.
+                # The only message relevant to us now is the exit-code of the subprocess.
+                for line in json_status_file:
+                    with suppress(json.decoder.JSONDecodeError):
+                        o = json.loads(line)
+                        if isinstance(o, collections.abc.Mapping) and 'exit-code' in o:
+                            child_exit_code = o['exit-code']
+                            break
+                if child_exit_code is None:
+                    raise SandboxError("`bwrap' terminated during sandbox setup with exitcode {}".format(exit_code),
+                                       reason="bwrap-sandbox-fail")
+                exit_code = child_exit_code
+
         self._vdir._mark_changed()
         return exit_code
 
-    def run_bwrap(self, argv, stdin, stdout, stderr, interactive):
+    def run_bwrap(self, argv, stdin, stdout, stderr, interactive, pass_fds):
         # Wrapper around subprocess.Popen() with common settings.
         #
         # This function blocks until the subprocess has terminated.
@@ -317,6 +345,7 @@ class SandboxBwrap(Sandbox):
                 # The default is to share file descriptors from the parent process
                 # to the subprocess, which is rarely good for sandboxing.
                 close_fds=True,
+                pass_fds=pass_fds,
                 stdin=stdin,
                 stdout=stdout,
                 stderr=stderr,
diff --git a/tests/integration/project/elements/sandbox-bwrap/break-shell.bst b/tests/integration/project/elements/sandbox-bwrap/break-shell.bst
new file mode 100644
index 000000000..c93a92350
--- /dev/null
+++ b/tests/integration/project/elements/sandbox-bwrap/break-shell.bst
@@ -0,0 +1,9 @@
+kind: manual
+depends:
+  - base/base-alpine.bst
+
+public:
+  bst:
+    integration-commands:
+    - |
+      chmod a-x /bin/sh
diff --git a/tests/integration/project/elements/sandbox-bwrap/command-exit-42.bst b/tests/integration/project/elements/sandbox-bwrap/command-exit-42.bst
new file mode 100644
index 000000000..c633334ae
--- /dev/null
+++ b/tests/integration/project/elements/sandbox-bwrap/command-exit-42.bst
@@ -0,0 +1,8 @@
+kind: manual
+depends:
+  - base/base-alpine.bst
+
+config:
+  build-commands:
+  - |
+    exit 42
diff --git a/tests/integration/project/elements/sandbox-bwrap/non-executable-shell.bst b/tests/integration/project/elements/sandbox-bwrap/non-executable-shell.bst
new file mode 100644
index 000000000..a57177bb3
--- /dev/null
+++ b/tests/integration/project/elements/sandbox-bwrap/non-executable-shell.bst
@@ -0,0 +1,9 @@
+kind: manual
+
+depends:
+  - sandbox-bwrap/break-shell.bst
+
+config:
+  build-commands:
+  - |
+    exit 42
diff --git a/tests/integration/sandbox-bwrap.py b/tests/integration/sandbox-bwrap.py
index 7d2a18498..d2484bc17 100644
--- a/tests/integration/sandbox-bwrap.py
+++ b/tests/integration/sandbox-bwrap.py
@@ -1,9 +1,11 @@
 import os
 import pytest
 
+from buildstream._exceptions import ErrorDomain
+
 from tests.testutils import cli_integration as cli
 from tests.testutils.integration import assert_contains
-from tests.testutils.site import HAVE_BWRAP
+from tests.testutils.site import HAVE_BWRAP, HAVE_BWRAP_JSON_STATUS
 
 
 pytestmark = pytest.mark.integration
@@ -29,3 +31,32 @@ def test_sandbox_bwrap_cleanup_build(cli, tmpdir, datafiles):
     # Here, BuildStream should not attempt any rmdir etc.
     result = cli.run(project=project, args=['build', element_name])
     assert result.exit_code == 0
+
+
+@pytest.mark.skipif(not HAVE_BWRAP, reason='Only available with bubblewrap')
+@pytest.mark.skipif(not HAVE_BWRAP_JSON_STATUS, reason='Only available with bubblewrap supporting --json-status-fd')
+@pytest.mark.datafiles(DATA_DIR)
+def test_sandbox_bwrap_distinguish_setup_error(cli, tmpdir, datafiles):
+    project = os.path.join(datafiles.dirname, datafiles.basename)
+    element_name = 'sandbox-bwrap/non-executable-shell.bst'
+
+    result = cli.run(project=project, args=['build', element_name])
+    result.assert_task_error(error_domain=ErrorDomain.SANDBOX, error_reason="bwrap-sandbox-fail")
+
+
+@pytest.mark.integration
+@pytest.mark.skipif(not HAVE_BWRAP, reason='Only available with bubblewrap')
+@pytest.mark.datafiles(DATA_DIR)
+def test_sandbox_bwrap_return_subprocess(cli, tmpdir, datafiles):
+    project = os.path.join(datafiles.dirname, datafiles.basename)
+    element_name = 'sandbox-bwrap/command-exit-42.bst'
+
+    cli.configure({
+        "logging": {
+            "message-format": "%{element}|%{message}",
+        },
+    })
+
+    result = cli.run(project=project, args=['build', element_name])
+    result.assert_task_error(error_domain=ErrorDomain.ELEMENT, error_reason=None)
+    assert "sandbox-bwrap/command-exit-42.bst|Command 'exit 42' failed with exitcode 42" in result.stderr
diff --git a/tests/testutils/site.py b/tests/testutils/site.py
index 6801be471..6c286e720 100644
--- a/tests/testutils/site.py
+++ b/tests/testutils/site.py
@@ -4,7 +4,7 @@
 import os
 import sys
 
-from buildstream import utils, ProgramNotFoundError
+from buildstream import _site, utils, ProgramNotFoundError
 
 try:
     utils.get_host_tool('bzr')
@@ -33,8 +33,10 @@ except (ImportError, ValueError):
 try:
     utils.get_host_tool('bwrap')
     HAVE_BWRAP = True
+    HAVE_BWRAP_JSON_STATUS = _site.get_bwrap_version() >= (0, 3, 2)
 except ProgramNotFoundError:
     HAVE_BWRAP = False
+    HAVE_BWRAP_JSON_STATUS = False
 
 try:
     utils.get_host_tool('lzip')