From d8a51a14fea8b2f00e66cc07e692e695a8861c4d Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Mon, 14 Dec 2009 18:12:24 -0500 Subject: [PATCH] Ignore exit code zero from activated services A variety of system components have migrated from legacy init into DBus service activation. Many of these system components "daemonize", which involves forking. The DBus activation system treated an exit as an activation failure, assuming that the child process which grabbed the DBus name didn't run first. While we're in here, also differentiate in this code path between the servicehelper (system) versus direct activation (session) paths. In the session activation path our error message mentioned a helper process which was confusing, since none was involved. Based on a patch and debugging research from Ray Strode --- bus/activation.c | 82 +++++++++++++------- configure.in | 1 + ...top.DBus.TestSuiteForkingEchoService.service.in | 3 + test/name-test/run-test.sh | 6 ++ test/test-service.c | 31 +++++++- 5 files changed, 91 insertions(+), 32 deletions(-) create mode 100644 test/data/valid-service-files/org.freedesktop.DBus.TestSuiteForkingEchoService.service.in mode change 100755 => 100644 test/name-test/run-test.sh diff --git a/bus/activation.c b/bus/activation.c index 782ffed..9683001 100644 --- a/bus/activation.c +++ b/bus/activation.c @@ -1212,8 +1212,8 @@ pending_activation_failed (BusPendingActivation *pending_activation, * Depending on the exit code of the helper, set the error accordingly */ static void -handle_activation_exit_error (int exit_code, - DBusError *error) +handle_servicehelper_exit_error (int exit_code, + DBusError *error) { switch (exit_code) { @@ -1268,13 +1268,27 @@ babysitter_watch_callback (DBusWatch *watch, BusPendingActivation *pending_activation = data; dbus_bool_t retval; DBusBabysitter *babysitter; + dbus_bool_t uses_servicehelper; babysitter = pending_activation->babysitter; - + _dbus_babysitter_ref (babysitter); - + retval = dbus_watch_handle (watch, condition); + /* There are two major cases here; are we the system bus or the session? Here this + * is distinguished by whether or not we use a setuid helper launcher. On the system + * bus, we want to ignore direct child exits (reflected by the DBUS_ERROR_SPAWN_CHILD_EXITED) + * because it may have (misguidedly) "daemonized", as can happen when code is taken from + * an init script and put into a DBus service file. + * + * On the session bus, we're monitoring the binary directly, not monitoring the service helper. + * In that case, just ignore the return value entirely; while it's less likely session code + * will be daemonizing, it's possible. The tradeoff is here system resilience versus + * developer/sysadmin convenience, and I've decided the former wins. + */ + uses_servicehelper = bus_context_get_servicehelper (pending_activation->activation->context) != NULL; + /* FIXME this is broken in the same way that * connection watches used to be; there should be * a separate callback for status change, instead @@ -1284,43 +1298,53 @@ babysitter_watch_callback (DBusWatch *watch, * Fixing this lets us move dbus_watch_handle * calls into dbus-mainloop.c */ - if (_dbus_babysitter_get_child_exited (babysitter)) { DBusError error; DBusHashIter iter; - + dbus_bool_t activation_failed; + int exit_code = 0; + dbus_error_init (&error); - _dbus_babysitter_set_child_exit_error (babysitter, &error); - /* refine the error code if we got an exit code */ - if (dbus_error_has_name (&error, DBUS_ERROR_SPAWN_CHILD_EXITED)) - { - int exit_code = 0; - if (_dbus_babysitter_get_child_exit_status (babysitter, &exit_code)) + if (_dbus_babysitter_get_child_exit_status (babysitter, &exit_code)) + { + activation_failed = exit_code != 0; + + if (activation_failed) { - dbus_error_free (&error); - handle_activation_exit_error (exit_code, &error); + if (uses_servicehelper) + handle_servicehelper_exit_error (exit_code, &error); + else + _dbus_babysitter_set_child_exit_error (babysitter, &error); } - } - - /* Destroy all pending activations with the same exec */ - _dbus_hash_iter_init (pending_activation->activation->pending_activations, - &iter); - while (_dbus_hash_iter_next (&iter)) + } + else { - BusPendingActivation *p = _dbus_hash_iter_get_value (&iter); - - if (p != pending_activation && strcmp (p->exec, pending_activation->exec) == 0) - pending_activation_failed (p, &error); + activation_failed = TRUE; + _dbus_babysitter_set_child_exit_error (babysitter, &error); } - - /* Destroys the pending activation */ - pending_activation_failed (pending_activation, &error); - dbus_error_free (&error); + if (activation_failed) + { + /* Destroy all pending activations with the same exec */ + _dbus_hash_iter_init (pending_activation->activation->pending_activations, + &iter); + while (_dbus_hash_iter_next (&iter)) + { + BusPendingActivation *p = _dbus_hash_iter_get_value (&iter); + + if (p != pending_activation && strcmp (p->exec, pending_activation->exec) == 0) + pending_activation_failed (p, &error); + } + + /* Destroys the pending activation */ + pending_activation_failed (pending_activation, &error); + + dbus_error_free (&error); + } } - + _dbus_babysitter_unref (babysitter); return retval; diff --git a/configure.in b/configure.in index 7ef6632..1f2c896 100644 --- a/configure.in +++ b/configure.in @@ -1499,6 +1499,7 @@ test/data/valid-config-files-system/debug-allow-all-pass.conf test/data/valid-config-files-system/debug-allow-all-fail.conf test/data/valid-service-files/org.freedesktop.DBus.TestSuite.PrivServer.service test/data/valid-service-files/org.freedesktop.DBus.TestSuiteEchoService.service +test/data/valid-service-files/org.freedesktop.DBus.TestSuiteForkingEchoService.service test/data/valid-service-files/org.freedesktop.DBus.TestSuiteSegfaultService.service test/data/valid-service-files/org.freedesktop.DBus.TestSuiteShellEchoServiceSuccess.service test/data/valid-service-files/org.freedesktop.DBus.TestSuiteShellEchoServiceFail.service diff --git a/test/data/valid-service-files/org.freedesktop.DBus.TestSuiteForkingEchoService.service.in b/test/data/valid-service-files/org.freedesktop.DBus.TestSuiteForkingEchoService.service.in new file mode 100644 index 0000000..49fcac3 --- /dev/null +++ b/test/data/valid-service-files/org.freedesktop.DBus.TestSuiteForkingEchoService.service.in @@ -0,0 +1,3 @@ +[D-BUS Service] +Name=org.freedesktop.DBus.TestSuiteForkingEchoService +Exec=@TEST_SERVICE_BINARY@ org.freedesktop.DBus.TestSuiteForkingEchoService fork diff --git a/test/name-test/run-test.sh b/test/name-test/run-test.sh old mode 100755 new mode 100644 index fba4558..4eb2425 --- a/test/name-test/run-test.sh +++ b/test/name-test/run-test.sh @@ -50,3 +50,9 @@ ${DBUS_TOP_BUILDDIR}/libtool --mode=execute $DEBUG $DBUS_TOP_BUILDDIR/test/name- echo "running test-shutdown" ${DBUS_TOP_BUILDDIR}/libtool --mode=execute $DEBUG $DBUS_TOP_BUILDDIR/test/name-test/test-shutdown || die "test-shutdown failed" + +echo "running test activation forking" +if ! python $DBUS_TOP_SRCDIR/test/name-test/test-activation-forking.py; then + echo "Failed test-activation-forking" + exit 1 +fi diff --git a/test/test-service.c b/test/test-service.c index c9f5839..6088006 100644 --- a/test/test-service.c +++ b/test/test-service.c @@ -398,7 +398,32 @@ main (int argc, DBusError error; int result; DBusConnection *connection; - + const char *name; + dbus_bool_t do_fork; + + if (argc != 3) + { + name = "org.freedesktop.DBus.TestSuiteEchoService"; + do_fork = FALSE; + } + else + { + name = argv[1]; + do_fork = strcmp (argv[2], "fork") == 0; + } + + /* The bare minimum for simulating a program "daemonizing"; the intent + * is to test services which move from being legacy init scripts to + * activated services. + * https://bugzilla.redhat.com/show_bug.cgi?id=545267 + */ + if (do_fork) + { + pid_t pid = fork (); + if (pid != 0) + exit (0); + } + dbus_error_init (&error); connection = dbus_bus_get (DBUS_BUS_STARTER, &error); if (connection == NULL) @@ -433,8 +458,8 @@ main (int argc, if (d != (void*) 0xdeadbeef) die ("dbus_connection_get_object_path_data() doesn't seem to work right\n"); } - - result = dbus_bus_request_name (connection, "org.freedesktop.DBus.TestSuiteEchoService", + + result = dbus_bus_request_name (connection, name, 0, &error); if (dbus_error_is_set (&error)) { -- 1.6.2.5