Attachment 105796 Details for Bug 80919 – [PATCH v3] Stop listening on DBusServer sockets when reaching max_incomplete_connections

[patch] [PATCH v3] Stop listening on DBusServer sockets when reaching max_incomplete_connections

0001-Stop-listening-on-DBusServer-sockets-when-reaching-m.patch (text/plain), 11.55 KB, created by Alban Crequy on 2014-09-05 11:30:58 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Alban Crequy

Created: 2014-09-05 11:30:58 UTC

Size: 11.55 KB

patch

obsolete

>From 6c52312ab8055f59107fad555d5c87f96a6994ba Mon Sep 17 00:00:00 2001
>From: Alban Crequy <alban.crequy@collabora.co.uk>
>Date: Fri, 4 Jul 2014 15:05:51 +0100
>Subject: [PATCH] Stop listening on DBusServer sockets when reaching
> max_incomplete_connections
>
>https://bugs.freedesktop.org/show_bug.cgi?id=80851
>https://bugs.freedesktop.org/show_bug.cgi?id=80919
>---
> bus/bus.c                    | 37 +++++++++++++++++++++++++++++++++++++
> bus/bus.h                    |  1 +
> bus/connection.c             | 42 ++++++++++++++++++------------------------
> bus/connection.h             |  3 ++-
> dbus/dbus-server-protected.h |  5 ++---
> dbus/dbus-server.c           | 19 +++++--------------
> dbus/dbus-watch.c            | 21 +++++++++++++++++++++
> dbus/dbus-watch.h            |  2 ++
> 8 files changed, 88 insertions(+), 42 deletions(-)
>
>diff --git a/bus/bus.c b/bus/bus.c
>index a514e31..a3dce24 100644
>--- a/bus/bus.c
>+++ b/bus/bus.c
>@@ -39,6 +39,7 @@
> #include <dbus/dbus-hash.h>
> #include <dbus/dbus-credentials.h>
> #include <dbus/dbus-internals.h>
>+#include <dbus/dbus-server-protected.h>
> 
> #ifdef DBUS_CYGWIN
> #include <signal.h>
>@@ -68,6 +69,7 @@ struct BusContext
>   unsigned int keep_umask : 1;
>   unsigned int allow_anonymous : 1;
>   unsigned int systemd_activation : 1;
>+  dbus_bool_t watches_enabled;
> };
> 
> static dbus_int32_t server_data_slot = -1;
>@@ -758,6 +760,8 @@ bus_context_new (const DBusString *config_file,
>       goto failed;
>     }
> 
>+  context->watches_enabled = TRUE;
>+
>   context->registry = bus_registry_new (context);
>   if (context->registry == NULL)
>     {
>@@ -1658,3 +1662,36 @@ bus_context_check_security_policy (BusContext     *context,
>   _dbus_verbose ("security policy allowing message\n");
>   return TRUE;
> }
>+
>+void
>+bus_context_check_all_watches (BusContext *context)
>+{
>+  DBusList *link;
>+  dbus_bool_t enabled = TRUE;
>+
>+  if (bus_connections_get_n_incomplete (context->connections) >=
>+      bus_context_get_max_incomplete_connections (context))
>+    {
>+      enabled = FALSE;
>+    }
>+
>+  if (context->watches_enabled == enabled)
>+    return;
>+
>+  context->watches_enabled = enabled;
>+
>+  for (link = _dbus_list_get_first_link (&context->servers);
>+       link != NULL;
>+       link = _dbus_list_get_next_link (&context->servers, link))
>+    {
>+      /* A BusContext might contains several DBusServer (if there are
>+       * several <listen> configuration items) and a DBusServer might
>+       * contain several DBusWatch in its DBusWatchList (if getaddrinfo
>+       * returns several addresses on a dual IPv4-IPv6 stack or if
>+       * systemd passes several fds).
>+       * We want to enable/disable them all.
>+       */
>+      DBusServer *server = link->data;
>+      _dbus_server_toggle_all_watches (server, enabled);
>+    }
>+}
>diff --git a/bus/bus.h b/bus/bus.h
>index 3597884..400c9d0 100644
>--- a/bus/bus.h
>+++ b/bus/bus.h
>@@ -125,5 +125,6 @@ dbus_bool_t       bus_context_check_security_policy              (BusContext
>                                                                   DBusConnection   *proposed_recipient,
>                                                                   DBusMessage      *message,
>                                                                   DBusError        *error);
>+void              bus_context_check_all_watches                  (BusContext       *context);
> 
> #endif /* BUS_BUS_H */
>diff --git a/bus/connection.c b/bus/connection.c
>index ea2d155..54fa3ab 100644
>--- a/bus/connection.c
>+++ b/bus/connection.c
>@@ -293,6 +293,10 @@ bus_connection_disconnected (DBusConnection *connection)
>           _dbus_list_remove_link (&d->connections->incomplete, d->link_in_connection_list);
>           d->link_in_connection_list = NULL;
>           d->connections->n_incomplete -= 1;
>+
>+          /* If we have dropped below the max. number of incomplete
>+           * connections, start accept()ing again */
>+          bus_context_check_all_watches (d->connections->context);
>         }
>       
>       _dbus_assert (d->connections->n_incomplete >= 0);
>@@ -688,31 +692,17 @@ bus_connections_setup_connection (BusConnections *connections,
>   
>   dbus_connection_ref (connection);
> 
>-  /* Note that we might disconnect ourselves here, but it only takes
>-   * effect on return to the main loop. We call this to free up
>-   * expired connections if possible, and to queue the timeout for our
>-   * own expiration.
>-   */
>   bus_connections_expire_incomplete (connections);
>   
>-  /* And we might also disconnect ourselves here, but again it
>-   * only takes effect on return to main loop.
>-   */
>-  if (connections->n_incomplete >
>-      bus_context_get_max_incomplete_connections (connections->context))
>-    {
>-      _dbus_verbose ("Number of incomplete connections exceeds max, dropping oldest one\n");
>-      
>-      _dbus_assert (connections->incomplete != NULL);
>-      /* Disconnect the oldest unauthenticated connection.  FIXME
>-       * would it be more secure to drop a *random* connection?  This
>-       * algorithm seems to mean that if someone can create new
>-       * connections quickly enough, they can keep anyone else from
>-       * completing authentication. But random may or may not really
>-       * help with that, a more elaborate solution might be required.
>-       */
>-      dbus_connection_close (connections->incomplete->data);
>-    }
>+  /* The listening socket is removed from the main loop,
>+   * i.e. does not accept(), while n_incomplete is at its
>+   * maximum value; so we shouldn't get here in that case */
>+  _dbus_assert (connections->n_incomplete <=
>+      bus_context_get_max_incomplete_connections (connections->context));
>+
>+  /* If we have the maximum number of incomplete connections,
>+   * stop accept()ing any more, to avert a DoS. See fd.o #80919 */
>+  bus_context_check_all_watches (d->connections->context);
>   
>   retval = TRUE;
> 
>@@ -1419,6 +1409,10 @@ bus_connection_complete (DBusConnection   *connection,
>   _dbus_assert (d->connections->n_incomplete >= 0);
>   _dbus_assert (d->connections->n_completed > 0);
> 
>+  /* If we have dropped below the max. number of incomplete
>+   * connections, start accept()ing again */
>+  bus_context_check_all_watches (d->connections->context);
>+
>   /* See if we can remove the timeout */
>   bus_connections_expire_incomplete (d->connections);
> 
>@@ -2348,7 +2342,6 @@ bus_transaction_add_cancel_hook (BusTransaction               *transaction,
>   return TRUE;
> }
> 
>-#ifdef DBUS_ENABLE_STATS
> int
> bus_connections_get_n_active (BusConnections *connections)
> {
>@@ -2361,6 +2354,7 @@ bus_connections_get_n_incomplete (BusConnections *connections)
>   return connections->n_incomplete;
> }
> 
>+#ifdef DBUS_ENABLE_STATS
> int
> bus_connections_get_total_match_rules (BusConnections *connections)
> {
>diff --git a/bus/connection.h b/bus/connection.h
>index 9f4f9ae..6fbcd38 100644
>--- a/bus/connection.h
>+++ b/bus/connection.h
>@@ -139,9 +139,10 @@ dbus_bool_t     bus_transaction_add_cancel_hook  (BusTransaction               *
>                                                   void                         *data,
>                                                   DBusFreeFunction              free_data_function);
> 
>-/* called by stats.c, only present if DBUS_ENABLE_STATS */
> int bus_connections_get_n_active                  (BusConnections *connections);
> int bus_connections_get_n_incomplete              (BusConnections *connections);
>+
>+/* called by stats.c, only present if DBUS_ENABLE_STATS */
> int bus_connections_get_total_match_rules         (BusConnections *connections);
> int bus_connections_get_peak_match_rules          (BusConnections *connections);
> int bus_connections_get_peak_match_rules_per_conn (BusConnections *connections);
>diff --git a/dbus/dbus-server-protected.h b/dbus/dbus-server-protected.h
>index dd5234b..e6dbd1e 100644
>--- a/dbus/dbus-server-protected.h
>+++ b/dbus/dbus-server-protected.h
>@@ -99,9 +99,8 @@ dbus_bool_t _dbus_server_add_watch      (DBusServer             *server,
>                                          DBusWatch              *watch);
> void        _dbus_server_remove_watch   (DBusServer             *server,
>                                          DBusWatch              *watch);
>-void        _dbus_server_toggle_watch   (DBusServer             *server,
>-                                         DBusWatch              *watch,
>-                                         dbus_bool_t             enabled);
>+void        _dbus_server_toggle_all_watches (DBusServer         *server,
>+                                             dbus_bool_t         enabled);
> dbus_bool_t _dbus_server_add_timeout    (DBusServer             *server,
>                                          DBusTimeout            *timeout);
> void        _dbus_server_remove_timeout (DBusServer             *server,
>diff --git a/dbus/dbus-server.c b/dbus/dbus-server.c
>index 19d8590..c1d5f6e 100644
>--- a/dbus/dbus-server.c
>+++ b/dbus/dbus-server.c
>@@ -312,26 +312,17 @@ _dbus_server_remove_watch  (DBusServer *server,
> }
> 
> /**
>- * Toggles a watch and notifies app via server's
>- * DBusWatchToggledFunction if available. It's an error to call this
>- * function on a watch that was not previously added.
>+ * Toggles all watch and notifies app via server's
>+ * DBusWatchToggledFunction if available.
>  *
>  * @param server the server.
>- * @param watch the watch to toggle.
>  * @param enabled whether to enable or disable
>  */
> void
>-_dbus_server_toggle_watch (DBusServer  *server,
>-                           DBusWatch   *watch,
>-                           dbus_bool_t  enabled)
>+_dbus_server_toggle_all_watches (DBusServer  *server,
>+                                dbus_bool_t  enabled)
> {
>-  _dbus_assert (watch != NULL);
>-
>-  HAVE_LOCK_CHECK (server);
>-  protected_change_watch (server, watch,
>-                          NULL, NULL,
>-                          _dbus_watch_list_toggle_watch,
>-                          enabled);
>+  _dbus_watch_list_toggle_all_watches (server->watches, enabled);
> }
> 
> /** Function to be called in protected_change_timeout() with refcount held */
>diff --git a/dbus/dbus-watch.c b/dbus/dbus-watch.c
>index b82c57d..76a5d64 100644
>--- a/dbus/dbus-watch.c
>+++ b/dbus/dbus-watch.c
>@@ -455,6 +455,27 @@ _dbus_watch_list_toggle_watch (DBusWatchList           *watch_list,
> }
> 
> /**
>+ * Sets all watches to the given enabled state, invoking the
>+ * application's DBusWatchToggledFunction if appropriate.
>+ *
>+ * @param watch_list the watch list.
>+ * @param enabled #TRUE to enable
>+ */
>+void
>+_dbus_watch_list_toggle_all_watches (DBusWatchList           *watch_list,
>+                                     dbus_bool_t              enabled)
>+{
>+  DBusList *link;
>+
>+  for (link = _dbus_list_get_first_link (&watch_list->watches);
>+       link != NULL;
>+       link = _dbus_list_get_next_link (&watch_list->watches, link))
>+    {
>+      _dbus_watch_list_toggle_watch (watch_list, link->data, enabled);
>+    }
>+}
>+
>+/**
>  * Sets the handler for the watch.
>  *
>  * @todo this function only exists because of the weird
>diff --git a/dbus/dbus-watch.h b/dbus/dbus-watch.h
>index c583214..321740e 100644
>--- a/dbus/dbus-watch.h
>+++ b/dbus/dbus-watch.h
>@@ -76,6 +76,8 @@ void           _dbus_watch_list_remove_watch  (DBusWatchList           *watch_li
> void           _dbus_watch_list_toggle_watch  (DBusWatchList           *watch_list,
>                                                DBusWatch               *watch,
>                                                dbus_bool_t              enabled);
>+void           _dbus_watch_list_toggle_all_watches (DBusWatchList      *watch_list,
>+                                               dbus_bool_t              enabled);
> dbus_bool_t    _dbus_watch_get_enabled        (DBusWatch              *watch);
> 
> dbus_bool_t    _dbus_watch_get_oom_last_time  (DBusWatch               *watch);
>-- 
>1.8.5.3
>

Actions: View | Splinter Review

Attachments on bug 80919: 102277 | 102375 | 102430 | 102769 | 105796

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.