flatpak-instance: Make constructing failable

If we created an instance and we failed to get the PID of the instance,
we would still succeed. If one later calls flatpak_instance_is_running
or uses the result of flatpak_instance_get_pid with kill, it's possible
to terminate the entire process group (kill 0).

Let's just error out as early as possible to avoid those weird
half-initialized cases.

That unfortunately means we have to adjust a bunch of callers as well,
but fortunately, this only affects internal API.

common/flatpak-installation.c

@@ -723,7 +723,15 @@ flatpak_installation_launch_full (FlatpakInstallation *self,
     return FALSE;
 
   if (instance_out)
-    *instance_out = flatpak_instance_new (instance_dir);
+    {
+      g_autoptr(FlatpakInstance) instance = NULL;
+
+      instance = flatpak_instance_new (instance_dir, error);
+      if (instance == NULL)
+        return FALSE;
+
+      *instance_out = g_steal_pointer (&instance);
+    }
 
   return TRUE;
 }

common/flatpak-instance-private.h

@@ -25,8 +25,10 @@
 
 #include "flatpak-instance.h"
 
-FlatpakInstance *flatpak_instance_new (const char *dir);
+FlatpakInstance *flatpak_instance_new (const char  *dir,
-FlatpakInstance *flatpak_instance_new_for_id (const char *id);
+                                       GError     **error);
+FlatpakInstance *flatpak_instance_new_for_id (const char  *id,
+                                              GError     **error);
 char *flatpak_instance_get_apps_directory (void);
 char *flatpak_instance_get_instances_directory (void);
 char *flatpak_instance_allocate_id (char **host_dir_out,

common/flatpak-instance.c

@@ -392,34 +392,47 @@ get_child_pid (const char *dir)
 }
 
 static int
-get_pid (const char *dir)
+get_pid (const char *dir,
+         GError    **error)
 {
   g_autofree char *file = NULL;
   g_autofree char *contents = NULL;
-  g_autoptr(GError) error = NULL;
+  int pid;
 
   file = g_build_filename (dir, "pid", NULL);
 
-  if (!g_file_get_contents (file, &contents, NULL, &error))
+  if (!g_file_get_contents (file, &contents, NULL, error))
+    return 0;
+
+  pid = (int) g_ascii_strtoll (contents, NULL, 10);
+  if (pid <= 0)
     {
-      g_info ("Failed to load pid file '%s': %s", file, error->message);
+      g_set_error (error, G_IO_ERROR, G_IO_ERROR_INVALID_DATA,
+                   "Invalid pid in '%s'", file);
       return 0;
     }
 
-  return (int) g_ascii_strtoll (contents, NULL, 10);
+  return pid;
 }
 
 FlatpakInstance *
-flatpak_instance_new (const char *dir)
+flatpak_instance_new (const char  *dir,
+                      GError     **error)
 {
-  FlatpakInstance *self = g_object_new (flatpak_instance_get_type (), NULL);
+  g_autoptr(FlatpakInstance) self = NULL;
-  FlatpakInstancePrivate *priv = flatpak_instance_get_instance_private (self);
+  FlatpakInstancePrivate *priv;
+
+  self = g_object_new (flatpak_instance_get_type (), NULL);
+  priv = flatpak_instance_get_instance_private (self);
 
   priv->dir = g_strdup (dir);
   priv->private_dir = g_strdup_printf ("%s-private", dir);
   priv->id = g_path_get_basename (dir);
 
-  priv->pid = get_pid (priv->dir);
+  priv->pid = get_pid (priv->dir, error);
+  if (priv->pid <= 0)
+    return NULL;
+
   priv->child_pid = get_child_pid (priv->dir);
   priv->info = get_instance_info (priv->dir);
 
@@ -448,7 +461,7 @@ flatpak_instance_new (const char *dir)
                                                     FLATPAK_METADATA_GROUP_INSTANCE, FLATPAK_METADATA_KEY_RUNTIME_COMMIT, NULL);
     }
 
