Instance: mitigate races, add new launch api

Avoid the 3-second 'afterlife' of instances by using
kill (pid, 0) instead of checking the file lock. We
trade pid for lock races, but this seems more reliable.

Also add flatpak_installation_launch_full that returns
a FlatpakInstance for the launched app, and lets us
avoid races around killing the app, by using a child
watch.

Closes: #2221
Approved by: matthiasclasen

10 files changed, 155 insertions, 41 deletions

diff --git a/app/flatpak-builtins-run.c b/app/flatpak-builtins-run.c
index 1ece0197..c7d6216a 100644
--- a/app/flatpak-builtins-run.c
+++ b/app/flatpak-builtins-run.c
@@ -203,6 +203,7 @@ flatpak_builtin_run (int argc, char **argv, GCancellable *cancellable, GError **
                         opt_command,
                         &argv[rest_argv_start + 1],
                         rest_argc - 1,
+                        NULL,
                         cancellable,
                         error))
     return FALSE;
diff --git a/common/flatpak-common-types-private.h b/common/flatpak-common-types-private.h
index e361777e..1a452fec 100644
--- a/common/flatpak-common-types-private.h
+++ b/common/flatpak-common-types-private.h
@@ -45,6 +45,7 @@ typedef enum {
   FLATPAK_RUN_FLAG_NO_DOCUMENTS_PORTAL = (1 << 15),
   FLATPAK_RUN_FLAG_BLUETOOTH          = (1 << 16),
   FLATPAK_RUN_FLAG_CANBUS            = (1 << 17),
+  FLATPAK_RUN_FLAG_DO_NOT_REAP        = (1 << 18),
 } FlatpakRunFlags;
 
 typedef struct FlatpakDir          FlatpakDir;
diff --git a/common/flatpak-installation.c b/common/flatpak-installation.c
index 6a0a6499..e9e03777 100644
--- a/common/flatpak-installation.c
+++ b/common/flatpak-installation.c
@@ -38,6 +38,7 @@
 #include "flatpak-enum-types.h"
 #include "flatpak-dir-private.h"
 #include "flatpak-run-private.h"
