From 920e32fea8a4788bb45a82b8997ec232a270ff88 Mon Sep 17 00:00:00 2001
From: Simon McVittie <smcv@collabora.com>
Date: Wed, 16 Dec 2020 12:05:15 +0000
Subject: [PATCH] pressure-vessel: Move environment variables into new
 PvEnviron object

This keeps track of environment variables' values and their locked
status in one place. We build it up over time, and then serialize it
into final_argv->envp.

In particular, this is a step towards no longer storing anything in the
variable named bwrap except for bubblewrap options, which will be
a necessary piece of refactoring when we move to launching the game
as a Flatpak sub-sandbox, without ever invoking bubblewrap ourselves.

Signed-off-by: Simon McVittie <smcv@collabora.com>
---
 pressure-vessel/environ.c             | 180 ++++++++++++++++++
 pressure-vessel/environ.h             |  46 +++++
 pressure-vessel/flatpak-run-private.h |   4 +-
 pressure-vessel/flatpak-run.c         |  10 +-
 pressure-vessel/meson.build           |   2 +
 pressure-vessel/runtime.c             |  66 ++++---
 pressure-vessel/runtime.h             |   4 +-
 pressure-vessel/wrap.c                | 259 +++++++++++++++-----------
 8 files changed, 415 insertions(+), 156 deletions(-)
 create mode 100644 pressure-vessel/environ.c
 create mode 100644 pressure-vessel/environ.h

diff --git a/pressure-vessel/environ.c b/pressure-vessel/environ.c
new file mode 100644
index 000000000..4840839f2
--- /dev/null
+++ b/pressure-vessel/environ.c
@@ -0,0 +1,180 @@
+/*
+ * Copyright © 2014-2018 Red Hat, Inc
+ * Copyright © 2020 Collabora Ltd.
+ *
+ * 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/>.
+ */
+
+#include "environ.h"
+
+/*
+ * PvEnviron:
+ *
+ * Each environment variable we deal with in pressure-vessel has
+ * the following possible values:
+ *
+ * - Set to a value (empty or non-empty)
+ * - Forced to be unset
+ * - Inherited from the execution environment of bwrap(1)
+ *
+ * together with a *locked* flag. The locked flag indicates that if the
+ * container is running pressure-vessel-launcher(1), then the variable
+ * cannot be overridden by pressure-vessel-launch(1).
+ *
+ * In particular, the locked flag can be combined with any value, and
+ * a variable can also be locked to the inherited value.
+ *
+ * We represent this as follows:
+ *
+ * - Set to a value: values[VAR] = VAL
+ * - Forced to be unset: values[VAR] = NULL
+ * - Inherited from the execution environment of bwrap(1): VAR not in `values`
+ *
+ * Variables that are locked are also added to `locked`.
+ */
+struct _PvEnviron
+{
+  /* (element-type filename filename) */
+  GHashTable *values;
+  /* (element-type filename ignored) */
+  GHashTable *locked;
+};
+
+PvEnviron *
+pv_environ_new (void)
+{
+  g_autoptr(PvEnviron) self = g_slice_new0 (PvEnviron);
+
+  self->values = g_hash_table_new_full (g_str_hash, g_str_equal,
+                                        g_free, g_free);
+  self->locked = g_hash_table_new_full (g_str_hash, g_str_equal,
+                                        g_free, NULL);
+  return g_steal_pointer (&self);
+}
+
+void
+pv_environ_free (PvEnviron *self)
+{
+  g_return_if_fail (self != NULL);
+
+  g_clear_pointer (&self->values, g_hash_table_unref);
+  g_clear_pointer (&self->locked, g_hash_table_unref);
+  g_slice_free (PvEnviron, self);
+}
+
+/*
+ * Lock @var to be set to @val, which may be %NULL to unset it.
+ * It cannot be overridden by a subsequent pressure-vessel-launch
+ * invocation.
+ */
+void
+pv_environ_lock_env (PvEnviron *self,
+                     const char *var,
+                     const char *val)
+{
+  g_return_if_fail (self != NULL);
+  g_return_if_fail (var != NULL);
+  /* val may be NULL, to lock it to be unset */
+
+  g_hash_table_replace (self->values, g_strdup (var), g_strdup (val));
+  g_hash_table_add (self->locked, g_strdup (var));
+}
+
+/*
+ * Lock @var to whatever value it happens to have inherited, in a way
+ * that cannot be overridden by a subsequent pressure-vessel-launch
+ * invocation.
+ */
+void
+pv_environ_lock_inherit_env (PvEnviron *self,
+                             const char *var)
+{
+  g_return_if_fail (self != NULL);
+  g_return_if_fail (var != NULL);
+
+  g_hash_table_remove (self->values, var);
+  g_hash_table_add (self->locked, g_strdup (var));
+}
+
+/*
+ * Set @var to @val, in a way that can be overridden by a subsequent
+ * pressure-vessel-launch invocation.
+ */
+void
+pv_environ_set_env_overridable (PvEnviron *self,
+                                const char *var,
+                                const char *val)
+{
+  g_return_if_fail (self != NULL);
+  g_return_if_fail (var != NULL);
+  /* val may be NULL, to lock it to be unset */
+
+  g_hash_table_replace (self->values, g_strdup (var), g_strdup (val));
+  g_hash_table_remove (self->locked, var);
+}
+
+static int
+generic_strcmp (gconstpointer a,
+                gconstpointer b)
+{
+  return strcmp (a, b);
+}
+
+/*
+ * Returns: (transfer container): The variables that are set or forced
+ *  to be unset, but not the variables that are locked to be inherited
+ */
+GList *
+pv_environ_get_vars (PvEnviron *self)
+{
+  g_return_val_if_fail (self != NULL, NULL);
+
+  return g_list_sort (g_hash_table_get_keys (self->values), generic_strcmp);
+}
+
+/*
+ * Returns: (transfer container): The variables that are locked in some way
+ */
+GList *
+pv_environ_get_locked (PvEnviron *self)
+{
+  g_return_val_if_fail (self != NULL, NULL);
+
+  return g_list_sort (g_hash_table_get_keys (self->locked), generic_strcmp);
+}
+
+/*
+ * Returns: %TRUE if @var is locked
+ */
+gboolean
+pv_environ_is_locked (PvEnviron *self,
+                      const char *var)
+{
+  g_return_val_if_fail (self != NULL, FALSE);
+
+  return g_hash_table_contains (self->locked, var);
+}
+
+/*
+ * Returns: (nullable): The value of @var, or %NULL if @var is either
+ *  locked-to-be-unset, locked-to-be-inherited or unspecified
+ */
+const char *
+pv_environ_getenv (PvEnviron *self,
+                   const char *var)
+{
+  g_return_val_if_fail (self != NULL, NULL);
+
+  return g_hash_table_lookup (self->values, var);
+}
diff --git a/pressure-vessel/environ.h b/pressure-vessel/environ.h
new file mode 100644
index 000000000..3f3faa9c7
--- /dev/null
+++ b/pressure-vessel/environ.h
@@ -0,0 +1,46 @@
+/*
+ * Copyright © 2014-2018 Red Hat, Inc
+ * Copyright © 2020 Collabora Ltd.
+ *
+ * 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/>.
+ */
+
+#pragma once
+
+#include "libglnx/libglnx.h"
+
+#include "steam-runtime-tools/glib-backports-internal.h"
+
+typedef struct _PvEnviron PvEnviron;
+
+PvEnviron *pv_environ_new (void);
+void pv_environ_free (PvEnviron *self);
+
+void pv_environ_lock_env (PvEnviron *self,
+                          const char *var,
+                          const char *val);
+void pv_environ_lock_inherit_env (PvEnviron *self,
+                                  const char *var);
+void pv_environ_set_env_overridable (PvEnviron *self,
+                                     const char *var,
+                                     const char *val);
+
+GList *pv_environ_get_vars (PvEnviron *self);
+GList *pv_environ_get_locked (PvEnviron *self);
+const char *pv_environ_getenv (PvEnviron *self,
+                               const char *var);
+gboolean pv_environ_is_locked (PvEnviron *self,
+                               const char *var);
+
+G_DEFINE_AUTOPTR_CLEANUP_FUNC (PvEnviron, pv_environ_free)
diff --git a/pressure-vessel/flatpak-run-private.h b/pressure-vessel/flatpak-run-private.h
index b9a7ac2ec..a5931a61e 100644
--- a/pressure-vessel/flatpak-run-private.h
+++ b/pressure-vessel/flatpak-run-private.h
@@ -39,11 +39,9 @@
 #include "steam-runtime-tools/glib-backports-internal.h"
 
 void flatpak_run_add_x11_args (FlatpakBwrap *bwrap,
-                               GHashTable   *extra_locked_vars_to_unset,
                                gboolean      allowed);
 gboolean flatpak_run_add_wayland_args (FlatpakBwrap *bwrap);
