Merge pull request #434 from smcv/bind-mount-diag

bind_mount: Return an error code, and provide a way to display it

4 files changed, 148 insertions, 17 deletions

diff --git a/bind-mount.c b/bind-mount.c
index e692762..4246c15 100644
--- a/bind-mount.c
+++ b/bind-mount.c
@@ -373,7 +373,7 @@ parse_mountinfo (int  proc_fd,
   return steal_pointer (&mount_tab);
 }
 
-int
+bind_mount_result
 bind_mount (int           proc_fd,
             const char   *src,
             const char   *dest,
@@ -394,18 +394,18 @@ bind_mount (int           proc_fd,
   if (src)
     {
       if (mount (src, dest, NULL, MS_SILENT | MS_BIND | (recursive ? MS_REC : 0), NULL) != 0)
-        return 1;
+        return BIND_MOUNT_ERROR_MOUNT;
     }
 
   /* The mount operation will resolve any symlinks in the destination
      path, so to find it in the mount table we need to do that too. */
   resolved_dest = realpath (dest, NULL);
   if (resolved_dest == NULL)
-    return 2;
+    return BIND_MOUNT_ERROR_REALPATH_DEST;
 
   dest_fd = open (resolved_dest, O_PATH | O_CLOEXEC);
   if (dest_fd < 0)
-    return 2;
+    return BIND_MOUNT_ERROR_REOPEN_DEST;
 
   /* If we are in a case-insensitive filesystem, mountinfo might contain a
    * different case combination of the path we requested to mount.
@@ -421,13 +421,13 @@ bind_mount (int           proc_fd,
   oldroot_dest_proc = get_oldroot_path (dest_proc);
   kernel_case_combination = readlink_malloc (oldroot_dest_proc);
   if (kernel_case_combination == NULL)
-    die_with_error ("Can't read the link in %s", oldroot_dest_proc);
+    return BIND_MOUNT_ERROR_READLINK_DEST_PROC_FD;
 
   mount_tab = parse_mountinfo (proc_fd, kernel_case_combination);
   if (mount_tab[0].mountpoint == NULL)
     {
       errno = EINVAL;
-      return 2; /* No mountpoint at dest */
+      return BIND_MOUNT_ERROR_FIND_DEST_MOUNT;
     }
 
   assert (path_equal (mount_tab[0].mountpoint, kernel_case_combination));
@@ -436,7 +436,7 @@ bind_mount (int           proc_fd,
   if (new_flags != current_flags &&
       mount ("none", resolved_dest,
              NULL, MS_SILENT | MS_BIND | MS_REMOUNT | new_flags, NULL) != 0)
-    return 3;
+    return BIND_MOUNT_ERROR_REMOUNT_DEST;
 
   /* We need to work around the fact that a bind mount does not apply the flags, so we need to manually
    * apply the flags to all submounts in the recursive case.
@@ -455,10 +455,92 @@ bind_mount (int           proc_fd,
               /* If we can't read the mountpoint we can't remount it, but that should
                  be safe to ignore because its not something the user can access. */
               if (errno != EACCES)
-                return 5;
+                return BIND_MOUNT_ERROR_REMOUNT_SUBMOUNT;
             }
         }
     }
 
-  return 0;
+  return BIND_MOUNT_SUCCESS;
+}
+
+/**
+ * Return a string representing bind_mount_result, like strerror().
+ * If want_errno_p is non-NULL, *want_errno_p is used to indicate whether
+ * it would make sense to print strerror(saved_errno).
+ */
+const char *
+bind_mount_result_to_string (bind_mount_result res,
+                             bool *want_errno_p)
+{
+  const char *string;
+  bool want_errno = TRUE;
+
+  switch (res)
+    {
+      case BIND_MOUNT_ERROR_MOUNT:
+        string = "Unable to mount source on destination";
+        break;
+
+      case BIND_MOUNT_ERROR_REALPATH_DEST:
+        string = "realpath(destination)";
+        break;
+
+      case BIND_MOUNT_ERROR_REOPEN_DEST:
+        string = "open(destination, O_PATH)";
+        break;
+
+      case BIND_MOUNT_ERROR_READLINK_DEST_PROC_FD:
+        string = "readlink(/proc/self/fd/<destination>)";
+        break;
+
+      case BIND_MOUNT_ERROR_FIND_DEST_MOUNT:
+        string = "Unable to find destination in mount table";
+        want_errno = FALSE;
+        break;
+
+      case BIND_MOUNT_ERROR_REMOUNT_DEST:
+        string = "Unable to remount destination with correct flags";
+        break;
+
+      case BIND_MOUNT_ERROR_REMOUNT_SUBMOUNT:
+        string = "Unable to remount recursively with correct flags";
+        break;
+
+      case BIND_MOUNT_SUCCESS:
+        string = "Success";
+        break;
+
+      default:
+        string = "(unknown/invalid bind_mount_result)";
+        break;
+    }
+
+  if (want_errno_p != NULL)
+    *want_errno_p = want_errno;
+
+  return string;
+}
+
+void
+die_with_bind_result (bind_mount_result res,
+                      int               saved_errno,
+                      const char       *format,
+                      ...)
+{
+  va_list args;
+  bool want_errno = TRUE;
+
+  fprintf (stderr, "bwrap: ");
+
+  va_start (args, format);
+  vfprintf (stderr, format, args);
+  va_end (args);
+
+  fprintf (stderr, ": %s", bind_mount_result_to_string (res, &want_errno));
+
+  if (want_errno)
+    fprintf (stderr, ": %s", strerror (saved_errno));
+
+  fprintf (stderr, "\n");
+  exit (1);
 }
