From eb63ba5039c8afe61210cf2b217ec75b4a86356e Mon Sep 17 00:00:00 2001 From: Havoc Pennington Date: Fri, 11 Apr 2003 03:05:58 +0000 Subject: [PATCH] 2003-04-10 Havoc Pennington * 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. --- ChangeLog | 11 ++++++- bus/activation.c | 85 +++++++++++++++++++++++++++++++++++++++++++++----- bus/driver.c | 27 +++++++++++++--- dbus/dbus-connection.c | 4 +-- dbus/dbus-internals.c | 19 +++++++++-- dbus/dbus-internals.h | 12 ++++--- dbus/dbus-spawn.c | 83 ++++++++++++++++++++++++++++-------------------- 7 files changed, 185 insertions(+), 56 deletions(-) diff --git a/ChangeLog b/ChangeLog index f36309a..2f407a4 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,12 @@ +2003-04-10 Havoc Pennington + + * 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 * 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 64e0d91..1a448a7 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 33017e9..f89b70a 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 5a6277d..9595f48 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 230c8ef..30f47f0 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 b6222fa..b0f4127 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 0e08cd7..c11dba2 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]); -- 2.7.4