Merge branch '222-comm-whitespace' into 'master'

Fix whitespace and error behaviour for _dbus_command_from_pid()

Closes #222

See merge request dbus/dbus!28

Reviewed-by: pwithnall

cmake/test/CMakeLists.txt

@@ -108,6 +108,7 @@ if(DBUS_WITH_GLIB)
     add_test_executable(test-relay ${TEST_DIR}/relay.c ${TEST_LIBRARIES})
     add_test_executable(test-server-oom ${TEST_DIR}/internals/server-oom.c ${TEST_LIBRARIES})
     add_test_executable(test-syntax ${TEST_DIR}/syntax.c ${TEST_LIBRARIES})
+    add_test_executable(test-sysdeps ${TEST_DIR}/internals/sysdeps.c ${TEST_LIBRARIES})
     add_test_executable(test-syslog ${TEST_DIR}/internals/syslog.c ${TEST_LIBRARIES})
     add_test_executable(test-uid-permissions ${TEST_DIR}/uid-permissions.c ${TEST_LIBRARIES})
     add_helper_executable(manual-authz ${TEST_DIR}/manual-authz.c ${TEST_LIBRARIES})

dbus/dbus-sysdeps-util-unix.c

@@ -1092,7 +1092,22 @@ string_squash_nonprintable (DBusString *str)
   
   buf = _dbus_string_get_udata (str);
   len = _dbus_string_get_length (str);
