311 lines
11 KiB
Diff
311 lines
11 KiB
Diff
From 949a64b127a32a3e5a4ce4278773f18e290c44c2 Mon Sep 17 00:00:00 2001
|
|
From: Colin Walters <walters@verbum.org>
|
|
Date: Mon, 14 Dec 2009 23:12:24 +0000
|
|
Subject: 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 <rstrode@redhat.com>
|
|
---
|
|
diff --git a/bus/activation.c b/bus/activation.c
|
|
index 782ffed..00caac2 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,24 @@ 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. With the launch helper,
|
|
+ * some process exit codes are meaningful, processed by handle_servicehelper_exit_error.
|
|
+ *
|
|
+ * In both cases though, just ignore when a process exits with status 0; it's possible for
|
|
+ * a program to (misguidedly) "daemonize", and that appears to us as an exit. This closes a race
|
|
+ * condition between this code and the child process claiming the bus name.
|
|
+ */
|
|
+ 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 +1295,59 @@ 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))
|
|
+ /* Explicitly check for SPAWN_CHILD_EXITED to avoid overwriting an
|
|
+ * exec error */
|
|
+ if (dbus_error_has_name (&error, DBUS_ERROR_SPAWN_CHILD_EXITED)
|
|
+ && _dbus_babysitter_get_child_exit_status (babysitter, &exit_code))
|
|
+ {
|
|
+ activation_failed = exit_code != 0;
|
|
+
|
|
+ dbus_error_free(&error);
|
|
+
|
|
+ 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;
|
|
}
|
|
-
|
|
- /* 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
|
|
--- a/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/Makefile.am b/test/name-test/Makefile.am
|
|
index 1c73b87..d8e72d1 100644
|
|
--- a/test/name-test/Makefile.am
|
|
+++ b/test/name-test/Makefile.am
|
|
@@ -10,7 +10,7 @@ else
|
|
TESTS=
|
|
endif
|
|
|
|
-EXTRA_DIST=run-test.sh run-test-systemserver.sh test-wait-for-echo.py
|
|
+EXTRA_DIST=run-test.sh run-test-systemserver.sh test-wait-for-echo.py test-activation-forking.py
|
|
|
|
if DBUS_BUILD_TESTS
|
|
|
|
diff --git a/test/name-test/run-test.sh b/test/name-test/run-test.sh
|
|
index fba4558..4eb2425 100755
|
|
--- 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/name-test/test-activation-forking.py b/test/name-test/test-activation-forking.py
|
|
new file mode 100644
|
|
index 0000000..0d82075
|
|
--- a/dev/null
|
|
+++ b/test/name-test/test-activation-forking.py
|
|
@@ -0,0 +1,60 @@
|
|
+#!/usr/bin/env python
|
|
+
|
|
+import os,sys
|
|
+
|
|
+try:
|
|
+ import gobject
|
|
+ import dbus
|
|
+ import dbus.mainloop.glib
|
|
+except:
|
|
+ print "Failed import, aborting test"
|
|
+ sys.exit(0)
|
|
+
|
|
+dbus.mainloop.glib.DBusGMainLoop(set_as_default=True)
|
|
+loop = gobject.MainLoop()
|
|
+
|
|
+exitcode = 0
|
|
+
|
|
+bus = dbus.SessionBus()
|
|
+bus_iface = dbus.Interface(bus.get_object('org.freedesktop.DBus', '/org/freedesktop/DBus'), 'org.freedesktop.DBus')
|
|
+
|
|
+o = bus.get_object('org.freedesktop.DBus.TestSuiteForkingEchoService', '/org/freedesktop/TestSuite')
|
|
+i = dbus.Interface(o, 'org.freedesktop.TestSuite')
|
|
+
|
|
+# Start it up
|
|
+reply = i.Echo("hello world")
|
|
+print "TestSuiteForkingEchoService initial reply OK"
|
|
+
|
|
+def ignore(*args, **kwargs):
|
|
+ pass
|
|
+
|
|
+# Now monitor for exits, when that happens, start it up again.
|
|
+# The goal here is to try to hit any race conditions in activation.
|
|
+counter = 0
|
|
+def on_forking_echo_owner_changed(name, old, new):
|
|
+ global counter
|
|
+ global o
|
|
+ global i
|
|
+ if counter > 10:
|
|
+ print "Activated 10 times OK, TestSuiteForkingEchoService pass"
|
|
+ loop.quit()
|
|
+ return
|
|
+ counter += 1
|
|
+ if new == '':
|
|
+ o = bus.get_object('org.freedesktop.DBus.TestSuiteForkingEchoService', '/org/freedesktop/TestSuite')
|
|
+ i = dbus.Interface(o, 'org.freedesktop.TestSuite')
|
|
+ i.Echo("counter %r" % counter)
|
|
+ i.Exit(reply_handler=ignore, error_handler=ignore)
|
|
+
|
|
+bus_iface.connect_to_signal('NameOwnerChanged', on_forking_echo_owner_changed, arg0='org.freedesktop.DBus.TestSuiteForkingEchoService')
|
|
+
|
|
+i.Exit(reply_handler=ignore, error_handler=ignore)
|
|
+
|
|
+def check_counter():
|
|
+ if counter == 0:
|
|
+ print "Failed to get NameOwnerChanged for TestSuiteForkingEchoService"
|
|
+ sys.exit(1)
|
|
+gobject.timeout_add(15000, check_counter)
|
|
+
|
|
+loop.run()
|
|
+sys.exit(0)
|
|
diff --git a/test/test-service.c b/test/test-service.c
|
|
index c9f5839..a57bf9c 100644
|
|
--- a/test/test-service.c
|
|
+++ b/test/test-service.c
|
|
@@ -398,7 +398,33 @@ 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);
|
|
+ sleep (1);
|
|
+ }
|
|
+
|
|
dbus_error_init (&error);
|
|
connection = dbus_bus_get (DBUS_BUS_STARTER, &error);
|
|
if (connection == NULL)
|
|
@@ -433,8 +459,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))
|
|
{
|
|
--
|
|
cgit v0.8.3-6-g21f6
|