+#include "flatpak-instance-private.h"
 #include "flatpak-error.h"
 
 /**
@@ -579,9 +580,56 @@ flatpak_installation_launch (FlatpakInstallation *self,
                              GCancellable        *cancellable,
                              GError             **error)
 {
+  return flatpak_installation_launch_full (self,
+                                           FLATPAK_LAUNCH_FLAGS_NONE,
+                                           name, arch, branch, commit,
+                                           NULL,
+                                           cancellable, error);
+}
+
+/**
+ * flatpak_installation_launch_full:
+ * @self: a #FlatpakInstallation
+ * @flags: set of #FlatpakLaunchFlags
+ * @name: name of the app to launch
+ * @arch: (nullable): which architecture to launch (default: current architecture)
+ * @branch: (nullable): which branch of the application (default: "master")
+ * @commit: (nullable): the commit of @branch to launch
+ * @instance_out: (nullable): return location for a #FlatpakInstance
+ * @cancellable: (nullable): a #GCancellable
+ * @error: return location for a #GError
+ *
+ * Launch an installed application.
+ *
+ * You can use flatpak_installation_get_installed_ref() or
+ * flatpak_installation_get_current_installed_app() to find out what builds
+ * are available, in order to get a value for @commit.
+ *
+ * Compared to flatpak_installation_launch(), this function returns a #FlatpakInstance
+ * that can be used to get information about the running instance. You can also use
+ * it to wait for the instance to be done with g_child_watch_add() if you pass the
+ * #FLATPAK_LAUNCH_FLAGS_DO_NOT_REAP flag.
+ *
+ * Returns: %TRUE, unless an error occurred
+ *
+ * Since: 1.1
+ */
+gboolean
+flatpak_installation_launch_full (FlatpakInstallation *self,
+                                  FlatpakLaunchFlags   flags,
+                                  const char          *name,
+                                  const char          *arch,
+                                  const char          *branch,
+                                  const char          *commit,
+                                  FlatpakInstance    **instance_out,
+                                  GCancellable        *cancellable,
+                                  GError             **error)
+{
   g_autoptr(FlatpakDir) dir = NULL;
   g_autoptr(FlatpakDeploy) app_deploy = NULL;
   g_autofree char *app_ref = NULL;
+  g_autofree char *instance_dir = NULL;
+  FlatpakRunFlags run_flags;
 
   dir = flatpak_installation_get_dir (self, error);
   if (dir == NULL)
@@ -597,14 +645,25 @@ flatpak_installation_launch (FlatpakInstallation *self,
   if (app_deploy == NULL)
     return FALSE;
 
-  return flatpak_run_app (app_ref,
-                          app_deploy,
-                          NULL, NULL,
-                          NULL, NULL,
-                          FLATPAK_RUN_FLAG_BACKGROUND,
-                          NULL,
-                          NULL, 0,
-                          cancellable, error);
+  run_flags = FLATPAK_RUN_FLAG_BACKGROUND;
+  if (flags & FLATPAK_LAUNCH_FLAGS_DO_NOT_REAP)
+    run_flags |= FLATPAK_RUN_FLAG_DO_NOT_REAP;
+
+  if (!flatpak_run_app (app_ref,
+                        app_deploy,
+                        NULL, NULL,
+                        NULL, NULL,
+                        run_flags,
+                        NULL,
+                        NULL, 0,
+                        &instance_dir,
+                        cancellable, error))
+    return FALSE;
+
+  if (instance_out)
+    *instance_out = flatpak_instance_new (instance_dir);
+
+  return TRUE;
 }
 
 
diff --git a/common/flatpak-installation.h b/common/flatpak-installation.h
index 57ae39de..1950e71f 100644
--- a/common/flatpak-installation.h
+++ b/common/flatpak-installation.h
@@ -29,6 +29,7 @@ typedef struct _FlatpakInstallation FlatpakInstallation;
 
 #include <gio/gio.h>
 #include <flatpak-installed-ref.h>
+#include <flatpak-instance.h>
 #include <flatpak-remote.h>
 
 #define FLATPAK_TYPE_INSTALLATION flatpak_installation_get_type ()
@@ -108,6 +109,19 @@ typedef enum {
 } FlatpakUninstallFlags;
 
 /**
+ * FlatpakLaunchFlags:
+ * @FLATPAK_LAUNCH_FLAGS_NONE: Default
+ * @FLATPAK_LAUNCH_FLAGS_DO_NOT_REAP: Do not reap the child. Use this if you want to wait
+ * for the child with g_child_watch_add(). (Snce: 1.1)
+ *
+ * Flags to alter the behavior of flatpak_installation_launch_full().
+ */
+typedef enum {
+  FLATPAK_LAUNCH_FLAGS_NONE        = 0,
+  FLATPAK_LAUNCH_FLAGS_DO_NOT_REAP = (1 << 0),
+} FlatpakLaunchFlags;
+
+/**
  * FlatpakStorageType:
  * @FLATPAK_STORAGE_TYPE_DEFAULT: default
  * @FLATPAK_STORAGE_TYPE_HARD_DISK: installation is on a hard disk
@@ -184,6 +198,15 @@ FLATPAK_EXTERN gboolean             flatpak_installation_launch (FlatpakInstalla
                                                                  const char          *commit,
                                                                  GCancellable        *cancellable,
                                                                  GError             **error);
+FLATPAK_EXTERN gboolean             flatpak_installation_launch_full (FlatpakInstallation *self,
+                                                                      FlatpakLaunchFlags   flags,
+                                                                      const char          *name,
+                                                                      const char          *arch,
+                                                                      const char          *branch,
+                                                                      const char          *commit,
+                                                                      FlatpakInstance    **instance_out,
+                                                                      GCancellable        *cancellable,
+                                                                      GError             **error);
 FLATPAK_EXTERN GFileMonitor        *flatpak_installation_create_monitor (FlatpakInstallation *self,
                                                                          GCancellable        *cancellable,
                                                                          GError             **error);
diff --git a/common/flatpak-instance-private.h b/common/flatpak-instance-private.h
new file mode 100644
index 00000000..64ac6485
--- /dev/null
+++ b/common/flatpak-instance-private.h
@@ -0,0 +1,28 @@
+/*
+ * Copyright © 2018 Red Hat, Inc
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.	 See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library. If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authors:
+ *       Matthias Clasen
+ */
+
+#ifndef __FLATPAK_INSTANCE_PRIVATE_H__
+#define __FLATPAK_INSTANCE_PRIVATE_H__
+
+#include "flatpak-instance.h"
+
+FlatpakInstance *flatpak_instance_new (const char *dir);
+
+#endif /* __FLATPAK_INSTANCE_PRIVATE_H__ */
diff --git a/common/flatpak-instance.c b/common/flatpak-instance.c
index 16dc472a..54b67ad8 100644
--- a/common/flatpak-instance.c
+++ b/common/flatpak-instance.c
@@ -23,6 +23,7 @@
 #include "flatpak-utils-private.h"
 #include "flatpak-run-private.h"
 #include "flatpak-instance.h"
+#include "flatpak-instance-private.h"
 #include "flatpak-enum-types.h"
 
 /**
@@ -37,6 +38,9 @@
  * Importantly, it also gives access to the PID of the main
  * processes in the sandbox.
  *
+ * One way to obtain FlatpakInstances is to use flatpak_instance_get_all().
+ * Another way is to use flatpak_installation_launch_full().
+ *
  * Note that process lifecycle tracking is fundamentally racy.
  * You have to be prepared for the sandbox and the processes
  * represented by a FlatpakInstance to not be around anymore.
@@ -373,15 +377,14 @@ get_pid (const char *dir)
   return (int) g_ascii_strtoll (contents, NULL, 10);
 }
 
-static FlatpakInstance *
-flatpak_instance_new (const char *id)
+FlatpakInstance *
+flatpak_instance_new (const char *dir)
 {
   FlatpakInstance *self = g_object_new (flatpak_instance_get_type (), NULL);
   FlatpakInstancePrivate *priv = flatpak_instance_get_instance_private (self);
 
-  priv->id = g_strdup (id);
-
-  priv->dir = g_build_filename (g_get_user_runtime_dir (), ".flatpak", id, NULL);
+  priv->dir = g_strdup (dir);
+  priv->id = g_path_get_basename (dir);
 
   priv->pid = get_pid (priv->dir);
   priv->child_pid = get_child_pid (priv->dir);
@@ -407,6 +410,15 @@ flatpak_instance_new (const char *id)
   return self;
 }
 
+static FlatpakInstance *
+flatpak_instance_new_for_id (const char *id)
+{
+  g_autofree char *dir = NULL;
+
+  dir = g_build_filename (g_get_user_runtime_dir (), ".flatpak", id, NULL);
+  return flatpak_instance_new (dir);
+}
+
 /**
  * flatpak_instance_get_all:
  *
@@ -463,7 +475,7 @@ flatpak_instance_get_all (void)
               continue;
             }
 
-          g_ptr_array_add (instances, flatpak_instance_new (dent->d_name));
+          g_ptr_array_add (instances, flatpak_instance_new_for_id (dent->d_name));
         }
     }
 
@@ -483,32 +495,9 @@ flatpak_instance_is_running (FlatpakInstance *self)
 {
   FlatpakInstancePrivate *priv = flatpak_instance_get_instance_private (self);
 
-  if (priv->dir)
-    {
-      g_autofree char *ref_file = g_strconcat (priv->dir, "/.ref", NULL);
-      struct stat statbuf;
-      struct flock l = {
-        .l_type = F_WRLCK,
-        .l_whence = SEEK_SET,
-        .l_start = 0,
-        .l_len = 0
-      };
-      glnx_autofd int lock_fd = open (ref_file, O_RDWR | O_CLOEXEC);
-      if (lock_fd != -1 &&
-          fstat (lock_fd, &statbuf) == 0 &&
-          /* Only gc if created at least 3 secs ago, to work around race mentioned in flatpak_run_allocate_id() */
-          statbuf.st_mtime + 3 < time (NULL) &&
-          fcntl (lock_fd, F_GETLK, &l) == 0 &&
-          l.l_type == F_UNLCK)
-        {
-          /* The instance is not used, remove it */
-          g_debug ("Cleaning up unused container id %s", priv->id);
-          glnx_shutil_rm_rf_at (-1, priv->dir, NULL, NULL);
-
-          g_clear_pointer (&priv->dir, g_free);
-        }
-    }
+  if (kill (priv->pid, 0) == 0)
+    return TRUE;
 
