Attachment 104665 Details for Bug 81469 – [PATCH 2/3] enforce new limit max_connections_per_systemd_unit

[patch] [PATCH 2/3] enforce new limit max_connections_per_systemd_unit

0002-enforce-new-limit-max_connections_per_systemd_unit.patch (text/plain), 13.04 KB, created by Alban Crequy on 2014-08-15 09:21:59 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Alban Crequy

Created: 2014-08-15 09:21:59 UTC

Size: 13.04 KB

patch

obsolete

>From 4b5fd19a7428e3ce42a00bef1336938bf64303fd Mon Sep 17 00:00:00 2001
>From: Alban Crequy <alban.crequy@collabora.co.uk>
>Date: Wed, 16 Jul 2014 16:36:52 +0100
>Subject: [PATCH 2/3] enforce new limit max_connections_per_systemd_unit
>
>When it is not possible to get the Unix pid of a connection, that connection
>bypass the limit. It can happen on Windows or on the non-supported case where
>dbus-daemon is configured to listen on TCP rather than a Unix socket.
>
>When a process is not part of a system unit, sd_pid_get_unit() fails and the
>process is not restricted.
>
>v2:
> - fix memory leaks in adjust_connections_for_systemd_unit in case of oom
> - fix memory release in bus_connections_setup_connection in case of oom
>
>v3:
> - change max_connections_per_cgroup to max_connections_per_systemd_unit
> - use sd_pid_get_unit()
>
>https://bugs.freedesktop.org/show_bug.cgi?id=81469
>---
> bus/connection.c | 205 ++++++++++++++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 196 insertions(+), 9 deletions(-)
>
>diff --git a/bus/connection.c b/bus/connection.c
>index 4c19c8c..2e9ba0c 100644
>--- a/bus/connection.c
>+++ b/bus/connection.c
>@@ -34,6 +34,11 @@
> #include <dbus/dbus-hash.h>
> #include <dbus/dbus-timeout.h>
> 
>+#ifdef HAVE_SYSTEMD
>+#include <systemd/sd-login.h>
>+#include <stdlib.h>
>+#endif
>+
> /* Trim executed commands to this length; we want to keep logs readable */
> #define MAX_LOG_COMMAND_LEN 50
> 
>@@ -60,6 +65,7 @@ struct BusConnections
>   BusContext *context;
>   DBusHashTable *completed_by_user; /**< Number of completed connections for each UID */
>   DBusHashTable *completed_by_process; /**< Number of completed connections for each pid */
>+  DBusHashTable *completed_by_systemd_unit; /**< Number of completed connections for each systemd unit */
>   DBusTimeout *expire_timeout; /**< Timeout for expiring incomplete connections. */
>   int stamp;                   /**< Incrementing number */
>   BusExpireList *pending_replies; /**< List of pending replies */
>@@ -92,6 +98,8 @@ typedef struct
>   DBusPreallocatedSend *oom_preallocated;
>   BusClientPolicy *policy;
>   unsigned long pid;
>+  dbus_bool_t tracking_systemd_unit;
>+  DBusString systemd_unit;
> 
>   char *cached_loginfo_string;
>   BusSELinuxID *selinux_id;
>@@ -129,6 +137,23 @@ connection_get_loop (DBusConnection *connection)
> 
> 
> static int
>+get_connections_for_systemd_unit (BusConnections *connections,
>+                                  DBusString     *systemd_unit)
>+{
>+  void *val;
>+  int current_count;
>+
>+  /* val is NULL is 0 when it isn't in the hash yet */
>+
>+  val = _dbus_hash_table_lookup_string (connections->completed_by_systemd_unit,
>+                                        _dbus_string_get_data (systemd_unit));
>+
>+  current_count = _DBUS_POINTER_TO_INT (val);
>+
>+  return current_count;
>+}
>+
>+static int
> get_connections_for_uid (BusConnections *connections,
>                          dbus_uid_t      uid)
> {
>@@ -163,6 +188,82 @@ get_connections_for_pid (BusConnections *connections,
> }
> 
> static dbus_bool_t
>+adjust_connections_for_systemd_unit (BusConnections *connections,
>+                               DBusString     *systemd_unit,
>+                               int             adjustment)
>+{
>+  int current_count;
>+  char *systemd_unit_str;
>+
>+  systemd_unit_str = _dbus_string_get_data (systemd_unit);
>+
>+  current_count = get_connections_for_systemd_unit (connections, systemd_unit);
>+
>+  _dbus_verbose ("Adjusting connection count for systemd unit %s: "
>+                 "was %d adjustment %d making %d\n",
>+                 systemd_unit_str, current_count, adjustment,
>+                 current_count + adjustment);
>+
>+  _dbus_assert (current_count >= 0);
>+
>+  current_count += adjustment;
>+
>+  _dbus_assert (current_count >= 0);
>+
>+  if (current_count == 0)
>+    {
>+      _dbus_hash_table_remove_string (connections->completed_by_systemd_unit,
>+                                      systemd_unit_str);
>+      return TRUE;
>+    }
>+  else if (adjustment < 0)
>+    {
>+      dbus_bool_t retval;
>+      DBusHashIter iter;
>+
>+      /* Memory allocations are not allowed */
>+      retval = _dbus_hash_iter_lookup (connections->completed_by_systemd_unit,
>+                                       systemd_unit_str, FALSE, &iter);
>+      _dbus_assert (retval);
>+
>+      _dbus_hash_iter_set_value (&iter, _DBUS_INT_TO_POINTER (current_count));
>+
>+      return retval;
>+    }
>+  else
>+    {
>+      dbus_bool_t retval;
>+      DBusString copy;
>+
>+      /* duplicate the key */
>+
>+      if (!_dbus_string_init (&copy))
>+        return FALSE;
>+
>+      if (!_dbus_string_copy (systemd_unit, 0, &copy, 0))
>+        {
>+          _dbus_string_free (&copy);
>+          return FALSE;
>+        }
>+
>+      if (!_dbus_string_steal_data (&copy, &systemd_unit_str))
>+        {
>+          _dbus_string_free (&copy);
>+          return FALSE;
>+        }
>+
>+      _dbus_string_free (&copy);
>+
>+      retval = _dbus_hash_table_insert_string (connections->completed_by_systemd_unit,
>+                   systemd_unit_str, _DBUS_INT_TO_POINTER (current_count));
>+      if (!retval)
>+        dbus_free (systemd_unit_str);
>+
>+      return retval;
>+    }
>+}
>+
>+static dbus_bool_t
> adjust_connections_for_uid (BusConnections *connections,
>                             dbus_uid_t      uid,
>                             int             adjustment)
>@@ -346,6 +447,12 @@ bus_connection_disconnected (DBusConnection *connection)
>               if (!adjust_connections_for_uid (d->connections,
>                                                uid, -1))
>                 _dbus_assert_not_reached ("adjusting downward should never fail");
>+
>+            }
>+          if (d->tracking_systemd_unit)
>+            {
>+              if (!adjust_connections_for_systemd_unit (d->connections, &d->systemd_unit, -1))
>+                _dbus_assert_not_reached ("adjusting downward should never fail");
>             }
>           if (d->pid > 0)
>             {
>@@ -365,6 +472,8 @@ bus_connection_disconnected (DBusConnection *connection)
>       _dbus_assert (d->connections->n_completed >= 0);
>     }
> 
>+  _dbus_string_free (&d->systemd_unit);
>+
>   bus_connection_drop_pending_replies (d->connections, connection);
>   
>   /* frees "d" as side effect */
>@@ -500,11 +609,16 @@ bus_connections_new (BusContext *context)
>   if (connections->completed_by_process == NULL)
>     goto failed_3;
> 
>+  connections->completed_by_systemd_unit =
>+          _dbus_hash_table_new (DBUS_HASH_STRING, dbus_free, NULL);
>+  if (connections->completed_by_systemd_unit == NULL)
>+    goto failed_4;
>+
>   connections->expire_timeout = _dbus_timeout_new (100, /* irrelevant */
>                                                    expire_incomplete_timeout,
>                                                    connections, NULL);
>   if (connections->expire_timeout == NULL)
>-    goto failed_4;
>+    goto failed_5;
> 
>   _dbus_timeout_set_enabled (connections->expire_timeout, FALSE);
> 
>@@ -513,21 +627,23 @@ bus_connections_new (BusContext *context)
>                                                       bus_pending_reply_expired,
>                                                       connections);
>   if (connections->pending_replies == NULL)
>-    goto failed_5;
>+    goto failed_6;
>   
>   if (!_dbus_loop_add_timeout (bus_context_get_loop (context),
>                                connections->expire_timeout))
>-    goto failed_6;
>+    goto failed_7;
>   
>   connections->refcount = 1;
>   connections->context = context;
>   
>   return connections;
> 
>- failed_6:
>+ failed_7:
>   bus_expire_list_free (connections->pending_replies);
>- failed_5:
>+ failed_6:
>   _dbus_timeout_unref (connections->expire_timeout);
>+ failed_5:
>+  _dbus_hash_table_unref (connections->completed_by_systemd_unit);
>  failed_4:
>   _dbus_hash_table_unref (connections->completed_by_process);
>  failed_3:
>@@ -595,6 +711,7 @@ bus_connections_unref (BusConnections *connections)
>       
>       _dbus_hash_table_unref (connections->completed_by_user);
>       _dbus_hash_table_unref (connections->completed_by_process);
>+      _dbus_hash_table_unref (connections->completed_by_systemd_unit);
>       
>       dbus_free (connections);
> 
>@@ -670,7 +787,7 @@ bus_connections_setup_connection (BusConnections *connections,
>   BusConnectionData *d;
>   dbus_bool_t retval;
>   DBusError error;
>-
>+  dbus_bool_t systemd_unit_release_if_oom = FALSE;
>   
>   d = dbus_new0 (BusConnectionData, 1);
>   
>@@ -735,6 +852,10 @@ bus_connections_setup_connection (BusConnections *connections,
>                                           allow_unix_user_function,
>                                           NULL, NULL);
> 
>+  if (!_dbus_string_init (&d->systemd_unit))
>+    goto out;
>+  systemd_unit_release_if_oom = TRUE;
>+
>   dbus_connection_set_dispatch_status_function (connection,
>                                                 dispatch_status_function,
>                                                 bus_context_get_loop (connections->context),
>@@ -812,6 +933,16 @@ bus_connections_setup_connection (BusConnections *connections,
>       dbus_connection_set_unix_user_function (connection,
>                                               NULL, NULL, NULL);
> 
>+      /* If d->systemd_unit has not been initialized with _dbus_string_init(),
>+       * it is still zerod as part of its container BusConnectionData. It must
>+       * not be released in this case (see assertions in _dbus_string_free
>+       * and DBUS_GENERIC_STRING_PREAMBLE).
>+       *
>+       * But it must be released in other cases.
>+       */
>+      if (systemd_unit_release_if_oom)
>+        _dbus_string_free (&d->systemd_unit);
>+
>       dbus_connection_set_windows_user_function (connection,
>                                                  NULL, NULL, NULL);
>       
>@@ -1434,6 +1565,7 @@ bus_connection_complete (DBusConnection   *connection,
>   unsigned long uid;
>   dbus_bool_t rollback_uid = FALSE;
>   dbus_bool_t rollback_pid = FALSE;
>+  dbus_bool_t rollback_systemd_unit = FALSE;
>   
>   d = BUS_CONNECTION_DATA (connection);
>   _dbus_assert (d != NULL);
>@@ -1473,12 +1605,18 @@ bus_connection_complete (DBusConnection   *connection,
>   
>   if (dbus_connection_get_unix_user (connection, &uid))
>     {
>-      if (!adjust_connections_for_uid (d->connections,
>-                                       uid, 1))
>+      if (!adjust_connections_for_uid (d->connections, uid, 1))
>         goto fail;
>       rollback_uid = TRUE;
>     }
> 
>+  if (d->tracking_systemd_unit)
>+    {
>+      if (!adjust_connections_for_systemd_unit (d->connections, &d->systemd_unit, 1))
>+        goto fail;
>+      rollback_systemd_unit = TRUE;
>+    }
>+
>   if (d->pid > 0)
>     {
>       if (!adjust_connections_for_pid (d->connections, d->pid, 1))
>@@ -1515,6 +1653,8 @@ fail:
>     adjust_connections_for_uid (d->connections, uid, -1);
>   if (rollback_pid)
>     adjust_connections_for_pid (d->connections, d->pid, -1);
>+  if (rollback_systemd_unit)
>+    adjust_connections_for_systemd_unit (d->connections, &d->systemd_unit, -1);
>   dbus_free (d->name);
>   d->name = NULL;
>   if (d->policy)
>@@ -1581,6 +1721,7 @@ bus_connections_check_limits (BusConnections  *connections,
> {
>   unsigned long uid;
>   unsigned long pid;
>+  int count;
>   BusConnectionData *d;
> 
>   d = BUS_CONNECTION_DATA (requesting_completion);
>@@ -1605,9 +1746,10 @@ bus_connections_check_limits (BusConnections  *connections,
>           return FALSE;
>         }
>     }
>-  
>+
>   /* bus_connections_check_limits() could be called several times on the same
>    * connection. Make sure to be idempotent. */
>+  
>   if (d->pid == 0 &&
>       dbus_connection_get_unix_process_id (requesting_completion, &pid))
>     {
>@@ -1626,6 +1768,51 @@ bus_connections_check_limits (BusConnections  *connections,
>         }
>     }
> 
>+#ifdef HAVE_SYSTEMD
>+  if (!d->tracking_systemd_unit && d->pid > 0)
>+    {
>+      char *systemd_unit_str;
>+      if (sd_pid_get_unit (d->pid, &systemd_unit_str) == 0)
>+        {
>+          if (_dbus_string_append (&d->systemd_unit, systemd_unit_str))
>+            {
>+              d->tracking_systemd_unit = TRUE;
>+            }
>+          else
>+            {
>+              free (systemd_unit_str);
>+              BUS_SET_OOM (error);
>+              return FALSE;
>+            }
>+
>+          free (systemd_unit_str);
>+        }
>+      else
>+        {
>+          _dbus_verbose ("Couldn't get connection's systemd unit\n");
>+          /* ignore the error: the connection will not be accounted */
>+        }
>+    }
>+
>+  _dbus_verbose ("Checking max_connections_per_systemd_unit? %d\n", d->tracking_systemd_unit);
>+  if (d->tracking_systemd_unit)
>+    {
>+      _dbus_verbose ("Checking max_connections_per_systemd_unit: current count=%d max=%d\n",
>+                     get_connections_for_systemd_unit (connections, &d->systemd_unit),
>+                     bus_context_get_max_connections_per_systemd_unit (connections->context));
>+
>+      count = get_connections_for_systemd_unit (connections, &d->systemd_unit);
>+      if (count >=
>+          bus_context_get_max_connections_per_systemd_unit (connections->context))
>+        {
>+          dbus_set_error (error, DBUS_ERROR_LIMITS_EXCEEDED,
>+                          "The maximum number of active connections for systemd unit %s has been reached",
>+                          _dbus_string_get_data (&d->systemd_unit));
>+          return FALSE;
>+        }
>+    }
>+#endif
>+
>   return TRUE;
> }
> 
>-- 
>1.8.5.3
>

Actions: View | Splinter Review

Attachments on bug 81469: 102994 | 102995 | 102996 | 104444 | 104445 | 104446 | 104447 | 104664 | 104665 | 104666

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.