instance: Create and destroy per-app-ID subdirs of XRD/.flatpak

If we want to provide a per-app-ID XDG_RUNTIME_DIR (#4120) or a
per-app-ID /tmp or /dev/shm (#4093) then we'll need somewhere to put
them. Unlike $XDG_RUNTIME_DIR/app/$FLATPAK_ID, this should be somewhere
that is *not* accessible to the app, so that we can trust its contents.

Signed-off-by: Simon McVittie <smcv@collabora.com>

common/flatpak-instance-private.h

@@ -25,10 +25,16 @@
 
 FlatpakInstance *flatpak_instance_new (const char *dir);
 FlatpakInstance *flatpak_instance_new_for_id (const char *id);
+char *flatpak_instance_get_apps_directory (void);
 char *flatpak_instance_get_instances_directory (void);
 char *flatpak_instance_allocate_id (char **host_dir_out,
                                     int *lock_fd_out);
 
 void flatpak_instance_iterate_all_and_gc (GPtrArray *out_instances);
 
+gboolean flatpak_instance_ensure_per_app_dir (const char *app_id,
+                                              int *lock_fd_out,
+                                              char **lock_path_out,
+                                              GError **error);
+
 #endif /* __FLATPAK_INSTANCE_PRIVATE_H__ */

common/flatpak-instance.c

@@ -26,6 +26,8 @@
 #include "flatpak-instance-private.h"
 #include "flatpak-enum-types.h"
 
+#include <glib/gi18n-lib.h>
+
 /**
  * SECTION:flatpak-instance
  * @Title: FlatpakInstance
@@ -420,6 +422,18 @@ flatpak_instance_new (const char *dir)
   return self;
 }
 
+/*
+ * Return the directory in which we create a numbered subdirectory per
+ * instance.
+ *
+ * This directory is not shared with Flatpak apps, and we rely on this
+ * for the sandbox boundary.
+ *
+ * This is currently the same as the
+ * flatpak_instance_get_apps_directory(). We can distinguish between
+ * instance IDs and app-IDs because instances are integers, and app-IDs
+ * always contain at least one dot.
+ */
 char *
 flatpak_instance_get_instances_directory (void)
 {
@@ -428,6 +442,94 @@ flatpak_instance_get_instances_directory (void)
   return g_build_filename (user_runtime_dir, ".flatpak", NULL);
 }
 
+/*
+ * Return the directory in which we create a subdirectory per
+ * concurrently running Flatpak app-ID to store app-specific data that
+ * is common to all instances of the same app.
+ *
+ * This directory is not shared with Flatpak apps, and we rely on this
+ * for the sandbox boundary.
+ *
+ * This is currently the same as the
+ * flatpak_instance_get_instances_directory(). We can distinguish between
+ * instance IDs and app-IDs because instances are integers, and app-IDs
+ * always contain at least one dot.
+ */
+char *
+flatpak_instance_get_apps_directory (void)
+{
+  return flatpak_instance_get_instances_directory ();
+}
+
+/*
+ * @app_id: $FLATPAK_ID
+ * @lock_fd_out: (out) (not optional): Used to return a lock on the
+ *  per-app directories
+ * @lock_path_out: (out) (not optional): Used to return the path to the
+ *  lock file, suitable for bind-mounting into the container
+ *
+ * Create a per-app directory and take out a lock on it.
+ */
+gboolean
+flatpak_instance_ensure_per_app_dir (const char *app_id,
+                                     int *lock_fd_out,
+                                     char **lock_path_out,
+                                     GError **error)
+{
+  glnx_autofd int lock_fd = -1;
+  g_autofree char *lock_path = NULL;
+  g_autofree char *per_app_parent = NULL;
+  g_autofree char *per_app_dir = NULL;
+  struct flock non_exclusive_lock =
+  {
+    .l_type = F_RDLCK,
+    .l_whence = SEEK_SET,
+    .l_start = 0,
+    .l_len = 0
+  };
+
+  g_return_val_if_fail (app_id != NULL, FALSE);
+  g_return_val_if_fail (lock_fd_out != NULL, FALSE);
+  g_return_val_if_fail (*lock_fd_out == -1, FALSE);
+  g_return_val_if_fail (lock_path_out != NULL, FALSE);
+  g_return_val_if_fail (*lock_path_out == NULL, FALSE);
+  g_return_val_if_fail (error == NULL || *error == NULL, FALSE);
+
+  per_app_parent = flatpak_instance_get_apps_directory ();
+  per_app_dir = g_build_filename (per_app_parent, app_id, NULL);
+  lock_path = g_build_filename (per_app_dir, ".ref", NULL);
+
+  if (g_mkdir_with_parents (per_app_dir, 0700) != 0)
+    return glnx_throw_errno_prefix (error,
+                                    _("Unable to create directory %s"),
+                                    per_app_dir);
+
+  /* Take a file lock inside the shared directory, and hold it during
+   * setup and in bwrap. We never delete the directory itself, or the
+   * lock file that it contains (that would defeat the locking scheme).
+   * Anyone cleaning up other members of per_app_dir must first verify
+   * that it contains the lock file .ref, and take out an exclusive lock
+   * on it. */
+  lock_fd = open (lock_path, O_RDWR | O_CREAT | O_CLOEXEC, 0600);
+
+  /* As with the per-instance directories, there's a race here, because
+   * we can't atomically open and lock the lockfile. We work around
+   * that by only doing GC if the lockfile is "old".
+   *
+   * If we can't get the lock immediately, that'll be because some other
+   * process is trying to carry out garbage-collection, so we wait
+   * for it to finish. */
+  if (lock_fd < 0 ||
+      fcntl (lock_fd, F_SETLKW, &non_exclusive_lock) != 0)
+    return glnx_throw_errno_prefix (error,
+                                    _("Unable to lock %s"),
+                                    lock_path);
+
+  *lock_fd_out = glnx_steal_fd (&lock_fd);
+  *lock_path_out = g_steal_pointer (&lock_path);
+  return TRUE;
+}
+
 /*
  * @host_dir_out: (not optional): used to return the directory on the host
  *  system representing this instance
@@ -502,6 +604,102 @@ flatpak_instance_new_for_id (const char *id)
   return flatpak_instance_new (dir);
 }
 
+/*
+ * The @error is not intended to be user-facing, and is there for
+ * testing/debugging.
+ */
+static gboolean
+flatpak_instance_gc_per_app_dirs (const char *instance_id,
+                                  GError **error)
+{
+  g_autofree char *per_instance_parent = NULL;
+  g_autofree char *per_app_parent = NULL;
+  g_autofree char *app_id = NULL;
+  g_autofree char *instance_dir = NULL;
+  g_autofree char *per_app_dir = NULL;
+  glnx_autofd int per_app_dir_fd = -1;
+  glnx_autofd int per_app_dir_lock_fd = -1;
+  g_autoptr(GError) local_error = NULL;
+  g_autoptr(GKeyFile) key_file = NULL;
+  struct flock exclusive_lock =
+  {
+    .l_type = F_WRLCK,
+    .l_whence = SEEK_SET,
+    .l_start = 0,
+   .l_len = 0,
+  };
+  struct stat statbuf;
+
+  per_instance_parent = flatpak_instance_get_instances_directory ();
+  per_app_parent = flatpak_instance_get_apps_directory ();
+
+  instance_dir = g_build_filename (per_instance_parent, instance_id, NULL);
+  key_file = get_instance_info (instance_dir);
+
+  if (key_file == NULL)
+    return glnx_throw (error, "Unable to load keyfile %s/info", instance_dir);
+
+  if (g_key_file_has_group (key_file, FLATPAK_METADATA_GROUP_APPLICATION))
+    app_id = g_key_file_get_string (key_file,
+                                    FLATPAK_METADATA_GROUP_APPLICATION,
+                                    FLATPAK_METADATA_KEY_NAME, error);
+  else
+    app_id = g_key_file_get_string (key_file,
+                                    FLATPAK_METADATA_GROUP_RUNTIME,
+                                    FLATPAK_METADATA_KEY_RUNTIME, error);
+
+  if (app_id == NULL)
+    {
+      g_prefix_error (error, "%s/info: ", instance_dir);
+      return FALSE;
+    }
+
+  /* Take an exclusive lock so we don't race with other instances */
+
+  per_app_dir = g_build_filename (per_app_parent, app_id, NULL);
+  per_app_dir_fd = openat (AT_FDCWD, per_app_dir,
+                           O_PATH | O_DIRECTORY | O_CLOEXEC);
+
+  if (per_app_dir_fd < 0)
+    return glnx_throw_errno_prefix (error, "open %s", per_app_dir);
+
+  per_app_dir_lock_fd = openat (per_app_dir_fd, ".ref",
+                                O_RDWR | O_CREAT | O_CLOEXEC, 0600);
+
+  if (per_app_dir_lock_fd < 0)
+    return glnx_throw_errno_prefix (error, "open %s/.ref", per_app_dir);
+
+  /* We don't wait for the lock: we're just doing GC opportunistically.
+   * If at least one instance is running, then we'll fail to get the
+   * exclusive lock. */
+  if (fcntl (per_app_dir_lock_fd, F_SETLK, &exclusive_lock) < 0)
+    return glnx_throw_errno_prefix (error, "lock %s/.ref", per_app_dir);
+
+  if (fstat (per_app_dir_lock_fd, &statbuf) < 0)
+    return glnx_throw_errno_prefix (error, "fstat %s/.ref", per_app_dir);
+
+  /* Only gc if created at least 3 secs ago, to work around the equivalent
+   * of the race mentioned in flatpak_instance_allocate_id() */
+  if (statbuf.st_mtime + 3 >= time (NULL))
+    return glnx_throw (error, "lock file too recent, avoiding race condition");
+
+  g_debug ("Cleaning up per-app-ID state for %s", app_id);
+
+  /* We don't practically use per_app_dir for anything yet, so delete
+   * an arbitrary subdirectory in order to have something to test */
+  if (!glnx_shutil_rm_rf_at (per_app_dir_fd, "test-cleanup", NULL, &local_error))
+    {
+      g_debug ("Unable to clean up %s/test-cleanup: %s", per_app_dir,
+               local_error->message);
+      g_clear_error (&local_error);
+    }
+
+  /* Deliberately don't clean up the .ref lock file or the directory itself.
+   * If we did that, we'd defeat our locking scheme, because a concurrent
+   * process could open the .ref file just before we unlink it. */
+  return TRUE;
+}
+
 void
 flatpak_instance_iterate_all_and_gc (GPtrArray *out_instances)
 {
@@ -521,6 +719,9 @@ flatpak_instance_iterate_all_and_gc (GPtrArray *out_instances)
       if (dent == NULL)
         break;
 
+      if (!flatpak_str_is_integer (dent->d_name))
+        continue;
+
       if (dent->d_type == DT_DIR)
         {
           g_autofree char *ref_file = g_strconcat (dent->d_name, "/.ref", NULL);
@@ -542,6 +743,7 @@ flatpak_instance_iterate_all_and_gc (GPtrArray *out_instances)
             {
               /* The instance is not used, remove it */
               g_debug ("Cleaning up unused container id %s", dent->d_name);
+              flatpak_instance_gc_per_app_dirs (dent->d_name, NULL);
               glnx_shutil_rm_rf_at (iter.fd, dent->d_name, NULL, NULL);
               continue;
             }

common/flatpak-run.c

@@ -3699,6 +3699,8 @@ flatpak_run_app (FlatpakDecomposed *app_ref,
   g_autofree char *runtime_extensions = NULL;
   g_autofree char *runtime_ld_path = NULL;
   g_autofree char *checksum = NULL;
+  glnx_autofd int per_app_dir_lock_fd = -1;
+  g_autofree char *per_app_dir_lock_path = NULL;
   int ld_so_fd = -1;
   g_autoptr(GFile) runtime_ld_so_conf = NULL;
   gboolean generate_ld_so_conf = TRUE;
@@ -4092,6 +4094,15 @@ flatpak_run_app (FlatpakDecomposed *app_ref,
 
   flags |= flatpak_context_get_run_flags (app_context);
 
+  if (!sandboxed)
+    {
+      if (!flatpak_instance_ensure_per_app_dir (app_id,
+                                                &per_app_dir_lock_fd,
+                                                &per_app_dir_lock_path,
+                                                error))
+        return FALSE;
+    }
+
   if (!flatpak_run_setup_base_argv (bwrap, runtime_files, app_id_dir, app_arch, flags, error))
     return FALSE;
 
@@ -4130,6 +4141,16 @@ flatpak_run_app (FlatpakDecomposed *app_ref,
                                          &exports, cancellable, error))
     return FALSE;
 
+  if (per_app_dir_lock_path != NULL)
+    {
+      g_autofree char *dest = g_strdup_printf ("/run/user/%d/.app-ref", getuid ());
+
+      flatpak_bwrap_add_args (bwrap,
+                              "--ro-bind", per_app_dir_lock_path, dest,
+                              "--lock-file", dest,
+                              NULL);
+    }
+
   if ((app_context->shares & FLATPAK_CONTEXT_SHARED_NETWORK) != 0)
     flatpak_run_add_resolved_args (bwrap);
 
@@ -4207,6 +4228,9 @@ flatpak_run_app (FlatpakDecomposed *app_ref,
                       args, n_args, error))
     return FALSE;
 
+  /* Hold onto the lock until we execute bwrap */
+  flatpak_bwrap_add_noinherit_fd (bwrap, glnx_steal_fd (&per_app_dir_lock_fd));
+
   flatpak_bwrap_finish (bwrap);
 
   commandline = flatpak_quote_argv ((const char **) bwrap->argv->pdata, -1);

tests/test-instance.c

@@ -36,6 +36,22 @@
 
 #include "testlib.h"
 
+static void
+populate_with_files (const char *dir)
+{
+  static const char * const names[] = { "one", "two", "three" };
+  gsize i;
+
+  for (i = 0; i < G_N_ELEMENTS (names); i++)
+    {
+      g_autoptr(GError) error = NULL;
+      g_autofree char *path = g_build_filename (dir, names[i], NULL);
+
+      g_file_set_contents (path, "hello", -1, &error);
+      g_assert_no_error (error);
+    }
+}
+
 static void
 test_gc (void)
 {
@@ -43,13 +59,20 @@ test_gc (void)
   g_autoptr(GError) error = NULL;
   g_autoptr(GPtrArray) instances = NULL;
   g_autofree char *instances_dir = flatpak_instance_get_instances_directory ();
+  g_autofree char *apps_dir = flatpak_instance_get_instances_directory ();
   g_autofree char *hold_lock = g_test_build_filename (G_TEST_BUILT, "hold-lock", NULL);
+  g_autofree char *alive_app_dir = NULL;
+  g_autofree char *alive_app_lock = NULL;
+  g_autofree char *alive_app_test_cleanup = NULL;
   g_autofree char *alive_instance_dir = NULL;
   g_autofree char *alive_instance_info = NULL;
   g_autofree char *alive_instance_lock = NULL;
   g_autofree char *alive_dead_instance_dir = NULL;
   g_autofree char *alive_dead_instance_info = NULL;
   g_autofree char *alive_dead_instance_lock = NULL;
+  g_autofree char *dead_app_dir = NULL;
+  g_autofree char *dead_app_lock = NULL;
+  g_autofree char *dead_app_test_cleanup = NULL;
   g_autofree char *dead_instance_dir = NULL;
   g_autofree char *dead_instance_info = NULL;
   g_autofree char *dead_instance_lock = NULL;
@@ -59,6 +82,8 @@ test_gc (void)
     "<BUILT>/hold-lock",
     "--lock-file",
     "<instance>/.ref",
+    "--lock-file",
+    "<appID>/.ref",
     NULL
   };
   GPid pid = -1;
@@ -69,6 +94,14 @@ test_gc (void)
 
   /* com.example.Alive has one instance, #1, running.
    * A second instance, #2, was running until recently but has exited. */
+  alive_app_dir = g_build_filename (apps_dir, "com.example.Alive", NULL);
+  g_assert_no_errno (g_mkdir_with_parents (alive_app_dir, 0700));
+  alive_app_test_cleanup = g_build_filename (alive_app_dir, "test-cleanup", NULL);
+  g_assert_no_errno (g_mkdir_with_parents (alive_app_test_cleanup, 0700));
+  populate_with_files (alive_app_test_cleanup);
+  alive_app_lock = g_build_filename (alive_app_dir, ".ref", NULL);
+  g_file_set_contents (alive_app_lock, "", 0, &error);
+  g_assert_no_error (error);
 
   alive_instance_dir = g_build_filename (instances_dir, "1", NULL);
   g_assert_no_errno (g_mkdir_with_parents (alive_instance_dir, 0700));
@@ -100,6 +133,7 @@ test_gc (void)
    * by our own process. */
   hold_lock_argv[0] = hold_lock;
   hold_lock_argv[2] = alive_instance_lock;
+  hold_lock_argv[4] = alive_app_lock;
   g_spawn_async_with_pipes (NULL,
                             (gchar **) hold_lock_argv,
                             NULL,
@@ -117,6 +151,14 @@ test_gc (void)
 
   /* com.example.Dead has no instances running.
    * Instance #4 was running until recently but has exited. */
+  dead_app_dir = g_build_filename (apps_dir, "com.example.Dead", NULL);
+  g_assert_no_errno (g_mkdir_with_parents (dead_app_dir, 0700));
+  dead_app_test_cleanup = g_build_filename (dead_app_dir, "test-cleanup", NULL);
+  g_assert_no_errno (g_mkdir_with_parents (dead_app_test_cleanup, 0700));
+  populate_with_files (dead_app_test_cleanup);
+  dead_app_lock = g_build_filename (dead_app_dir, ".ref", NULL);
+  g_file_set_contents (dead_app_lock, "", 0, &error);
+  g_assert_no_error (error);
 
   dead_instance_dir = g_build_filename (instances_dir, "4", NULL);
   g_assert_no_errno (g_mkdir_with_parents (dead_instance_dir, 0700));
@@ -136,8 +178,10 @@ test_gc (void)
 
   /* Pretend the locks were created in early 1970, to bypass the workaround
    * for a race */
+  g_assert_no_errno (g_utime (alive_app_lock, &a_while_ago));
   g_assert_no_errno (g_utime (alive_instance_lock, &a_while_ago));
   g_assert_no_errno (g_utime (alive_dead_instance_lock, &a_while_ago));
+  g_assert_no_errno (g_utime (dead_app_lock, &a_while_ago));
   g_assert_no_errno (g_utime (dead_instance_lock, &a_while_ago));
 
   /* This has the side-effect of GC'ing instances */
@@ -148,6 +192,19 @@ test_gc (void)
   g_assert_cmpint (stat (alive_dead_instance_dir, &stat_buf) == 0 ? 0 : errno, ==, ENOENT);
   g_assert_cmpint (stat (dead_instance_dir, &stat_buf) == 0 ? 0 : errno, ==, ENOENT);
 
+  /* We don't GC the per-app directories themselves, or their lock files */
+  g_assert_no_errno (stat (alive_app_dir, &stat_buf));
+  g_assert_no_errno (stat (alive_app_lock, &stat_buf));
+  g_assert_no_errno (stat (dead_app_dir, &stat_buf));
+  g_assert_no_errno (stat (dead_app_lock, &stat_buf));
+
+  /* Until we implement something that actively uses this directory,
+   * use the test-cleanup subdirectory to check whether GC took place.
+   * We GC the test-cleanup subdirectory if there is no instance alive.
+   * We do not GC it if there is still an instance holding the lock. */
+  g_assert_no_errno (stat (alive_app_test_cleanup, &stat_buf));
+  g_assert_cmpint (stat (dead_app_test_cleanup, &stat_buf) == 0 ? 0 : errno, ==, ENOENT);
+
   g_assert_cmpuint (instances->len, ==, 1);
   instance = g_ptr_array_index (instances, 0);
   g_assert_true (FLATPAK_IS_INSTANCE (instance));