-  return self;
+  return g_steal_pointer (&self);
 }
 
 /*
@@ -822,13 +835,14 @@ flatpak_instance_allocate_id (char **host_dir_out,
 }
 
 FlatpakInstance *
-flatpak_instance_new_for_id (const char *id)
+flatpak_instance_new_for_id (const char  *id,
+                             GError     **error)
 {
   g_autofree char *base_dir = flatpak_instance_get_instances_directory ();
   g_autofree char *dir = NULL;
 
   dir = g_build_filename (base_dir, id, NULL);
-  return flatpak_instance_new (dir);
+  return flatpak_instance_new (dir, error);
 }
 
 /*
@@ -1158,7 +1172,13 @@ flatpak_instance_iterate_all_and_gc (GPtrArray *out_instances)
             }
 
           if (out_instances != NULL)
-            g_ptr_array_add (out_instances, flatpak_instance_new_for_id (dent->d_name));
+            {
+              g_autoptr(FlatpakInstance) inst = NULL;
+
+              inst = flatpak_instance_new_for_id (dent->d_name, NULL);
+              if (inst)
+                g_ptr_array_add (out_instances, g_steal_pointer (&inst));
+            }
         }
     }
 }

portal/flatpak-portal.c

@@ -459,7 +459,13 @@ instance_id_read_finish (GObject      *source,
 
   data->buffer[bytes_read] = 0;
 
-  instance = flatpak_instance_new_for_id (data->buffer);
+  instance = flatpak_instance_new_for_id (data->buffer, &error);
+  if (!instance)
+    {
+      g_warning ("Failed to create instance for id %s: %s",
+                 data->buffer, error->message);
+      return;
+    }
 
   watcher_data = g_new0 (BwrapinfoWatcherData, 1);
   watcher_data->instance = g_steal_pointer (&instance);
@@ -888,23 +894,23 @@ handle_spawn (PortalFlatpak         *object,
       instance_id = g_key_file_get_string (app_info,
                                            FLATPAK_METADATA_GROUP_INSTANCE,
                                            FLATPAK_METADATA_KEY_INSTANCE_ID, NULL);
-    }
+      if (!instance_id)
+        {
+          g_dbus_method_invocation_return_error (invocation, G_DBUS_ERROR,
+                                                 G_DBUS_ERROR_INVALID_ARGS,
+                                                 "Caller has no instance id");
+          return G_DBUS_METHOD_INVOCATION_HANDLED;
+        }
 
-  if (!instance_id)
+      instance = flatpak_instance_new_for_id (instance_id, &error);
-    {
+      if (!instance)
-      g_dbus_method_invocation_return_error (invocation, G_DBUS_ERROR,
+        {
-                                             G_DBUS_ERROR_INVALID_ARGS,
+          g_dbus_method_invocation_return_error (invocation, G_DBUS_ERROR,
-                                             "Caller has no instance id");
+                                                 G_DBUS_ERROR_FAILED,
-      return G_DBUS_METHOD_INVOCATION_HANDLED;
+                                                 "Could not access caller instance: %s",
-    }
+                                                 error->message);
-
+          return G_DBUS_METHOD_INVOCATION_HANDLED;
-  instance = flatpak_instance_new_for_id (instance_id);
+        }
-  if (!instance)
-    {
-      g_dbus_method_invocation_return_error (invocation, G_DBUS_ERROR,
-                                             G_DBUS_ERROR_FAILED,
-                                             "Could not access caller instance");
-      return G_DBUS_METHOD_INVOCATION_HANDLED;
     }
 
   if ((flatpak = g_getenv ("FLATPAK_PORTAL_MOCK_FLATPAK")) != NULL)

tests/test-instance.c

@@ -150,6 +150,13 @@ test_gc (void)
   g_assert_cmpint (pid, >, 1);
   g_assert_cmpint (stdout_fd, >=, 0);
 
+  {
+    g_autofree char *pid_file = g_build_filename (alive_instance_dir, "pid", NULL);
+    g_autofree char *pid_str = g_strdup_printf ("%d", pid);
+    g_file_set_contents (pid_file, pid_str, -1, &error);
+    g_assert_no_error (error);
+  }
+
   /* 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);