]> git.ipfire.org Git - thirdparty/dbus.git/commitdiff
Ignore exit code zero from activated services
authorColin Walters <walters@verbum.org>
Mon, 14 Dec 2009 23:12:24 +0000 (18:12 -0500)
committerColin Walters <walters@verbum.org>
Tue, 15 Dec 2009 18:08:02 +0000 (13:08 -0500)
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>

bus/activation.c
configure.in
test/data/valid-service-files/org.freedesktop.DBus.TestSuiteForkingEchoService.service.in [new file with mode: 0644]
test/name-test/Makefile.am
test/name-test/run-test.sh
test/name-test/test-activation-forking.py [new file with mode: 0644]
test/test-service.c

index 782ffed82755c8532065b238a9a9787b6b0f30d0..00caac27785f696402e047640bb3f53a6ba85797 100644 (file)
@@ -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;
index 7ef6632ed4ce5316844900035d442b664e8f8190..1f2c89691ae12f923fdb7231003a1ace4a80a345 100644 (file)
@@ -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 (file)
index 0000000..49fcac3
--- /dev/null
@@ -0,0 +1,3 @@
+[D-BUS Service]
+Name=org.freedesktop.DBus.TestSuiteForkingEchoService
+Exec=@TEST_SERVICE_BINARY@ org.freedesktop.DBus.TestSuiteForkingEchoService fork
index 1c73b8771ff1fd1d01674a3394fbdf1927a89bc1..d8e72d14a3f7c3b29b0236827652366dc60b14ae 100644 (file)
@@ -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
 
index fba45584008fe11e308e27f3ab14edd53d57c9b4..4eb242529a6ec8ffd6ea9df75f24b920b290197d 100755 (executable)
@@ -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 (file)
index 0000000..0d82075
--- /dev/null
@@ -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)
index c9f583903520cbea61c083c9d6f17fa947e6d897..a57bf9c2bf0315c57967e814cc309a02725c11eb 100644 (file)
@@ -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))
     {