From 1a4114999a7a18ad91038632f7cef7055b32c48c Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Mon, 6 Nov 2017 16:16:31 +0000 Subject: [PATCH] bus_connections_setup_connection: If we can't set it up, log why While reviewing fd.o#101354, Philip Withnall pointed out that if we rejected a connection in the new code there, we didn't log why. It turns out we didn't log that in the more normal code path either. Redo the error handling so that failure to set up a connection is logged. Signed-off-by: Simon McVittie --- bus/bus.c | 3 +-- bus/connection.c | 58 +++++++++++++++++++++++++++----------------------------- 2 files changed, 29 insertions(+), 32 deletions(-) diff --git a/bus/bus.c b/bus/bus.c index 27a13ec9..f0f07f6c 100644 --- a/bus/bus.c +++ b/bus/bus.c @@ -172,10 +172,9 @@ new_connection_callback (DBusServer *server, { BusContext *context = data; + /* If this fails it logs a warning, so we don't need to do that */ if (!bus_connections_setup_connection (context->connections, new_connection)) { - _dbus_verbose ("No memory to setup new connection\n"); - /* if we don't do this, it will get unref'd without * being disconnected... kind of strange really * that we have to do this, people won't get it right diff --git a/bus/connection.c b/bus/connection.c index 3fc62c78..53605fa3 100644 --- a/bus/connection.c +++ b/bus/connection.c @@ -725,15 +725,13 @@ bus_connections_setup_connection (BusConnections *connections, DBusConnection *connection) { - BusConnectionData *d; - dbus_bool_t retval; + BusConnectionData *d = NULL; DBusError error; - d = dbus_new0 (BusConnectionData, 1); if (d == NULL) - return FALSE; + goto oom; d->connections = connections; d->connection = connection; @@ -747,39 +745,35 @@ bus_connections_setup_connection (BusConnections *connections, connection_data_slot, d, free_connection_data)) { + /* We have to free d explicitly, because this is the only code + * path where it's non-NULL but dbus_connection_set_data() hasn't + * taken responsibility for freeing it. */ dbus_free (d); - return FALSE; + d = NULL; + goto oom; } dbus_connection_set_route_peer_messages (connection, TRUE); - - retval = FALSE; dbus_error_init (&error); d->selinux_id = bus_selinux_init_connection_id (connection, &error); if (dbus_error_is_set (&error)) { - /* This is a bit bogus because we pretend all errors - * are OOM; this is done because we know that in bus.c - * an OOM error disconnects the connection, which is - * the same thing we want on any other error. - */ + bus_context_log (connections->context, DBUS_SYSTEM_LOG_WARNING, + "Unable to set up new connection: %s", error.message); dbus_error_free (&error); - goto out; + goto error; } d->apparmor_confinement = bus_apparmor_init_connection_confinement (connection, &error); if (dbus_error_is_set (&error)) { - /* This is a bit bogus because we pretend all errors - * are OOM; this is done because we know that in bus.c - * an OOM error disconnects the connection, which is - * the same thing we want on any other error. - */ + bus_context_log (connections->context, DBUS_SYSTEM_LOG_WARNING, + "Unable to set up new connection: %s", error.message); dbus_error_free (&error); - goto out; + goto error; } if (!dbus_connection_set_watch_functions (connection, @@ -788,14 +782,14 @@ bus_connections_setup_connection (BusConnections *connections, toggle_connection_watch, connection, NULL)) - goto out; + goto oom; if (!dbus_connection_set_timeout_functions (connection, add_connection_timeout, remove_connection_timeout, NULL, connection, NULL)) - goto out; + goto oom; /* For now we don't need to set a Windows user function because * there are no policies in the config file controlling what @@ -813,18 +807,18 @@ bus_connections_setup_connection (BusConnections *connections, d->link_in_connection_list = _dbus_list_alloc_link (connection); if (d->link_in_connection_list == NULL) - goto out; + goto oom; /* Setup the connection with the dispatcher */ if (!bus_dispatch_add_connection (connection)) - goto out; + goto oom; if (dbus_connection_get_dispatch_status (connection) != DBUS_DISPATCH_COMPLETE) { if (!_dbus_loop_queue_dispatch (bus_context_get_loop (connections->context), connection)) { bus_dispatch_remove_connection (connection); - goto out; + goto oom; } } @@ -833,12 +827,12 @@ bus_connections_setup_connection (BusConnections *connections, pending_unix_fds_timeout_cb, connection, NULL); if (d->pending_unix_fds_timeout == NULL) - goto out; + goto oom; _dbus_timeout_disable (d->pending_unix_fds_timeout); if (!_dbus_loop_add_timeout (bus_context_get_loop (connections->context), d->pending_unix_fds_timeout)) - goto out; + goto oom; _dbus_connection_set_pending_fds_function (connection, (DBusPendingFdsChangeFunction) check_pending_fds_cb, @@ -861,10 +855,14 @@ bus_connections_setup_connection (BusConnections *connections, * stop accept()ing any more, to avert a DoS. See fd.o #80919 */ bus_context_check_all_watches (d->connections->context); - retval = TRUE; + return TRUE; - out: - if (!retval) +oom: + bus_context_log (connections->context, DBUS_SYSTEM_LOG_WARNING, + "No memory to set up new connection"); + /* fall through */ +error: + if (d != NULL) { d->selinux_id = NULL; @@ -916,7 +914,7 @@ bus_connections_setup_connection (BusConnections *connections, /* "d" has now been freed */ } - return retval; + return FALSE; } void -- 2.15.0