diff --git a/common/flatpak-instance-private.h b/common/flatpak-instance-private.h index b4792269..0dda174b 100644 --- a/common/flatpak-instance-private.h +++ b/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__ */ diff --git a/common/flatpak-instance.c b/common/flatpak-instance.c index 7de233f3..73f155e2 100644 --- a/common/flatpak-instance.c +++ b/common/flatpak-instance.c @@ -26,6 +26,8 @@ #include "flatpak-instance-private.h" #include "flatpak-enum-types.h" +#include + /** * 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; } diff --git a/common/flatpak-run.c b/common/flatpak-run.c index f306b3cb..a7c3deb3 100644 --- a/common/flatpak-run.c +++ b/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); diff --git a/tests/test-instance.c b/tests/test-instance.c index c73369d7..2ea23e3e 100644 --- a/tests/test-instance.c +++ b/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) "/hold-lock", "--lock-file", "/.ref", + "--lock-file", + "/.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));