-void flatpak_run_add_pulseaudio_args (FlatpakBwrap *bwrap,
-                                      GHashTable   *extra_locked_vars_to_unset);
+void flatpak_run_add_pulseaudio_args (FlatpakBwrap *bwrap);
 gboolean flatpak_run_add_system_dbus_args (FlatpakBwrap *app_bwrap);
 gboolean flatpak_run_add_session_dbus_args (FlatpakBwrap *app_bwrap);
 void     flatpak_run_apply_env_appid (FlatpakBwrap *bwrap,
diff --git a/pressure-vessel/flatpak-run.c b/pressure-vessel/flatpak-run.c
index 5b244f725..9714e56b0 100644
--- a/pressure-vessel/flatpak-run.c
+++ b/pressure-vessel/flatpak-run.c
@@ -160,7 +160,6 @@ write_xauth (char *number, FILE *output)
 
 void
 flatpak_run_add_x11_args (FlatpakBwrap *bwrap,
-                          GHashTable   *extra_locked_vars_to_unset,
                           gboolean      allowed)
 {
   g_autofree char *x11_socket = NULL;
@@ -176,7 +175,7 @@ flatpak_run_add_x11_args (FlatpakBwrap *bwrap,
 
   if (!allowed)
     {
-      g_hash_table_add (extra_locked_vars_to_unset, g_strdup ("DISPLAY"));
+      flatpak_bwrap_unset_env (bwrap, "DISPLAY");
       return;
     }
 
@@ -230,7 +229,7 @@ flatpak_run_add_x11_args (FlatpakBwrap *bwrap,
     }
   else
     {
-      g_hash_table_add (extra_locked_vars_to_unset, g_strdup ("DISPLAY"));
+      flatpak_bwrap_unset_env (bwrap, "DISPLAY");
     }
 }
 
@@ -534,8 +533,7 @@ flatpak_run_parse_pulse_server (const char *value)
 }
 
 void
-flatpak_run_add_pulseaudio_args (FlatpakBwrap *bwrap,
-                                 GHashTable   *extra_locked_vars_to_unset)
+flatpak_run_add_pulseaudio_args (FlatpakBwrap *bwrap)
 {
   g_autofree char *pulseaudio_server = flatpak_run_get_pulseaudio_server ();
   g_autofree char *pulseaudio_socket = NULL;
@@ -547,7 +545,7 @@ flatpak_run_add_pulseaudio_args (FlatpakBwrap *bwrap,
   if (!pulseaudio_socket)
     pulseaudio_socket = g_build_filename (user_runtime_dir, "pulse/native", NULL);
 
-  g_hash_table_add (extra_locked_vars_to_unset, g_strdup ("PULSE_SERVER"));
+  flatpak_bwrap_unset_env (bwrap, "PULSE_SERVER");
 
   /* SteamOS system-wide PulseAudio instance */
   if (!g_file_test (pulseaudio_socket, G_FILE_TEST_EXISTS))
diff --git a/pressure-vessel/meson.build b/pressure-vessel/meson.build
index ed93e41b7..2534a40c9 100644
--- a/pressure-vessel/meson.build
+++ b/pressure-vessel/meson.build
@@ -191,6 +191,8 @@ executable(
   sources : [
     'bwrap.c',
     'bwrap.h',
+    'environ.c',
+    'environ.h',
     'exports.c',
     'exports.h',
     'flatpak-common-types-private.h',
diff --git a/pressure-vessel/runtime.c b/pressure-vessel/runtime.c
index 64be4c2ba..57248775f 100644
--- a/pressure-vessel/runtime.c
+++ b/pressure-vessel/runtime.c
@@ -350,11 +350,10 @@ G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC (RuntimeArchitecture,
 
 static gboolean pv_runtime_use_provider_graphics_stack (PvRuntime *self,
                                                         FlatpakBwrap *bwrap,
-                                                        GHashTable *extra_locked_vars_to_unset,
-                                                        GHashTable *extra_locked_vars_to_inherit,
+                                                        PvEnviron *container_env,
                                                         GError **error);
 static void pv_runtime_set_search_paths (PvRuntime *self,
-                                         FlatpakBwrap *bwrap);
+                                         PvEnviron *container_env);
 
 static void
 pv_runtime_init (PvRuntime *self)
@@ -1626,8 +1625,7 @@ static gboolean
 bind_runtime (PvRuntime *self,
               FlatpakExports *exports,
               FlatpakBwrap *bwrap,
-              GHashTable *extra_locked_vars_to_unset,
-              GHashTable *extra_locked_vars_to_inherit,
+              PvEnviron *container_env,
               GError **error)
 {
   static const char * const bind_mutable[] =
@@ -1693,7 +1691,7 @@ bind_runtime (PvRuntime *self,
                           "--symlink", "../run", "/var/run",
                           NULL);
 
-  flatpak_bwrap_set_env (bwrap, "XDG_RUNTIME_DIR", xrd, TRUE);
+  pv_environ_lock_env (container_env, "XDG_RUNTIME_DIR", xrd);
 
   if (g_strcmp0 (self->provider_in_host_namespace, "/") != 0
       || g_strcmp0 (self->provider_in_container_namespace, "/run/host") != 0)
@@ -1811,8 +1809,8 @@ bind_runtime (PvRuntime *self,
 
   if (self->flags & PV_RUNTIME_FLAGS_PROVIDER_GRAPHICS_STACK)
     {
-      if (!pv_runtime_use_provider_graphics_stack (self, bwrap, extra_locked_vars_to_unset,
-                                                   extra_locked_vars_to_inherit, error))
+      if (!pv_runtime_use_provider_graphics_stack (self, bwrap, container_env,
+                                                   error))
         return FALSE;
     }
 
@@ -3169,8 +3167,7 @@ pv_runtime_finish_libc_family (PvRuntime *self,
 static gboolean
 pv_runtime_use_provider_graphics_stack (PvRuntime *self,
                                         FlatpakBwrap *bwrap,
-                                        GHashTable *extra_locked_vars_to_unset,
-                                        GHashTable *extra_locked_vars_to_inherit,
+                                        PvEnviron *container_env,
                                         GError **error)
 {
   gsize i, j;
@@ -3211,6 +3208,7 @@ pv_runtime_use_provider_graphics_stack (PvRuntime *self,
 
   g_return_val_if_fail (PV_IS_RUNTIME (self), FALSE);
   g_return_val_if_fail (!pv_bwrap_was_finished (bwrap), FALSE);
+  g_return_val_if_fail (container_env != NULL, FALSE);
   g_return_val_if_fail (error == NULL || *error == NULL, FALSE);
 
   if (!pv_runtime_provide_container_access (self, error))
@@ -3654,21 +3652,23 @@ pv_runtime_use_provider_graphics_stack (PvRuntime *self,
     }
 
   if (dri_path->len != 0)
-    flatpak_bwrap_set_env (bwrap, "LIBGL_DRIVERS_PATH", dri_path->str, TRUE);
+    pv_environ_lock_env (container_env, "LIBGL_DRIVERS_PATH", dri_path->str);
   else
-    g_hash_table_add (extra_locked_vars_to_unset, g_strdup ("LIBGL_DRIVERS_PATH"));
+    pv_environ_lock_env (container_env, "LIBGL_DRIVERS_PATH", NULL);
 
   if (egl_path->len != 0)
-    flatpak_bwrap_set_env (bwrap, "__EGL_VENDOR_LIBRARY_FILENAMES", egl_path->str, TRUE);
+    pv_environ_lock_env (container_env, "__EGL_VENDOR_LIBRARY_FILENAMES",
+                         egl_path->str);
   else
-    g_hash_table_add (extra_locked_vars_to_unset, g_strdup ("__EGL_VENDOR_LIBRARY_FILENAMES"));
+    pv_environ_lock_env (container_env, "__EGL_VENDOR_LIBRARY_FILENAMES",
+                         NULL);
 
-  g_hash_table_add (extra_locked_vars_to_unset, g_strdup ("__EGL_VENDOR_LIBRARY_DIRS"));
+  pv_environ_lock_env (container_env, "__EGL_VENDOR_LIBRARY_DIRS", NULL);
 
   if (vulkan_path->len != 0)
-    flatpak_bwrap_set_env (bwrap, "VK_ICD_FILENAMES", vulkan_path->str, TRUE);
+    pv_environ_lock_env (container_env, "VK_ICD_FILENAMES", vulkan_path->str);
   else
-    g_hash_table_add (extra_locked_vars_to_unset, g_strdup ("VK_ICD_FILENAMES"));
+    pv_environ_lock_env (container_env, "VK_ICD_FILENAMES", NULL);
 
   if (self->flags & PV_RUNTIME_FLAGS_IMPORT_VULKAN_LAYERS)
     {
@@ -3690,15 +3690,17 @@ pv_runtime_use_provider_graphics_stack (PvRuntime *self,
           else
             prepended_data_dirs = g_steal_pointer (&override_share);
 
-          flatpak_bwrap_set_env (bwrap, "XDG_DATA_DIRS", prepended_data_dirs, TRUE);
+          pv_environ_lock_env (container_env, "XDG_DATA_DIRS",
+                               prepended_data_dirs);
         }
-      g_hash_table_add (extra_locked_vars_to_unset, g_strdup ("VK_LAYER_PATH"));
+      pv_environ_lock_env (container_env, "VK_LAYER_PATH", NULL);
     }
 
   if (va_api_path->len != 0)
-    flatpak_bwrap_set_env (bwrap, "LIBVA_DRIVERS_PATH", va_api_path->str, TRUE);
+    pv_environ_lock_env (container_env, "LIBVA_DRIVERS_PATH",
+                         va_api_path->str);
   else
-    g_hash_table_add (extra_locked_vars_to_unset, g_strdup ("LIBVA_DRIVERS_PATH"));
+    pv_environ_lock_env (container_env, "LIBVA_DRIVERS_PATH", NULL);
 
   /* We binded the VDPAU drivers in "%{libdir}/vdpau".
    * Unfortunately VDPAU_DRIVER_PATH can hold just a single path, so we can't
@@ -3709,8 +3711,7 @@ pv_runtime_use_provider_graphics_stack (PvRuntime *self,
   g_autofree gchar *vdpau_val = g_strdup_printf ("%s/lib/platform-${PLATFORM}/vdpau",
                                                  self->overrides_in_container);
 
-  flatpak_bwrap_set_env (bwrap, "VDPAU_DRIVER_PATH", vdpau_val, TRUE);
-
+  pv_environ_lock_env (container_env, "VDPAU_DRIVER_PATH", vdpau_val);
   return TRUE;
 }
 
@@ -3718,8 +3719,7 @@ gboolean
 pv_runtime_bind (PvRuntime *self,
                  FlatpakExports *exports,
                  FlatpakBwrap *bwrap,
-                 GHashTable *extra_locked_vars_to_unset,
-                 GHashTable *extra_locked_vars_to_inherit,
+                 PvEnviron *container_env,
                  GError **error)
 {
   g_autofree gchar *pressure_vessel_prefix = NULL;
@@ -3727,15 +3727,13 @@ pv_runtime_bind (PvRuntime *self,
   g_return_val_if_fail (PV_IS_RUNTIME (self), FALSE);
   g_return_val_if_fail (exports != NULL, FALSE);
   g_return_val_if_fail (!pv_bwrap_was_finished (bwrap), FALSE);
-  g_return_val_if_fail (extra_locked_vars_to_unset != NULL, FALSE);
-  g_return_val_if_fail (extra_locked_vars_to_inherit != NULL, FALSE);
+  g_return_val_if_fail (container_env != NULL, FALSE);
   g_return_val_if_fail (error == NULL || *error == NULL, FALSE);
 
   if (!bind_runtime (self,
                      exports,
                      bwrap,
-                     extra_locked_vars_to_unset,
-                     extra_locked_vars_to_inherit,
+                     container_env,
                      error))
     return FALSE;
 
@@ -3792,16 +3790,16 @@ pv_runtime_bind (PvRuntime *self,
    * backwards compatibility is not a concern for game developers who
    * have specifically opted-in to using the newer runtime. */
   if (self->is_scout)
-    flatpak_bwrap_set_env (bwrap, "STEAM_RUNTIME", "/", TRUE);
+    pv_environ_lock_env (container_env, "STEAM_RUNTIME", "/");
 
-  pv_runtime_set_search_paths (self, bwrap);
+  pv_runtime_set_search_paths (self, container_env);
 
   return TRUE;
 }
 
 void
 pv_runtime_set_search_paths (PvRuntime *self,
-                             FlatpakBwrap *bwrap)
+                             PvEnviron *container_env)
 {
   g_autoptr(GString) ld_library_path = g_string_new ("");
   gsize i;
@@ -3824,8 +3822,8 @@ pv_runtime_set_search_paths (PvRuntime *self,
 
   /* The PATH from outside the container doesn't really make sense inside the
    * container: in principle the layout could be totally different. */
-  flatpak_bwrap_set_env (bwrap, "PATH", "/usr/bin:/bin", TRUE);
-  flatpak_bwrap_set_env (bwrap, "LD_LIBRARY_PATH", ld_library_path->str, TRUE);
+  pv_environ_lock_env (container_env, "PATH", "/usr/bin:/bin");
+  pv_environ_lock_env (container_env, "LD_LIBRARY_PATH", ld_library_path->str);
 }
 
 static void
diff --git a/pressure-vessel/runtime.h b/pressure-vessel/runtime.h
index 1d4d4ea33..4163d6c97 100644
--- a/pressure-vessel/runtime.h
+++ b/pressure-vessel/runtime.h
@@ -26,6 +26,7 @@
 #include "steam-runtime-tools/glib-backports-internal.h"
 #include "libglnx/libglnx.h"
 
+#include "environ.h"
 #include "flatpak-bwrap-private.h"
 #include "flatpak-exports-private.h"
 #include "flatpak-utils-base-private.h"
@@ -85,8 +86,7 @@ gboolean pv_runtime_get_adverb (PvRuntime *self,
 gboolean pv_runtime_bind (PvRuntime *self,
                           FlatpakExports *exports,
                           FlatpakBwrap *bwrap,
-                          GHashTable *extra_locked_vars_to_unset,
-                          GHashTable *extra_locked_vars_to_inherit,
+                          PvEnviron *container_env,
                           GError **error);
 void pv_runtime_cleanup (PvRuntime *self);
 
diff --git a/pressure-vessel/wrap.c b/pressure-vessel/wrap.c
index 24da9c776..46d2c7162 100644
--- a/pressure-vessel/wrap.c
+++ b/pressure-vessel/wrap.c
@@ -36,6 +36,7 @@
 
 #include "bwrap.h"
 #include "bwrap-lock.h"
+#include "environ.h"
 #include "flatpak-bwrap-private.h"
 #include "flatpak-run-private.h"
 #include "flatpak-utils-base-private.h"
@@ -385,7 +386,7 @@ static const EnvMount known_required_env[] =
 
 static void
 bind_and_propagate_from_environ (FlatpakExports *exports,
-                                 FlatpakBwrap *bwrap,
+                                 PvEnviron *container_env,
                                  const char *variable,
                                  EnvMountFlags flags)
 {
@@ -461,7 +462,7 @@ bind_and_propagate_from_environ (FlatpakExports *exports,
     {
       g_autofree gchar *joined = g_strjoinv (":", values);
 
-      flatpak_bwrap_set_env (bwrap, variable, joined, TRUE);
+      pv_environ_lock_env (container_env, variable, joined);
     }
 }
 
@@ -482,6 +483,7 @@ static gboolean expose_steam (FlatpakExports *exports,
 static gboolean
 use_fake_home (FlatpakExports *exports,
                FlatpakBwrap *bwrap,
+               PvEnviron *container_env,
                const gchar *fake_home,
                GError **error)
 {
@@ -542,9 +544,9 @@ use_fake_home (FlatpakExports *exports,
                           "--bind", tmp, "/var/tmp",
                           NULL);
 
-  flatpak_bwrap_set_env (bwrap, "XDG_CACHE_HOME", cache, TRUE);
-  flatpak_bwrap_set_env (bwrap, "XDG_CONFIG_HOME", config, TRUE);
-  flatpak_bwrap_set_env (bwrap, "XDG_DATA_HOME", data, TRUE);
+  pv_environ_lock_env (container_env, "XDG_CACHE_HOME", cache);
+  pv_environ_lock_env (container_env, "XDG_CONFIG_HOME", config);
+  pv_environ_lock_env (container_env, "XDG_DATA_HOME", data);
 
   flatpak_exports_add_path_expose (exports,
                                    FLATPAK_FILESYSTEM_MODE_READ_WRITE,
@@ -1211,6 +1213,7 @@ main (int argc,
   g_auto(GStrv) original_environ = NULL;
   int original_argc = argc;
   gboolean is_flatpak_env = g_file_test ("/.flatpak-info", G_FILE_TEST_IS_REGULAR);
+  g_autoptr(PvEnviron) container_env = NULL;
   g_autoptr(FlatpakBwrap) bwrap = NULL;
   g_autoptr(FlatpakBwrap) argv_in_container = NULL;
   g_autoptr(FlatpakBwrap) final_argv = NULL;
@@ -1225,17 +1228,8 @@ main (int argc,
   g_autofree gchar *tools_dir = NULL;
   g_autoptr(PvRuntime) runtime = NULL;
   g_autoptr(FILE) original_stdout = NULL;
-  GHashTableIter iter;
-  const gchar *env_var;
   g_autoptr(GString) lock_env = g_string_new ("");
   g_auto(GLnxTmpfile) lock_env_tmpf  = { 0, };
-  gpointer k;
-  g_autoptr(GHashTable) extra_locked_vars_to_unset = g_hash_table_new_full (g_str_hash,
-                                                                            g_str_equal,
-                                                                            g_free, NULL);
-  g_autoptr(GHashTable) extra_locked_vars_to_inherit = g_hash_table_new_full (g_str_hash,
-                                                                            g_str_equal,
-                                                                            g_free, NULL);
   g_autofree char *lock_env_fd = NULL;
   g_autoptr(GArray) pass_fds_through_adverb = g_array_new (FALSE, FALSE, sizeof (int));
 
@@ -1626,6 +1620,7 @@ main (int argc,
   bwrap = flatpak_bwrap_new (flatpak_bwrap_empty_env);
   flatpak_bwrap_add_arg (bwrap, bwrap_executable);
   exports = flatpak_exports_new ();
+  container_env = pv_environ_new ();
 
   if (is_flatpak_env)
     {
@@ -1699,8 +1694,7 @@ main (int argc,
       if (!pv_runtime_bind (runtime,
                             exports,
                             bwrap,
-                            extra_locked_vars_to_unset,
-                            extra_locked_vars_to_inherit,
+                            container_env,
                             error))
         goto out;
     }
@@ -1779,7 +1773,8 @@ main (int argc,
     }
   else
     {
-      if (!use_fake_home (exports, bwrap, opt_fake_home, error))
+      if (!use_fake_home (exports, bwrap, container_env, opt_fake_home,
+                          error))
         goto out;
     }
 
@@ -1862,11 +1857,12 @@ main (int argc,
     }
 
   if (adjusted_ld_preload->len != 0)
-    flatpak_bwrap_set_env (bwrap, "LD_PRELOAD", adjusted_ld_preload->str, TRUE);
+    pv_environ_set_env_overridable (container_env, "LD_PRELOAD",
+                                    adjusted_ld_preload->str);
 
   g_debug ("Making Steam environment variables available if required...");
   for (i = 0; i < G_N_ELEMENTS (known_required_env); i++)
-    bind_and_propagate_from_environ (exports, bwrap,
+    bind_and_propagate_from_environ (exports, container_env,
                                      known_required_env[i].name,
                                      known_required_env[i].flags);
 
@@ -1920,7 +1916,7 @@ main (int argc,
                           "--chdir", cwd_p_host,
                           NULL);
 
-  g_hash_table_add (extra_locked_vars_to_unset, g_strdup ("PWD"));
+  pv_environ_lock_env (container_env, "PWD", NULL);
 
   /* Put Steam Runtime environment variables back, if /usr is mounted
    * from the host. */
@@ -1945,7 +1941,8 @@ main (int argc,
 
               *equals = '\0';
 
-              flatpak_bwrap_set_env (bwrap, opt_env_if_host[i], equals + 1, TRUE);
+              pv_environ_lock_env (container_env, opt_env_if_host[i],
+                                   equals + 1);
 
               *equals = '=';
             }
@@ -1968,6 +1965,16 @@ main (int argc,
     {
       g_autoptr(FlatpakBwrap) sharing_bwrap =
         flatpak_bwrap_new (flatpak_bwrap_empty_env);
+      g_auto(GStrv) envp = NULL;
+
+      /* If these are set by flatpak_run_add_x11_args(), etc., we'll
+       * change them from locked-and-unset to locked-and-set later.
+       * Every variable that is unset with flatpak_bwrap_unset_env() in
+       * the functions we borrow from Flatpak (below) should be listed
+       * here. */
+      pv_environ_lock_env (container_env, "DISPLAY", NULL);
+      pv_environ_lock_env (container_env, "PULSE_SERVER", NULL);
+      pv_environ_lock_env (container_env, "XAUTHORITY", NULL);
 
       flatpak_run_add_font_path_args (sharing_bwrap);
 
@@ -1991,132 +1998,159 @@ main (int argc,
             }
           else
             {
-              flatpak_run_add_x11_args (sharing_bwrap, extra_locked_vars_to_unset, TRUE);
+              flatpak_run_add_x11_args (sharing_bwrap, TRUE);
             }
 
-          flatpak_run_add_pulseaudio_args (sharing_bwrap, extra_locked_vars_to_unset);
+          flatpak_run_add_pulseaudio_args (sharing_bwrap);
           flatpak_run_add_session_dbus_args (sharing_bwrap);
           flatpak_run_add_system_dbus_args (sharing_bwrap);
         }
 
+      envp = pv_bwrap_steal_envp (sharing_bwrap);
+
+      for (i = 0; envp[i] != NULL; i++)
+        {
+          static const char * const known_vars[] =
+          {
+            "DBUS_SESSION_BUS_ADDRESS",
+            "DBUS_SYSTEM_BUS_ADDRESS",
+            "DISPLAY",
+            "PULSE_CLIENTCONFIG",
+            "PULSE_SERVER",
+            "XAUTHORITY",
+          };
+          char *equals = strchr (envp[i], '=');
+          const char *var = envp[i];
+          const char *val = NULL;
+          gsize j;
+
+          if (equals != NULL)
+            {
+              *equals = '\0';
+              val = equals + 1;
+            }
+
+          for (j = 0; j < G_N_ELEMENTS (known_vars); j++)
+            {
+              if (strcmp (var, known_vars[j]) == 0)
+                break;
+            }
+
+          /* If this warning is reached, we might need to add this
+           * variable to the block of
+           * pv_environ_lock_env (container_env, ., NULL) calls above */
+          if (j >= G_N_ELEMENTS (known_vars))
+            g_warning ("Extra environment variable %s set during container "
+                       "setup but not in known_vars; check logic",
+                       var);
+
+          pv_environ_lock_env (container_env, var, val);
+        }
+
+      g_warn_if_fail (g_strv_length (sharing_bwrap->envp) == 0);
       flatpak_bwrap_append_bwrap (bwrap, sharing_bwrap);
     }
 
   if (is_flatpak_env)
     {
+      g_autoptr(GList) vars = NULL;
+      const GList *iter;
+
       /* These are the environment variables that will be wrong, or useless,
-       * in the new container that will be created. */
-      g_hash_table_add (extra_locked_vars_to_unset, g_strdup ("FLATPAK_ID"));
-      g_hash_table_add (extra_locked_vars_to_unset, g_strdup ("FLATPAK_SANDBOX_DIR"));
-      g_hash_table_add (extra_locked_vars_to_unset, g_strdup ("LD_AUDIT"));
+       * in the new container that will be created by escaping from the
+       * sandbox. Force them to be unset. */
+      pv_environ_lock_env (container_env, "FLATPAK_ID", NULL);
+      pv_environ_lock_env (container_env, "FLATPAK_SANDBOX_DIR", NULL);
+      pv_environ_lock_env (container_env, "LD_AUDIT", NULL);
 
       /* These are the environment variables that might differ in the host
        * system. However from inside a container we are not able to know the
        * host's value. So we allow them to inherit the value from the host. */
-      g_hash_table_add (extra_locked_vars_to_inherit, g_strdup ("DBUS_SESSION_BUS_ADDRESS"));
-      g_hash_table_add (extra_locked_vars_to_inherit, g_strdup ("DBUS_SYSTEM_BUS_ADDRESS"));
-      g_hash_table_add (extra_locked_vars_to_inherit, g_strdup ("DISPLAY"));
-      g_hash_table_add (extra_locked_vars_to_inherit, g_strdup ("XDG_RUNTIME_DIR"));
+      pv_environ_lock_inherit_env (container_env, "DBUS_SESSION_BUS_ADDRESS");
+      pv_environ_lock_inherit_env (container_env, "DBUS_SYSTEM_BUS_ADDRESS");
+      pv_environ_lock_inherit_env (container_env, "DISPLAY");
+      pv_environ_lock_inherit_env (container_env, "XDG_RUNTIME_DIR");
 
       /* The bwrap envp will be completely ignored when calling
        * pv-launch, and in fact putting them in its environment
        * variables would be wrong, because pv-launch needs to see the
        * current execution environment's DBUS_SESSION_BUS_ADDRESS
        * (if different). For this reason we convert them to `--setenv`. */
-      for (i = 0; bwrap->envp != NULL && bwrap->envp[i] != NULL; i++)
-        {
-          g_auto(GStrv) split = g_strsplit (bwrap->envp[i], "=", 2);
+      vars = pv_environ_get_vars (container_env);
 
-          g_assert (split != NULL);
-          g_assert (split[0] != NULL);
-          g_assert (split[1] != NULL);
+      for (iter = vars; iter != NULL; iter = iter->next)
+        {
+          const char *var = iter->data;
+          const char *val = pv_environ_getenv (container_env, var);
 
-          flatpak_bwrap_add_args (bwrap,
-                                  "--setenv", split[0], split[1],
-                                  NULL);
+          if (val != NULL)
+            flatpak_bwrap_add_args (bwrap,
+                                    "--setenv", var, val,
+                                    NULL);
+          else
+            flatpak_bwrap_add_args (bwrap,
+                                    "--unsetenv", var,
+                                    NULL);
         }
     }
 
-  for (i = 0; bwrap->envp != NULL && bwrap->envp[i] != NULL; i++)
-    {
-      /* List only the variables names and not their values */
-      char *equals = strchr (bwrap->envp[i], '=');
-
-      g_assert (equals != NULL);
-
-      /* Never lock LD_PRELOAD, otherwise, for example, we might miss the
-       * overlay renderer library that the Steam client adds to LD_PRELOAD */
-      if (g_str_has_prefix (bwrap->envp[i], "LD_PRELOAD="))
-        continue;
-
-      *equals = '\0';
+  final_argv = flatpak_bwrap_new (original_environ);
 
-      g_string_append (lock_env, bwrap->envp[i]);
-
-      *equals = '=';
-
-      g_string_append_c (lock_env, '\0');
-    }
-
-  g_hash_table_iter_init (&iter, extra_locked_vars_to_unset);
-  while (g_hash_table_iter_next (&iter, &k, NULL))
+  /* Lock variables where appropriate */
     {
-      env_var = k;
-      g_string_append (lock_env, env_var);
-      g_string_append_c (lock_env, '\0');
+      g_autoptr(GList) vars = pv_environ_get_locked (container_env);
+      const GList *iter;
 
-      flatpak_bwrap_add_args (bwrap,
-                              "--unsetenv", env_var,
-                              NULL);
-    }
+      for (iter = vars; iter != NULL; iter = iter->next)
+        {
+          const char *var = iter->data;
 
-  g_hash_table_iter_init (&iter, extra_locked_vars_to_inherit);
-  while (g_hash_table_iter_next (&iter, &k, NULL))
-    {
-      /* To inherit the variable value, we just use the lock
-       * mechanism without `--setenv` or '--unsetenv'. In this way, when we
-       * execute `launcher` with `--host`, it will inherit the value from the
-       * host system. */
-      env_var = k;
-      g_string_append (lock_env, env_var);
-      g_string_append_c (lock_env, '\0');
+          g_debug ("Locking environment variable: %s", var);
+          g_string_append (lock_env, var);
+          g_string_append_c (lock_env, '\0');
+        }
     }
 
-  /* Populate the bwrap environ with the variables that are not in the
-   * `extra_locked_vars_to_unset` list or `extra_locked_vars_to_inherit` list,
-   * and without overriding the values that are already present in
-   * bwrap->envp.
+  /* Populate final_argv->envp, overwriting its copy of original_environ.
    * We skip this if we are in a Flatpak environment, because in that case
    * we already used `--setenv` for all the variables that we care about and
-   * the bwrap envp will be ignored anyway. */
+   * the final_argv->envp will be ignored anyway, other than as a way to
+   * invoke pv-launch (for which original_environ is appropriate). */
   if (!is_flatpak_env)
     {
-      for (i = 0; original_environ[i] != NULL; i++)
+      g_autoptr(GList) vars = NULL;
+      const GList *iter;
+
+      vars = pv_environ_get_vars (container_env);
+
+      for (iter = vars; iter != NULL; iter = iter->next)
         {
-          char *eq = strchr (original_environ[i], '=');
-          if (eq)
-            {
-              g_autofree gchar *key = g_strndup (original_environ[i],
-                                                 eq - original_environ[i]);
-              if (!g_hash_table_contains (extra_locked_vars_to_unset, key)
-                  && !g_hash_table_contains (extra_locked_vars_to_inherit, key))
-                flatpak_bwrap_set_env (bwrap, key, eq + 1, FALSE);
-            }
+          const char *var = iter->data;
+          const char *val = pv_environ_getenv (container_env, var);
+
+          if (val != NULL)
+            flatpak_bwrap_set_env (final_argv, var, val, TRUE);
+          else
+            flatpak_bwrap_unset_env (final_argv, var);
         }
 
       /* The setuid bwrap will filter out some of the environment variables,
-       * so we have to go via --setenv */
+       * so we still have to go via --setenv for these. */
       for (i = 0; unsecure_environment_variables[i] != NULL; i++)
         {
-          const gchar *val = g_environ_getenv (bwrap->envp, unsecure_environment_variables[i]);
+          const char *var = unsecure_environment_variables[i];
+          const gchar *val = pv_environ_getenv (container_env, var);
+
           if (val != NULL)
-            {
-              flatpak_bwrap_add_args (bwrap, "--setenv",
-                                      unsecure_environment_variables[i], val, NULL);
-            }
+            flatpak_bwrap_add_args (bwrap, "--setenv", var, val, NULL);
         }
     }
 
+  /* Now that we've populated final_argv->envp, it's too late to change
+   * any environment variables. Make sure we get an assertion failure
+   * if we try. */
+  g_clear_pointer (&container_env, pv_environ_free);
+
   if (!flatpak_buffer_to_sealed_memfd_or_tmpfile (&lock_env_tmpf, "lock-env",
                                                   lock_env->str, lock_env->len,
                                                   error))
@@ -2297,6 +2331,7 @@ main (int argc,
 
       flatpak_bwrap_add_arg (adverb_argv, "--");
 
+      g_warn_if_fail (g_strv_length (adverb_argv->envp) == 0);
       flatpak_bwrap_append_bwrap (argv_in_container, adverb_argv);
     }
 
@@ -2321,6 +2356,7 @@ main (int argc,
        * passed to the launcher */
       flatpak_bwrap_append_argsv (launcher_argv, &argv[1], argc - 1);
 
+      g_warn_if_fail (g_strv_length (launcher_argv->envp) == 0);
       flatpak_bwrap_append_bwrap (argv_in_container, launcher_argv);
     }
   else
@@ -2333,18 +2369,16 @@ main (int argc,
       flatpak_bwrap_append_argsv (argv_in_container, &argv[1], argc - 1);
     }
 
+  g_warn_if_fail (g_strv_length (argv_in_container->envp) == 0);
   flatpak_bwrap_append_bwrap (bwrap, argv_in_container);
 
-  final_argv = flatpak_bwrap_new (flatpak_bwrap_empty_env);
+  g_warn_if_fail (g_strv_length (bwrap->envp) == 0);
 
   if (is_flatpak_env)
     {
-      /* Use pv-launch to launch bwrap on the host. This needs to run
-       * with the environment variables from the current execution
-       * environment (not the container that we are setting up!)
-       * so that it gets the right DBUS_SESSION_BUS_ADDRESS, if
-       * different. */
-      g_autoptr(FlatpakBwrap) launch_on_host = flatpak_bwrap_new (original_environ);
+      /* Use pv-launch to launch bwrap on the host. */
+      g_autoptr(FlatpakBwrap) launch_on_host =
+        flatpak_bwrap_new (flatpak_bwrap_empty_env);
       flatpak_bwrap_add_arg (launch_on_host, launch_executable);
       flatpak_bwrap_add_arg (launch_on_host, "--bus-name=org.freedesktop.Flatpak");
 
@@ -2361,15 +2395,18 @@ main (int argc,
 
       flatpak_bwrap_add_arg (launch_on_host, "--");
 
-      /* Don't use the environment variables from @bwrap any more.
-       * We already converted them to --setenv and locked them. */
-      g_strfreev (pv_bwrap_steal_envp (bwrap));
-
+      g_warn_if_fail (g_strv_length (launch_on_host->envp) == 0);
       flatpak_bwrap_append_bwrap (final_argv, launch_on_host);
     }
 
   flatpak_bwrap_append_bwrap (final_argv, bwrap);
 
+  /* We'll have permuted the order anyway, so we might as well sort it,
+   * to make debugging a bit easier. */
+  if (final_argv->envp != NULL)
+    qsort (final_argv->envp, g_strv_length (final_argv->envp),
+           sizeof (char *), pv_envp_cmp);
+
   if (opt_verbose)
     {
       g_message ("Final command to execute:");
-- 
GitLab