-  
+
+  /* /proc/$pid/cmdline is a sequence of \0-terminated words, but we
+   * want a sequence of space-separated words, with no extra trailing
+   * space:
+   *     "/bin/sleep" "\0" "60" "\0"
+   *  -> "/bin/sleep" "\0" "60"
+   *  -> "/bin/sleep" " " "60"
+   *
+   * so chop off the trailing NUL before cleaning up unprintable
+   * characters. */
+  if (len > 0 && buf[len - 1] == '\0')
+    {
+      _dbus_string_shorten (str, 1);
+      len--;
+    }
+
   for (i = 0; i < len; i++)
     {
       unsigned char c = (unsigned char) buf[i];

dbus/dbus-sysdeps-util-win.c

@@ -1399,7 +1399,8 @@ _dbus_command_for_pid (unsigned long  pid,
                        int            max_len,
                        DBusError     *error)
 {
-  // FIXME
+  dbus_set_error (error, DBUS_ERROR_NOT_SUPPORTED,
+                  "_dbus_command_for_pid() not implemented on Windows");
   return FALSE;
 }
 

test/Makefile.am

@@ -66,7 +66,6 @@ TEST_BINARIES = \
 	test-segfault \
 	test-service \
 	test-shell-service \
-	test-sleep-forever \
 	$(NULL)
 
 ## These are conceptually part of directories that come earlier in SUBDIRS
@@ -109,6 +108,9 @@ test_refs_LDADD = libdbus-testutils.la $(GLIB_LIBS)
 test_server_oom_SOURCES = internals/server-oom.c
 test_server_oom_LDADD = libdbus-testutils.la $(GLIB_LIBS)
 
+test_sysdeps_SOURCES = internals/sysdeps.c
+test_sysdeps_LDADD =  libdbus-testutils.la $(GLIB_LIBS)
+
 test_syslog_SOURCES = internals/syslog.c
 test_syslog_LDADD = libdbus-testutils.la $(GLIB_LIBS)
 
@@ -137,6 +139,7 @@ nobase_testexec_PROGRAMS =
 nobase_testmeta_DATA =
 
 installable_helpers = \
+	test-sleep-forever \
 	$(NULL)
 installable_tests = \
 	test-shell \
@@ -172,6 +175,7 @@ installable_tests += \
 	test-relay \
 	test-server-oom \
 	test-syntax \
+	test-sysdeps \
 	test-syslog \
 	test-uid-permissions \
 	test-variant \
@@ -424,10 +428,18 @@ if DBUS_ENABLE_INSTALLED_TESTS
   nobase_testmeta_DATA += $(installable_test_meta)
   nobase_testmeta_DATA += $(installable_test_meta_with_config)
 else !DBUS_ENABLE_INSTALLED_TESTS
-  noinst_PROGRAMS += $(installable_tests) $(installable_manual_tests)
+  noinst_PROGRAMS += $(installable_helpers)
+  noinst_PROGRAMS += $(installable_manual_tests)
+  noinst_PROGRAMS += $(installable_tests)
 endif !DBUS_ENABLE_INSTALLED_TESTS
 
-endif DBUS_ENABLE_MODULAR_TESTS
+else !DBUS_ENABLE_MODULAR_TESTS
+
+if DBUS_ENABLE_EMBEDDED_TESTS
+  noinst_PROGRAMS += $(installable_helpers)
+endif DBUS_ENABLE_EMBEDDED_TESTS
+
+endif !DBUS_ENABLE_MODULAR_TESTS
 
 # If we're installing the tests into a DESTDIR we can't run them
 # again using the installed copy, because we don't know how to

test/internals/sysdeps.c

@@ -0,0 +1,162 @@
+/*
+ * Copyright © 2018 Collabora Ltd.
+ *
+ * Permission is hereby granted, free of charge, to any person
+ * obtaining a copy of this software and associated documentation files
+ * (the "Software"), to deal in the Software without restriction,
+ * including without limitation the rights to use, copy, modify, merge,
+ * publish, distribute, sublicense, and/or sell copies of the Software,
+ * and to permit persons to whom the Software is furnished to do so,
+ * subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be
+ * included in all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+ * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
+ * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
+ * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
+ * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+
+#include <config.h>
+
+#include <dbus/dbus.h>
+#include "dbus/dbus-sysdeps.h"
+#include "test-utils-glib.h"
+
+typedef struct
+{
+  int dummy;
+} Fixture;
+
+static void
+setup (Fixture *f G_GNUC_UNUSED,
+       gconstpointer context G_GNUC_UNUSED)
+{
+}
+
+/* In an unfortunate clash of terminology, GPid is actually a process
+ * handle (a HANDLE, vaguely analogous to a file descriptor) on Windows.
+ * For _dbus_command_for_pid() we need an actual process ID. */
+#ifdef G_OS_UNIX
+# define NO_PROCESS 0
+# define terminate(process) kill (process, SIGTERM)
+# define get_pid(process) process
+#else
+# define NO_PROCESS NULL
+# define terminate(process) TerminateProcess (process, 1)
+# define get_pid(process) GetProcessId (process)
+#endif
+
+static void
+test_command_for_pid (Fixture *f,
+                      gconstpointer context G_GNUC_UNUSED)
+{
+  gchar *argv[] = { NULL, NULL, NULL };
+  GError *error = NULL;
+  GPid process = NO_PROCESS;
+  unsigned long pid;
+  DBusError d_error = DBUS_ERROR_INIT;
+  DBusString string;
+
+  argv[0] = test_get_helper_executable ("test-sleep-forever" DBUS_EXEEXT);
+
+  if (argv[0] == NULL)
+    {
+      g_test_skip ("DBUS_TEST_EXEC not set");
+      return;
+    }
+
+  argv[1] = g_strdup ("bees");
+
+  if (!g_spawn_async (NULL, argv, NULL, G_SPAWN_DEFAULT, NULL, NULL,
+                      &process, &error))
+    g_error ("Unable to run %s: %s", argv[0], error->message);
+
+  pid = get_pid (process);
+
+  if (!_dbus_string_init (&string))
+    g_error ("out of memory");
+
+  if (_dbus_command_for_pid (pid, &string, strlen (argv[0]) + 1024, &d_error))
+    {
+      gchar *expected;
+
+      g_test_message ("Process %lu: \"%s\"", pid,
+                      _dbus_string_get_const_data (&string));
+
+      expected = g_strdup_printf ("%s %s", argv[0], argv[1]);
+
+      g_assert_cmpstr (_dbus_string_get_const_data (&string), ==,
+                       expected);
+      g_assert_cmpuint (_dbus_string_get_length (&string), ==,
+                        strlen (expected));
+      g_free (expected);
+    }
+  else
+    {
+      g_test_message ("Unable to get command for process ID: %s: %s",
+                      d_error.name, d_error.message);
+      g_assert_nonnull (d_error.name);
+      g_assert_nonnull (d_error.message);
+      dbus_error_free (&d_error);
+    }
+
+  if (!_dbus_string_set_length (&string, 0))
+    g_error ("out of memory");
+
+  /* Test truncation */
+  if (_dbus_command_for_pid (pid, &string, 10, NULL))
+    {
+      gchar *expected;
+
+      g_test_message ("Process %lu (truncated): \"%s\"", pid,
+                      _dbus_string_get_const_data (&string));
+
+      expected = g_strdup_printf ("%s %s", argv[0], argv[1]);
+      expected[10] = '\0';
+
+      g_assert_cmpstr (_dbus_string_get_const_data (&string), ==,
+                       expected);
+      g_assert_cmpuint (_dbus_string_get_length (&string), ==, 10);
+      g_free (expected);
+    }
+  else
+    {
+      g_test_message ("Unable to get command for process ID");
+    }
+
+  if (process != NO_PROCESS)
+    terminate (process);
+
+  _dbus_string_free (&string);
+  g_spawn_close_pid (process);
+  g_free (argv[1]);
+  g_free (argv[0]);
+}
+
+static void
+teardown (Fixture *f G_GNUC_UNUSED,
+          gconstpointer context G_GNUC_UNUSED)
+{
+}
+
+int
+main (int argc,
+      char **argv)
+{
+  int ret;
+
+  test_init (&argc, &argv);
+
+  g_test_add ("/sysdeps/command_for_pid",
+              Fixture, NULL, setup, test_command_for_pid, teardown);
+
+  ret = g_test_run ();
+  dbus_shutdown ();
+  return ret;
+}

test/test-utils-glib.c

@@ -863,3 +863,50 @@ test_store_result_cb (GObject *source_object G_GNUC_UNUSED,
   g_assert_null (*result_p);
   *result_p = g_object_ref (result);
 }
+
+/*
+ * Report that a test should have failed, but we are tolerating the
+ * failure because it represents a known bug or missing feature.
+ *
+ * This is the same as g_test_incomplete(), but with a workaround for
+ * GLib bug 1474 so that we don't fail tests on older GLib.
+ */
+void
+test_incomplete (const gchar *message)
+{
+  if (glib_check_version (2, 57, 3))
+    {
+      /* In GLib >= 2.57.3, g_test_incomplete() behaves as intended:
+       * the test result is reported as an expected failure and the
+       * overall test exits 0 */
+      g_test_incomplete (message);
+    }
+  else
+    {
+      /* In GLib < 2.57.3, g_test_incomplete() reported the wrong TAP
+       * result (an unexpected success) and the overall test exited 1,
+       * which would break "make check". g_test_skip() is the next
+       * best thing available. */
+      g_test_skip (message);
+    }
+}
+
+/*
+ * Return location of @exe test helper executable, or NULL if unknown.
+ *
+ * @exe must already include %DBUS_EXEEXT if appropriate.
+ *
+ * Returns: (transfer full) (nullable): an absolute path or NULL.
+ */
+gchar *
+test_get_helper_executable (const gchar *exe)
+{
+  const char *dbus_test_exec;
+
+  dbus_test_exec = _dbus_getenv ("DBUS_TEST_EXEC");
+
+  if (dbus_test_exec == NULL)
+    return NULL;
+
+  return g_build_filename (dbus_test_exec, exe, NULL);
+}

test/test-utils-glib.h

@@ -127,4 +127,8 @@ void test_store_result_cb (GObject *source_object,
                            GAsyncResult *result,
                            gpointer user_data);
 
+void test_incomplete (const gchar *message);
+
+gchar *test_get_helper_executable (const gchar *exe);
+
 #endif