Attachment 135261 Details for Bug 103592 – bus_connections_setup_connection: If we can't set it up, log why

[patch] bus_connections_setup_connection: If we can't set it up, log why

0001-bus_connections_setup_connection-If-we-can-t-set-it-.patch (text/plain), 6.51 KB, created by Simon McVittie on 2017-11-06 16:35:01 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Simon McVittie

Created: 2017-11-06 16:35:01 UTC

Size: 6.51 KB

patch

obsolete

>From 1a4114999a7a18ad91038632f7cef7055b32c48c Mon Sep 17 00:00:00 2001
>From: Simon McVittie <smcv@collabora.com>
>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 <smcv@collabora.com>
>---
> 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
>

Actions: View | Splinter Review

Attachments on bug 103592: 135261

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.