2003-04-10 Havoc Pennington <hp@pobox.com>

	* dbus/dbus-spawn.c (_dbus_spawn_async_with_babysitter): move all
	the possible parent failures before we fork, so that we don't
	fail to create a babysitter after creating the child.

	* bus/activation.c (bus_activation_activate_service): kill child
	if we don't successfully complete the activation.

7 files changed, 185 insertions, 56 deletions

diff --git a/ChangeLog b/ChangeLog
index f36309a0..2f407a46 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,12 @@
+2003-04-10  Havoc Pennington  <hp@pobox.com>
+
+	* dbus/dbus-spawn.c (_dbus_spawn_async_with_babysitter): move all
+	the possible parent failures before we fork, so that we don't 
+	fail to create a babysitter after creating the child.
+
+	* bus/activation.c (bus_activation_activate_service): kill child
+	if we don't successfully complete the activation.
+
 2003-04-10  Havoc Pennington  <hp@redhat.com>
 
 	* dbus/dbus-connection.c (dbus_connection_flush): don't spin on
@@ -9,7 +18,7 @@
 	pending activation if the function fails.
 
 	* dbus/dbus-list.c (_dbus_list_insert_before_link) 
-	(_dbus_list_insert_after_link): new code to facilitate 
+	(_dbus_list_insert_after_link): new code to facilitate
 	services.c fixes
 
 	* dbus/dbus-hash.c (_dbus_hash_table_insert_string_preallocated):
diff --git a/bus/activation.c b/bus/activation.c
index 64e0d914..1a448a79 100644
--- a/bus/activation.c
+++ b/bus/activation.c
@@ -779,6 +779,45 @@ pending_activation_timed_out (void *data)
   return TRUE;
 }
 