-  return priv->dir != NULL;
+  return FALSE;
 }
 
diff --git a/common/flatpak-run-private.h b/common/flatpak-run-private.h
index 90073c51..5c7f4195 100644
--- a/common/flatpak-run-private.h
+++ b/common/flatpak-run-private.h
@@ -170,6 +170,7 @@ gboolean flatpak_run_app (const char     *app_ref,
                           const char     *custom_command,
                           char           *args[],
                           int             n_args,
+                          char          **instance_dir_out,
                           GCancellable   *cancellable,
                           GError        **error);
 
diff --git a/common/flatpak-run.c b/common/flatpak-run.c
index 637669b4..4aa6b6b5 100644
--- a/common/flatpak-run.c
+++ b/common/flatpak-run.c
@@ -2862,6 +2862,7 @@ flatpak_run_app (const char     *app_ref,
                  const char     *custom_command,
                  char           *args[],
                  int             n_args,
+                 char          **instance_dir_out,
                  GCancellable   *cancellable,
                  GError        **error)
 {
@@ -3156,11 +3157,16 @@ flatpak_run_app (const char     *app_ref,
       GPid child_pid;
       char pid_str[64];
       g_autofree char *pid_path = NULL;
+      GSpawnFlags spawn_flags;
+
+      spawn_flags = G_SPAWN_SEARCH_PATH;
+      if (flags & FLATPAK_RUN_FLAG_DO_NOT_REAP)
+        spawn_flags |= G_SPAWN_DO_NOT_REAP_CHILD;
 
       if (!g_spawn_async (NULL,
                           (char **) bwrap->argv->pdata,
                           bwrap->envp,
-                          G_SPAWN_SEARCH_PATH,
+                          spawn_flags,
                           flatpak_bwrap_child_setup_cb, bwrap->fds,
                           &child_pid,
                           error))
@@ -3190,5 +3196,8 @@ flatpak_run_app (const char     *app_ref,
       /* Not actually reached... */
     }
 
+  if (instance_dir_out)
+    *instance_dir_out = g_steal_pointer (&instance_id_host_dir);
+
   return TRUE;
 }
diff --git a/doc/reference/Makefile.am b/doc/reference/Makefile.am
index 2d619841..6b9a5471 100644
--- a/doc/reference/Makefile.am
+++ b/doc/reference/Makefile.am
@@ -33,6 +33,7 @@ IGNORE_HFILES = \
 	flatpak-installation-private.h \
 	flatpak-transaction-private.h \
 	flatpak-utils-private.h \
+	flatpak-instance-private.h \
 	$(NULL)
 
 EXTRA_HFILES =
diff --git a/doc/reference/flatpak-sections.txt b/doc/reference/flatpak-sections.txt
index f3855eb4..062766e2 100644
--- a/doc/reference/flatpak-sections.txt
+++ b/doc/reference/flatpak-sections.txt
@@ -16,6 +16,7 @@ flatpak_installation_update_full
 flatpak_installation_uninstall
 flatpak_installation_uninstall_full
 flatpak_installation_launch
+flatpak_installation_launch_full
 flatpak_installation_get_current_installed_app
 flatpak_installation_get_display_name
 flatpak_installation_get_id
@@ -56,6 +57,7 @@ FlatpakProgressCallback
 FlatpakUpdateFlags
 FlatpakInstallFlags
 FlatpakUninstallFlags
+FlatpakLaunchFlags
 FlatpakStorageType
 <SUBSECTION Standard>
 FLATPAK_INSTALLATION