run: Factor out parsing X11 displays into a helper function

This allows it to be unit-tested.

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

common/flatpak-run-private.h

@@ -200,4 +200,9 @@ gboolean flatpak_run_app (FlatpakDecomposed  *app_ref,
 
 extern const char * const *flatpak_abs_usrmerged_dirs;
 
+gboolean flatpak_run_parse_x11_display (const char  *display,
+                                        char       **x11_socket,
+                                        char       **original_display_nr,
+                                        GError     **error);
+
 #endif /* __FLATPAK_RUN_H__ */

common/flatpak-run.c

@@ -204,12 +204,54 @@ write_xauth (const char *number,
 }
 #endif /* ENABLE_XAUTH */
 
+/*
+ * @x11_socket: (out) (not optional):
+ * @display_nr_out: (out) (not optional):
+ */
+gboolean
+flatpak_run_parse_x11_display (const char  *display,
+                               char       **x11_socket,
+                               char       **display_nr_out,
+                               GError     **error)
+{
+  const char *colon;
+  const char *display_nr;
+  const char *display_nr_end;
+
+  colon = strchr (display, ':');
+
+  if (colon == NULL)
+    return glnx_throw (error, "No colon found in DISPLAY=%s", display);
+
+  if (!g_ascii_isdigit (colon[1]))
+    return glnx_throw (error, "Colon not followed by a digit in DISPLAY=%s", display);
+
+  display_nr = &colon[1];
+  display_nr_end = display_nr;
+
+  while (g_ascii_isdigit (*display_nr_end))
+    display_nr_end++;
+
+  if (display == colon)
+    {
+      *display_nr_out = g_strndup (display_nr, display_nr_end - display_nr);
+      *x11_socket = g_strdup_printf ("/tmp/.X11-unix/X%s", *display_nr_out);
+    }
+  else
+    {
+      return glnx_throw (error, "Non-local X11 address DISPLAY=%s", display);
+    }
+
+  return TRUE;
+}
+
 static void
 flatpak_run_add_x11_args (FlatpakBwrap *bwrap,
                           gboolean      allowed)
 {
   g_autofree char *x11_socket = NULL;
   const char *display;
+  g_autoptr(GError) local_error = NULL;
 
   /* Always cover /tmp/.X11-unix, that way we never see the host one in case
    * we have access to the host /tmp. If you request X access we'll put the right
@@ -245,17 +287,21 @@ flatpak_run_add_x11_args (FlatpakBwrap *bwrap,
   g_debug ("Allowing x11 access");
 
   display = g_getenv ("DISPLAY");
-  if (display && display[0] == ':' && g_ascii_isdigit (display[1]))
+
+  if (display != NULL)
     {
-      const char *display_nr = &display[1];
-      const char *display_nr_end = display_nr;
-      g_autofree char *d = NULL;
+      g_autofree char *original_display_nr = NULL;
 
-      while (g_ascii_isdigit (*display_nr_end))
-        display_nr_end++;
+      if (!flatpak_run_parse_x11_display (display, &x11_socket, &original_display_nr,
+                                          &local_error))
+        {
+          g_warning ("%s", local_error->message);
+          flatpak_bwrap_unset_env (bwrap, "DISPLAY");
+          return;
+        }
 
-      d = g_strndup (display_nr, display_nr_end - display_nr);
-      x11_socket = g_strdup_printf ("/tmp/.X11-unix/X%s", d);
+      g_assert (original_display_nr != NULL);
+      g_assert (x11_socket != NULL);
 
       flatpak_bwrap_add_args (bwrap,
                               "--ro-bind", x11_socket, "/tmp/.X11-unix/X99",
@@ -276,7 +322,7 @@ flatpak_run_add_x11_args (FlatpakBwrap *bwrap,
                 {
                   static const char dest[] = "/run/flatpak/Xauthority";
 
-                  write_xauth (d, output);
+                  write_xauth (original_display_nr, output);
                   flatpak_bwrap_add_args_data_fd (bwrap, "--ro-bind-data", tmp_fd, dest);
 
                   flatpak_bwrap_set_env (bwrap, "XAUTHORITY", dest, TRUE);

tests/testcommon.c

@@ -9,6 +9,7 @@
 #include "flatpak-utils-private.h"
 #include "flatpak-appdata-private.h"
 #include "flatpak-builtins-utils.h"
+#include "flatpak-run-private.h"
 #include "flatpak-table-printer.h"
 #include "parse-datetime.h"
 
@@ -1758,6 +1759,80 @@ test_str_is_integer (void)
   g_assert_false (flatpak_str_is_integer ("a1234"));
 }
 
+/* These are part of the X11 protocol, so we can safely hard-code them here */
+#define FamilyInternet6 6
+#define FamilyLocal 256
+#define FamilyWild 65535
+
+typedef struct
+{
+  const char *display;
+  int family;
+  const char *x11_socket;
+  const char *remote_host;
+  const char *display_number;
+} DisplayTest;
+
+static const DisplayTest x11_display_tests[] =
+{
+  /* Valid test-cases */
+  { ":0", FamilyLocal, "/tmp/.X11-unix/X0", NULL, "0" },
+  { ":0.0", FamilyLocal, "/tmp/.X11-unix/X0", NULL, "0" },
+  { ":42.0", FamilyLocal, "/tmp/.X11-unix/X42", NULL, "42" },
+  { "othermachine:23", FamilyWild, NULL, "othermachine", "23" },
+  { "bees.example.com:23", FamilyWild, NULL, "bees.example.com", "23" },
+  { "[::1]:0", FamilyInternet6, NULL, "::1", "0" },
+
+  /* Invalid test-cases */
+  { "", 0 },
+  { "nope", 0 },
+  { ":!", 0 },
+  { "othermachine::" },
+};
+
+static void
+test_parse_x11_display (void)
+{
+  gsize i;
+
+  for (i = 0; i < G_N_ELEMENTS (x11_display_tests); i++)
+    {
+      const DisplayTest *test = &x11_display_tests[i];
+      g_autofree char *x11_socket = NULL;
+      g_autofree char *remote_host = NULL;
+      g_autofree char *display_number = NULL;
+      gboolean ok;
+      g_autoptr(GError) error = NULL;
+
+      g_test_message ("%s", test->display);
+
+      ok = flatpak_run_parse_x11_display (test->display,
+                                          &x11_socket,
+                                          &display_number,
+                                          &error);
+
+      /* TODO: should be able to parse non-local addresses, too */
+      if (test->family != FamilyLocal)
+        {
+          g_assert_nonnull (error);
+          g_assert_false (ok);
+          g_assert_null (x11_socket);
+          g_assert_null (remote_host);
+          g_assert_null (display_number);
+          g_test_message ("-> could not parse: %s", error->message);
+        }
+      else
+        {
+          g_assert_no_error (error);
+          g_assert_true (ok);
+          g_assert_cmpstr (x11_socket, ==, test->x11_socket);
+          g_assert_cmpstr (remote_host, ==, test->remote_host);
+          g_assert_cmpstr (display_number, ==, test->display_number);
+          g_test_message ("-> successfully parsed");
+        }
+    }
+}
+
 int
 main (int argc, char *argv[])
 {
@@ -1790,6 +1865,7 @@ main (int argc, char *argv[])
   g_test_add_func ("/common/needs-quoting", test_needs_quoting);
   g_test_add_func ("/common/quote-argv", test_quote_argv);
   g_test_add_func ("/common/str-is-integer", test_str_is_integer);
+  g_test_add_func ("/common/parse-x11-display", test_parse_x11_display);
 
   g_test_add_func ("/app/looks-like-branch", test_looks_like_branch);
   g_test_add_func ("/app/columns", test_columns);