+static void
+cancel_pending (void *data)
+{
+  BusPendingActivation *pending_activation = data;
+
+  _dbus_verbose ("Canceling pending activation of %s\n",
+                 pending_activation->service_name);
+
+  if (pending_activation->babysitter)
+    _dbus_babysitter_kill_child (pending_activation->babysitter);
+  
+  _dbus_hash_table_remove_string (pending_activation->activation->pending_activations,
+                                  pending_activation->service_name);
+}
+
+static void
+free_pending_cancel_data (void *data)
+{
+  BusPendingActivation *pending_activation = data;
+  
+  bus_pending_activation_unref (pending_activation);
+}
+
+static dbus_bool_t
+add_cancel_pending_to_transaction (BusTransaction       *transaction,
+                                   BusPendingActivation *pending_activation)
+{  
+  if (!bus_transaction_add_cancel_hook (transaction, cancel_pending,
+                                        pending_activation,
+                                        free_pending_cancel_data))
+    return FALSE;
+
+  bus_pending_activation_ref (pending_activation); 
+  
+  _dbus_verbose ("Saved pending activation to be canceled if the transaction fails\n");
+  
+  return TRUE;
+}
+
 dbus_bool_t
 bus_activation_activate_service (BusActivation  *activation,
 				 DBusConnection *connection,
@@ -817,6 +856,7 @@ bus_activation_activate_service (BusActivation  *activation,
 
       if (!message)
 	{
+          _dbus_verbose ("No memory to create reply to activate message\n");
 	  BUS_SET_OOM (error);
 	  return FALSE;
 	}
@@ -826,6 +866,7 @@ bus_activation_activate_service (BusActivation  *activation,
 				     DBUS_TYPE_UINT32, DBUS_ACTIVATION_REPLY_ALREADY_ACTIVE, 
 				     0))
 	{
+          _dbus_verbose ("No memory to set args of reply to activate message\n");
 	  BUS_SET_OOM (error);
 	  dbus_message_unref (message);
 	  return FALSE;
@@ -834,7 +875,10 @@ bus_activation_activate_service (BusActivation  *activation,
       retval = bus_transaction_send_message (transaction, connection, message);
       dbus_message_unref (message);
       if (!retval)
-	BUS_SET_OOM (error);
+        {
+          _dbus_verbose ("Failed to send reply\n");
+          BUS_SET_OOM (error);
+        }
 
       return retval;
     }
@@ -842,6 +886,7 @@ bus_activation_activate_service (BusActivation  *activation,
   pending_activation_entry = dbus_new0 (BusPendingActivationEntry, 1);
   if (!pending_activation_entry)
     {
+      _dbus_verbose ("Failed to create pending activation entry\n");
       BUS_SET_OOM (error);
       return FALSE;
     }
@@ -855,11 +900,15 @@ bus_activation_activate_service (BusActivation  *activation,
   pending_activation = _dbus_hash_table_lookup_string (activation->pending_activations, service_name);
   if (pending_activation)
     {
+      /* FIXME security - a client could keep sending activations over and
+       * over, growing this queue.
+       */
       if (!_dbus_list_append (&pending_activation->entries, pending_activation_entry))
 	{
+          _dbus_verbose ("Failed to append a new entry to pending activation\n");
+          
 	  BUS_SET_OOM (error);
 	  bus_pending_activation_entry_free (pending_activation_entry);
-
 	  return FALSE;
 	}
     }
@@ -868,6 +917,8 @@ bus_activation_activate_service (BusActivation  *activation,
       pending_activation = dbus_new0 (BusPendingActivation, 1);
       if (!pending_activation)
 	{
+          _dbus_verbose ("Failed to create pending activation\n");
+          
 	  BUS_SET_OOM (error);
 	  bus_pending_activation_entry_free (pending_activation_entry);	  
 	  return FALSE;
@@ -879,6 +930,8 @@ bus_activation_activate_service (BusActivation  *activation,
       pending_activation->service_name = _dbus_strdup (service_name);
       if (!pending_activation->service_name)
 	{
+          _dbus_verbose ("Failed to copy service name for pending activation\n");
+          
 	  BUS_SET_OOM (error);
 	  bus_pending_activation_unref (pending_activation);
 	  bus_pending_activation_entry_free (pending_activation_entry);	  
@@ -892,6 +945,8 @@ bus_activation_activate_service (BusActivation  *activation,
                            NULL);
       if (!pending_activation->timeout)
 	{
+          _dbus_verbose ("Failed to create timeout for pending activation\n");
+          
 	  BUS_SET_OOM (error);
 	  bus_pending_activation_unref (pending_activation);
 	  bus_pending_activation_entry_free (pending_activation_entry);	  
@@ -904,6 +959,8 @@ bus_activation_activate_service (BusActivation  *activation,
                                    pending_activation,
                                    NULL))
 	{
+          _dbus_verbose ("Failed to add timeout for pending activation\n");
+          
 	  BUS_SET_OOM (error);
 	  bus_pending_activation_unref (pending_activation);
 	  bus_pending_activation_entry_free (pending_activation_entry);	  
@@ -914,6 +971,8 @@ bus_activation_activate_service (BusActivation  *activation,
       
       if (!_dbus_list_append (&pending_activation->entries, pending_activation_entry))
 	{
+          _dbus_verbose ("Failed to add entry to just-created pending activation\n");
+          
 	  BUS_SET_OOM (error);
 	  bus_pending_activation_unref (pending_activation);
 	  bus_pending_activation_entry_free (pending_activation_entry);	  
@@ -921,13 +980,25 @@ bus_activation_activate_service (BusActivation  *activation,
 	}
       
       if (!_dbus_hash_table_insert_string (activation->pending_activations,
-					   pending_activation->service_name, pending_activation))
+					   pending_activation->service_name,
+                                           pending_activation))
 	{
+          _dbus_verbose ("Failed to put pending activation in hash table\n");
+          
 	  BUS_SET_OOM (error);
 	  bus_pending_activation_unref (pending_activation);
 	  return FALSE;
 	}
     }
+
+  if (!add_cancel_pending_to_transaction (transaction, pending_activation))
+    {
+      _dbus_verbose ("Failed to add pending activation cancel hook to transaction\n");
+      BUS_SET_OOM (error);
+      _dbus_hash_table_remove_string (activation->pending_activations,
+				      pending_activation->service_name);
+      return FALSE;
+    }
   
   /* FIXME we need to support a full command line, not just a single
    * argv[0]
@@ -941,8 +1012,8 @@ bus_activation_activate_service (BusActivation  *activation,
                                           child_setup, activation, 
                                           error))
     {
-      _dbus_hash_table_remove_string (activation->pending_activations,
-				      pending_activation->service_name);
+      _dbus_verbose ("Failed to spawn child\n");
+      _DBUS_ASSERT_ERROR_IS_SET (error);
       return FALSE;
     }
 
@@ -956,9 +1027,7 @@ bus_activation_activate_service (BusActivation  *activation,
                                              NULL))
     {
       BUS_SET_OOM (error);
-      
-      _dbus_hash_table_remove_string (activation->pending_activations,
-				      pending_activation->service_name);
+      _dbus_verbose ("Failed to set babysitter watch functions\n");
       return FALSE;
     }
   
diff --git a/bus/driver.c b/bus/driver.c
index 33017e9f..f89b70a3 100644
--- a/bus/driver.c
+++ b/bus/driver.c
@@ -584,13 +584,21 @@ bus_driver_handle_activate_service (DBusConnection *connection,
                               DBUS_TYPE_STRING, &name,
                               DBUS_TYPE_UINT32, &flags,
                               0))
-    return FALSE;
+    {
+      _DBUS_ASSERT_ERROR_IS_SET (error);
+      _dbus_verbose ("No memory to get arguments to ActivateService\n");
+      return FALSE;
+    }
 
   retval = FALSE;
 
   if (!bus_activation_activate_service (activation, connection, transaction,
                                         message, name, error))
-    goto out;
+    {
+      _DBUS_ASSERT_ERROR_IS_SET (error);
+      _dbus_verbose ("bus_activation_activate_service() failed\n");
+      goto out;
+    }
 
   retval = TRUE;
   
@@ -650,15 +658,26 @@ bus_driver_handle_message (DBusConnection *connection,
     {
       if (strcmp (message_handlers[i].name, name) == 0)
         {
+          _dbus_verbose ("Running driver handler for %s\n", name);
           if ((* message_handlers[i].handler) (connection, transaction, message, error))
-            return TRUE;
+            {
+              _DBUS_ASSERT_ERROR_IS_CLEAR (error);
+              _dbus_verbose ("Driver handler succeeded\n");
+              return TRUE;
+            }
           else
-            return FALSE;
+            {
+              _DBUS_ASSERT_ERROR_IS_SET (error);
+              _dbus_verbose ("Driver handler returned failure\n");
+              return FALSE;
+            }
         }
       
       ++i;
     }
 
+  _dbus_verbose ("No driver handler for %s\n", name);
+
   dbus_set_error (error, DBUS_ERROR_UNKNOWN_MESSAGE,
                   "%s does not understand message %s",
                   DBUS_SERVICE_DBUS, name);
diff --git a/dbus/dbus-connection.c b/dbus/dbus-connection.c
index 5a6277de..9595f48a 100644
--- a/dbus/dbus-connection.c
+++ b/dbus/dbus-connection.c
@@ -1527,9 +1527,9 @@ dbus_connection_send_with_reply_and_block (DBusConnection     *connection,
   _dbus_get_current_time (&tv_sec, &tv_usec);
   
   if (tv_sec < start_tv_sec)
-    ; /* clock set backward, bail out */
+    _dbus_verbose ("dbus_connection_send_with_reply_and_block(): clock set backward\n");
   else if (connection->disconnect_message_link == NULL)
-    ; /* we're disconnected, bail out */
+    _dbus_verbose ("dbus_connection_send_with_reply_and_block(): disconnected\n");
   else if (tv_sec < end_tv_sec ||
            (tv_sec == end_tv_sec && tv_usec < end_tv_usec))
     {
diff --git a/dbus/dbus-internals.c b/dbus/dbus-internals.c
index 230c8efe..30f47f00 100644
--- a/dbus/dbus-internals.c
+++ b/dbus/dbus-internals.c
@@ -174,6 +174,8 @@ _dbus_warn (const char *format,
   va_end (args);
 }
 
+static dbus_bool_t verbose_initted = FALSE;
+
 /**
  * Prints a warning message to stderr
  * if the user has enabled verbose mode.
@@ -188,7 +190,6 @@ _dbus_verbose_real (const char *format,
 {
   va_list args;
   static dbus_bool_t verbose = TRUE;
-  static dbus_bool_t initted = FALSE;
   static unsigned long pid;
   
   /* things are written a bit oddly here so that
@@ -198,11 +199,11 @@ _dbus_verbose_real (const char *format,
   if (!verbose)
     return;
   
-  if (!initted)
+  if (!verbose_initted)
     {
       verbose = _dbus_getenv ("DBUS_VERBOSE") != NULL;
       pid = _dbus_getpid ();
-      initted = TRUE;
+      verbose_initted = TRUE;
       if (!verbose)
         return;
     }
@@ -215,6 +216,18 @@ _dbus_verbose_real (const char *format,
 }
 
 /**
+ * Reinitializes the verbose logging code, used
+ * as a hack in dbus-spawn.c so that a child
+ * process re-reads its pid
+ *
+ */
+void
+_dbus_verbose_reset_real (void)
+{
+  verbose_initted = FALSE;
+}
+
+/**
  * Duplicates a string. Result must be freed with
  * dbus_free(). Returns #NULL if memory allocation fails.
  * If the string to be duplicated is #NULL, returns #NULL.
diff --git a/dbus/dbus-internals.h b/dbus/dbus-internals.h
index b6222fae..b0f41278 100644
--- a/dbus/dbus-internals.h
+++ b/dbus/dbus-internals.h
@@ -53,14 +53,15 @@ DBUS_BEGIN_DECLS;
 #define _DBUS_GNUC_NORETURN
 #endif  /* !__GNUC__ */
 
-void _dbus_warn         (const char *format,
-                         ...) _DBUS_GNUC_PRINTF (1, 2);
-void _dbus_verbose_real (const char *format,
-                         ...) _DBUS_GNUC_PRINTF (1, 2);
-
+void _dbus_warn               (const char *format,
+                               ...) _DBUS_GNUC_PRINTF (1, 2);
+void _dbus_verbose_real       (const char *format,
+                               ...) _DBUS_GNUC_PRINTF (1, 2);
+void _dbus_verbose_reset_real (void);
 
 #ifdef DBUS_ENABLE_VERBOSE_MODE
 #  define _dbus_verbose _dbus_verbose_real
+#  define _dbus_verbose_reset _dbus_verbose_reset_real
 #else
 #  ifdef HAVE_ISO_VARARGS
 #    define _dbus_verbose(...)
@@ -69,6 +70,7 @@ void _dbus_verbose_real (const char *format,
 #  else
 #    error "This compiler does not support varargs macros and thus verbose mode can't be disabled meaningfully"
 #  endif
+#  define _dbus_verbose_reset()
 #endif /* !DBUS_ENABLE_VERBOSE_MODE */
 
 const char* _dbus_strerror (int error_number);
diff --git a/dbus/dbus-spawn.c b/dbus/dbus-spawn.c
index 0e08cd78..c11dba21 100644
--- a/dbus/dbus-spawn.c
+++ b/dbus/dbus-spawn.c
@@ -544,7 +544,6 @@ babysitter_iteration (DBusBabysitter *sitter,
  */
 #define LIVE_CHILDREN(sitter) ((sitter)->socket_to_babysitter >= 0 || (sitter)->error_pipe_from_child >= 0)
 
-
 /**
  * Blocks until the babysitter process gives us the PID of the spawned grandchild,
  * then kills the spawned grandchild.
@@ -559,6 +558,9 @@ _dbus_babysitter_kill_child (DBusBabysitter *sitter)
          sitter->grandchild_pid == -1)
     babysitter_iteration (sitter, TRUE);
 
+  _dbus_verbose ("Got child PID %ld for killing\n",
+                 (long) sitter->grandchild_pid);
+  
   if (sitter->grandchild_pid == -1)
     return; /* child is already dead, or we're so hosed we'll never recover */
 
@@ -826,6 +828,10 @@ do_exec (int                       child_err_report_fd,
   int i, max_open;
 #endif
 
+  _dbus_verbose_reset ();
+  _dbus_verbose ("Child process has PID %lu\n",
+                 _dbus_getpid ());
+  
   if (child_setup)
     (* child_setup) (user_data);
 
@@ -921,6 +927,11 @@ babysit (pid_t grandchild_pid,
 {
   int sigchld_pipe[2];
 
+  /* We don't exec, so we keep parent state, such as the pid that
+   * _dbus_verbose() uses. Reset the pid here.
+   */
+  _dbus_verbose_reset ();
+  
   /* I thought SIGCHLD would just wake up the poll, but
    * that didn't seem to work, so added this pipe.
    * Probably the pipe is more likely to work on busted
@@ -1029,6 +1040,43 @@ _dbus_spawn_async_with_babysitter (DBusBabysitter          **sitter_p,
 
   _dbus_fd_set_close_on_exec (babysitter_pipe[0]);
   _dbus_fd_set_close_on_exec (babysitter_pipe[1]);
+
+  /* Setting up the babysitter is only useful in the parent,
+   * but we don't want to run out of memory and fail
+   * after we've already forked, since then we'd leak
+   * child processes everywhere.
+   */
+  sitter->error_watch = _dbus_watch_new (child_err_report_pipe[READ_END],
+                                         DBUS_WATCH_READABLE,
+                                         TRUE);
+  if (sitter->error_watch == NULL)
+    {
+      dbus_set_error (error, DBUS_ERROR_NO_MEMORY, NULL);
+      goto cleanup_and_fail;
+    }
+        
+  if (!_dbus_watch_list_add_watch (sitter->watches,  sitter->error_watch))
+    {
+      dbus_set_error (error, DBUS_ERROR_NO_MEMORY, NULL);
+      goto cleanup_and_fail;
+    }
+      
+  sitter->sitter_watch = _dbus_watch_new (babysitter_pipe[0],
+                                          DBUS_WATCH_READABLE,
+                                          TRUE);
+  if (sitter->sitter_watch == NULL)
+    {
+      dbus_set_error (error, DBUS_ERROR_NO_MEMORY, NULL);
+      goto cleanup_and_fail;
+    }
+      
+  if (!_dbus_watch_list_add_watch (sitter->watches,  sitter->sitter_watch))
+    {
+      dbus_set_error (error, DBUS_ERROR_NO_MEMORY, NULL);
+      goto cleanup_and_fail;
+    }
+
+  _DBUS_ASSERT_ERROR_IS_CLEAR (error);
   
   pid = fork ();
   
@@ -1077,38 +1125,7 @@ _dbus_spawn_async_with_babysitter (DBusBabysitter          **sitter_p,
 	}
     }
   else
-    {
-      /* Parent */
-      sitter->error_watch = _dbus_watch_new (child_err_report_pipe[READ_END],
-                                             DBUS_WATCH_READABLE,
-                                             TRUE);
-      if (sitter->error_watch == NULL)
-        {
-          dbus_set_error (error, DBUS_ERROR_NO_MEMORY, NULL);
-          goto cleanup_and_fail;
-        }
-        
-      if (!_dbus_watch_list_add_watch (sitter->watches,  sitter->error_watch))
-        {
-          dbus_set_error (error, DBUS_ERROR_NO_MEMORY, NULL);
-          goto cleanup_and_fail;
-        }
-      
-      sitter->sitter_watch = _dbus_watch_new (babysitter_pipe[0],
-                                              DBUS_WATCH_READABLE,
-                                              TRUE);
-      if (sitter->sitter_watch == NULL)
-        {
-          dbus_set_error (error, DBUS_ERROR_NO_MEMORY, NULL);
-          goto cleanup_and_fail;
-        }
-      
-      if (!_dbus_watch_list_add_watch (sitter->watches,  sitter->sitter_watch))
-        {
-          dbus_set_error (error, DBUS_ERROR_NO_MEMORY, NULL);
-          goto cleanup_and_fail;
-        }
-      
+    {      
       /* Close the uncared-about ends of the pipes */
       close_and_invalidate (&child_err_report_pipe[WRITE_END]);
       close_and_invalidate (&babysitter_pipe[1]);