From 168bbe146c301d4ca515d8e8ac57c1eda2fa06c7 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Wed, 31 Oct 2018 15:09:24 +0000 Subject: [PATCH 1/9] tests: Build installable helpers whenever any tests are enabled We might need these to run tests at build-time, so we should build them whenever either modular or embedded tests are enabled, even if installed-tests aren't. We haven't noticed this bug until now because $(installable_helpers) only contained test-apparmor-activation, which isn't normally needed at build-time because the AppArmor test can only work when run as root. Signed-off-by: Simon McVittie --- test/Makefile.am | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/test/Makefile.am b/test/Makefile.am index 399c3370..539acb6c 100644 --- a/test/Makefile.am +++ b/test/Makefile.am @@ -424,10 +424,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 From d30188a135c12e030b3ca3f1c9c34daca2161725 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Thu, 25 Oct 2018 16:42:21 +0100 Subject: [PATCH 2/9] tests: Build test-sleep-forever even if embedded tests are disabled It will be used as a long-running subprocess to test _dbus_command_for_pid(). Signed-off-by: Simon McVittie --- test/Makefile.am | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/Makefile.am b/test/Makefile.am index 539acb6c..5f99b689 100644 --- a/test/Makefile.am +++ b/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 @@ -137,6 +136,7 @@ nobase_testexec_PROGRAMS = nobase_testmeta_DATA = installable_helpers = \ + test-sleep-forever \ $(NULL) installable_tests = \ test-shell \ From 2a2c6e6790a25e96210115ef04b21c332fb4effa Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Wed, 31 Oct 2018 15:19:19 +0000 Subject: [PATCH 3/9] test_incomplete: Add function This is a wrapper for g_test_incomplete(), which works around bugs in that function prior to GLib 2.57.3. I originally wrote it for librsvg. Signed-off-by: Simon McVittie --- test/test-utils-glib.c | 27 +++++++++++++++++++++++++++ test/test-utils-glib.h | 2 ++ 2 files changed, 29 insertions(+) diff --git a/test/test-utils-glib.c b/test/test-utils-glib.c index 3b447fe8..930b87df 100644 --- a/test/test-utils-glib.c +++ b/test/test-utils-glib.c @@ -863,3 +863,30 @@ 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); + } +} diff --git a/test/test-utils-glib.h b/test/test-utils-glib.h index d36f4301..7cfc8785 100644 --- a/test/test-utils-glib.h +++ b/test/test-utils-glib.h @@ -127,4 +127,6 @@ void test_store_result_cb (GObject *source_object, GAsyncResult *result, gpointer user_data); +void test_incomplete (const gchar *message); + #endif From 6eb1c2cd53d6f38f769964b3e4f509a4ef957ab3 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Wed, 31 Oct 2018 15:22:26 +0000 Subject: [PATCH 4/9] test_get_helper_executable: Add function This is basically the same as get_test_exec() in dbus-spawn-test.c, but GLib-flavoured. Signed-off-by: Simon McVittie --- test/test-utils-glib.c | 20 ++++++++++++++++++++ test/test-utils-glib.h | 2 ++ 2 files changed, 22 insertions(+) diff --git a/test/test-utils-glib.c b/test/test-utils-glib.c index 930b87df..04d98aa5 100644 --- a/test/test-utils-glib.c +++ b/test/test-utils-glib.c @@ -890,3 +890,23 @@ test_incomplete (const gchar *message) 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); +} diff --git a/test/test-utils-glib.h b/test/test-utils-glib.h index 7cfc8785..93b52b74 100644 --- a/test/test-utils-glib.h +++ b/test/test-utils-glib.h @@ -129,4 +129,6 @@ void test_store_result_cb (GObject *source_object, void test_incomplete (const gchar *message); +gchar *test_get_helper_executable (const gchar *exe); + #endif From 93c1d08300605893cb1c6645bead1b1654378c56 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Wed, 31 Oct 2018 15:21:12 +0000 Subject: [PATCH 5/9] tests: Add a unit test for _dbus_command_for_pid() In particular this demonstrates that dbus#222 has been solved. Signed-off-by: Simon McVittie --- cmake/test/CMakeLists.txt | 1 + test/Makefile.am | 4 + test/internals/sysdeps.c | 166 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 171 insertions(+) create mode 100644 test/internals/sysdeps.c diff --git a/cmake/test/CMakeLists.txt b/cmake/test/CMakeLists.txt index 72175e20..3b139784 100644 --- a/cmake/test/CMakeLists.txt +++ b/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}) diff --git a/test/Makefile.am b/test/Makefile.am index 5f99b689..e439bfe4 100644 --- a/test/Makefile.am +++ b/test/Makefile.am @@ -108,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) @@ -172,6 +175,7 @@ installable_tests += \ test-relay \ test-server-oom \ test-syntax \ + test-sysdeps \ test-syslog \ test-uid-permissions \ test-variant \ diff --git a/test/internals/sysdeps.c b/test/internals/sysdeps.c new file mode 100644 index 00000000..06c302c4 --- /dev/null +++ b/test/internals/sysdeps.c @@ -0,0 +1,166 @@ +/* + * 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 + +#include +#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; + 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, NULL)) + { + gchar *expected; + int len; + + g_test_message ("Process %lu: \"%s\"", pid, + _dbus_string_get_const_data (&string)); + + len = _dbus_string_get_length (&string); + /* TODO: _dbus_command_for_pid() currently appends an unwanted + * space. See dbus#222 */ + _dbus_string_chop_white (&string); + + if (_dbus_string_get_length (&string) < len) + test_incomplete ("Ignoring unwanted whitespace"); + + 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"); + } + + 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; +} From f7bf69443df17ca4b58256030c337b0369c7ab80 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Thu, 25 Oct 2018 16:43:36 +0100 Subject: [PATCH 6/9] sysdeps: Remove trailing NUL from command lines from /proc Resolves: https://gitlab.freedesktop.org/dbus/dbus/issues/222 Signed-off-by: Simon McVittie --- dbus/dbus-sysdeps-util-unix.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/dbus/dbus-sysdeps-util-unix.c b/dbus/dbus-sysdeps-util-unix.c index 24eba4e3..262dcfde 100644 --- a/dbus/dbus-sysdeps-util-unix.c +++ b/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]; From d70040d8d2543eb0dc455f2c1db8660ead93ad97 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Wed, 31 Oct 2018 15:22:30 +0000 Subject: [PATCH 7/9] tests: Assert that dbus#222 has been fixed --- test/internals/sysdeps.c | 9 --------- 1 file changed, 9 deletions(-) diff --git a/test/internals/sysdeps.c b/test/internals/sysdeps.c index 06c302c4..f2e75e06 100644 --- a/test/internals/sysdeps.c +++ b/test/internals/sysdeps.c @@ -84,19 +84,10 @@ test_command_for_pid (Fixture *f, if (_dbus_command_for_pid (pid, &string, strlen (argv[0]) + 1024, NULL)) { gchar *expected; - int len; g_test_message ("Process %lu: \"%s\"", pid, _dbus_string_get_const_data (&string)); - len = _dbus_string_get_length (&string); - /* TODO: _dbus_command_for_pid() currently appends an unwanted - * space. See dbus#222 */ - _dbus_string_chop_white (&string); - - if (_dbus_string_get_length (&string) < len) - test_incomplete ("Ignoring unwanted whitespace"); - expected = g_strdup_printf ("%s %s", argv[0], argv[1]); g_assert_cmpstr (_dbus_string_get_const_data (&string), ==, From 47fc3ed2a9da3bbeb8b4004c8482ca56f69dcf55 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Thu, 25 Oct 2018 16:47:18 +0100 Subject: [PATCH 8/9] sysdeps: Return an error for _dbus_command_for_pid on Windows If a function returns boolean for success/error, and returns a DBusError, then it should set the DBusError if and only if it returns FALSE. Signed-off-by: Simon McVittie --- dbus/dbus-sysdeps-util-win.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/dbus/dbus-sysdeps-util-win.c b/dbus/dbus-sysdeps-util-win.c index 5aad2a86..fc356908 100644 --- a/dbus/dbus-sysdeps-util-win.c +++ b/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; } From e8cdb9171e989fb0f9d89f42832455800a17079e Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Thu, 25 Oct 2018 16:58:15 +0100 Subject: [PATCH 9/9] tests: Assert that _dbus_command_for_pid() has correct error behaviour Signed-off-by: Simon McVittie --- test/internals/sysdeps.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/test/internals/sysdeps.c b/test/internals/sysdeps.c index f2e75e06..992c4619 100644 --- a/test/internals/sysdeps.c +++ b/test/internals/sysdeps.c @@ -60,6 +60,7 @@ test_command_for_pid (Fixture *f, 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); @@ -81,7 +82,7 @@ test_command_for_pid (Fixture *f, if (!_dbus_string_init (&string)) g_error ("out of memory"); - if (_dbus_command_for_pid (pid, &string, strlen (argv[0]) + 1024, NULL)) + if (_dbus_command_for_pid (pid, &string, strlen (argv[0]) + 1024, &d_error)) { gchar *expected; @@ -98,7 +99,11 @@ test_command_for_pid (Fixture *f, } else { - g_test_message ("Unable to get command for process ID"); + 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))