diff --git a/bind-mount.h b/bind-mount.h
index c763763..2ec9031 100644
--- a/bind-mount.h
+++ b/bind-mount.h
@@ -18,13 +18,37 @@
 
 #pragma once
 
+#include "utils.h"
+
 typedef enum {
   BIND_READONLY = (1 << 0),
   BIND_DEVICES = (1 << 2),
   BIND_RECURSIVE = (1 << 3),
 } bind_option_t;
 
-int bind_mount (int           proc_fd,
-                const char   *src,
-                const char   *dest,
-                bind_option_t options);
+typedef enum
+{
+  BIND_MOUNT_SUCCESS = 0,
+  BIND_MOUNT_ERROR_MOUNT,
+  BIND_MOUNT_ERROR_REALPATH_DEST,
+  BIND_MOUNT_ERROR_REOPEN_DEST,
+  BIND_MOUNT_ERROR_READLINK_DEST_PROC_FD,
+  BIND_MOUNT_ERROR_FIND_DEST_MOUNT,
+  BIND_MOUNT_ERROR_REMOUNT_DEST,
+  BIND_MOUNT_ERROR_REMOUNT_SUBMOUNT,
+} bind_mount_result;
+
+bind_mount_result bind_mount (int           proc_fd,
+                              const char   *src,
+                              const char   *dest,
+                              bind_option_t options);
+
+const char *bind_mount_result_to_string (bind_mount_result res,
+                                         bool *want_errno);
+
+void die_with_bind_result (bind_mount_result res,
+                           int               saved_errno,
+                           const char       *format,
+                           ...)
+  __attribute__((__noreturn__))
+  __attribute__((format (printf, 3, 4)));
diff --git a/bubblewrap.c b/bubblewrap.c
index 7809dd8..6225330 100644
--- a/bubblewrap.c
+++ b/bubblewrap.c
@@ -942,6 +942,8 @@ privileged_op (int         privileged_op_socket,
                const char *arg1,
                const char *arg2)
 {
+  bind_mount_result bind_result;
+
   if (privileged_op_socket != -1)
     {
       uint32_t buffer[2048];  /* 8k, but is int32 to guarantee nice alignment */
@@ -1006,15 +1008,23 @@ privileged_op (int         privileged_op_socket,
       break;
 
     case PRIV_SEP_OP_REMOUNT_RO_NO_RECURSIVE:
-      if (bind_mount (proc_fd, NULL, arg2, BIND_READONLY) != 0)
-        die_with_error ("Can't remount readonly on %s", arg2);
+      bind_result = bind_mount (proc_fd, NULL, arg2, BIND_READONLY);
+
+      if (bind_result != BIND_MOUNT_SUCCESS)
+        die_with_bind_result (bind_result, errno,
+                              "Can't remount readonly on %s", arg2);
+
       break;
 
     case PRIV_SEP_OP_BIND_MOUNT:
       /* We always bind directories recursively, otherwise this would let us
          access files that are otherwise covered on the host */
-      if (bind_mount (proc_fd, arg1, arg2, BIND_RECURSIVE | flags) != 0)
-        die_with_error ("Can't bind mount %s on %s", arg1, arg2);
+      bind_result = bind_mount (proc_fd, arg1, arg2, BIND_RECURSIVE | flags);
+
+      if (bind_result != BIND_MOUNT_SUCCESS)
+        die_with_bind_result (bind_result, errno,
+                              "Can't bind mount %s on %s", arg1, arg2);
+
       break;
 
     case PRIV_SEP_OP_PROC_MOUNT:
@@ -1425,6 +1435,19 @@ resolve_symlinks_in_ops (void)
                 die_with_error("Can't find source path %s", old_source);
             }
           break;
+
+        case SETUP_MOUNT_PROC:
+        case SETUP_MOUNT_DEV:
+        case SETUP_MOUNT_TMPFS:
+        case SETUP_MOUNT_MQUEUE:
+        case SETUP_MAKE_DIR:
+        case SETUP_MAKE_FILE:
+        case SETUP_MAKE_BIND_FILE:
+        case SETUP_MAKE_RO_BIND_FILE:
+        case SETUP_MAKE_SYMLINK:
+        case SETUP_REMOUNT_RO_NO_RECURSIVE:
+        case SETUP_SET_HOSTNAME:
+        case SETUP_CHMOD:
         default:
           break;
         }
diff --git a/configure.ac b/configure.ac
index 6167ea4..3e761a0 100644
--- a/configure.ac
+++ b/configure.ac
@@ -106,6 +106,8 @@ CC_CHECK_FLAGS_APPEND([WARN_CFLAGS], [CFLAGS], [\
         -Werror=incompatible-pointer-types \
         -Werror=misleading-indentation \
 	-Werror=missing-include-dirs -Werror=aggregate-return \
+        -Werror=switch-default \
+        -Wswitch-enum \
 ])
 AC_SUBST(WARN_CFLAGS)