From 8d8ad4e4239ffd506c8c23aa1a50115905fe599a Mon Sep 17 00:00:00 2001 From: Alban Crequy Date: Wed, 16 Jul 2014 16:36:52 +0100 Subject: [PATCH 3/4] enforce new limit max_connections_per_cgroup 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. v2: - fix memory leaks in adjust_connections_for_cgroup in case of oom - fix memory release in bus_connections_setup_connection in case of oom https://bugs.freedesktop.org/show_bug.cgi?id=81469 --- bus/connection.c | 187 ++++++++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 178 insertions(+), 9 deletions(-) diff --git a/bus/connection.c b/bus/connection.c index 4c19c8c..dca2137 100644 --- a/bus/connection.c +++ b/bus/connection.c @@ -60,6 +60,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_cgroup; /**< Number of completed connections for each cgroup */ DBusTimeout *expire_timeout; /**< Timeout for expiring incomplete connections. */ int stamp; /**< Incrementing number */ BusExpireList *pending_replies; /**< List of pending replies */ @@ -92,6 +93,8 @@ typedef struct DBusPreallocatedSend *oom_preallocated; BusClientPolicy *policy; unsigned long pid; + dbus_bool_t tracking_cgroup; + DBusString cgroup; char *cached_loginfo_string; BusSELinuxID *selinux_id; @@ -129,6 +132,23 @@ connection_get_loop (DBusConnection *connection) static int +get_connections_for_cgroup (BusConnections *connections, + DBusString *cgroup) +{ + 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_cgroup, + _dbus_string_get_data (cgroup)); + + current_count = _DBUS_POINTER_TO_INT (val); + + return current_count; +} + +static int get_connections_for_uid (BusConnections *connections, dbus_uid_t uid) { @@ -163,6 +183,82 @@ get_connections_for_pid (BusConnections *connections, } static dbus_bool_t +adjust_connections_for_cgroup (BusConnections *connections, + DBusString *cgroup, + int adjustment) +{ + int current_count; + char *cgroup_str; + + cgroup_str = _dbus_string_get_data (cgroup); + + current_count = get_connections_for_cgroup (connections, cgroup); + + _dbus_verbose ("Adjusting connection count for cgroup %s: " + "was %d adjustment %d making %d\n", + cgroup_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_cgroup, + cgroup_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_cgroup, + cgroup_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 (©)) + return FALSE; + + if (!_dbus_string_copy (cgroup, 0, ©, 0)) + { + _dbus_string_free (©); + return FALSE; + } + + if (!_dbus_string_steal_data (©, &cgroup_str)) + { + _dbus_string_free (©); + return FALSE; + } + + _dbus_string_free (©); + + retval = _dbus_hash_table_insert_string (connections->completed_by_cgroup, + cgroup_str, _DBUS_INT_TO_POINTER (current_count)); + if (!retval) + dbus_free (cgroup_str); + + return retval; + } +} + +static dbus_bool_t adjust_connections_for_uid (BusConnections *connections, dbus_uid_t uid, int adjustment) @@ -346,6 +442,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_cgroup) + { + if (!adjust_connections_for_cgroup (d->connections, &d->cgroup, -1)) + _dbus_assert_not_reached ("adjusting downward should never fail"); } if (d->pid > 0) { @@ -365,6 +467,8 @@ bus_connection_disconnected (DBusConnection *connection) _dbus_assert (d->connections->n_completed >= 0); } + _dbus_string_free (&d->cgroup); + bus_connection_drop_pending_replies (d->connections, connection); /* frees "d" as side effect */ @@ -500,11 +604,16 @@ bus_connections_new (BusContext *context) if (connections->completed_by_process == NULL) goto failed_3; + connections->completed_by_cgroup = _dbus_hash_table_new (DBUS_HASH_STRING, + dbus_free, NULL); + if (connections->completed_by_cgroup == 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 +622,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_cgroup); failed_4: _dbus_hash_table_unref (connections->completed_by_process); failed_3: @@ -595,6 +706,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_cgroup); dbus_free (connections); @@ -670,7 +782,7 @@ bus_connections_setup_connection (BusConnections *connections, BusConnectionData *d; dbus_bool_t retval; DBusError error; - + dbus_bool_t cgroup_release_if_oom = FALSE; d = dbus_new0 (BusConnectionData, 1); @@ -735,6 +847,10 @@ bus_connections_setup_connection (BusConnections *connections, allow_unix_user_function, NULL, NULL); + if (!_dbus_string_init (&d->cgroup)) + goto out; + cgroup_release_if_oom = TRUE; + dbus_connection_set_dispatch_status_function (connection, dispatch_status_function, bus_context_get_loop (connections->context), @@ -812,6 +928,16 @@ bus_connections_setup_connection (BusConnections *connections, dbus_connection_set_unix_user_function (connection, NULL, NULL, NULL); + /* If d->cgroup 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 (cgroup_release_if_oom) + _dbus_string_free (&d->cgroup); + dbus_connection_set_windows_user_function (connection, NULL, NULL, NULL); @@ -1434,6 +1560,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_cgroup = FALSE; d = BUS_CONNECTION_DATA (connection); _dbus_assert (d != NULL); @@ -1473,12 +1600,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_cgroup) + { + if (!adjust_connections_for_cgroup (d->connections, &d->cgroup, 1)) + goto fail; + rollback_cgroup = TRUE; + } + if (d->pid > 0) { if (!adjust_connections_for_pid (d->connections, d->pid, 1)) @@ -1515,6 +1648,8 @@ fail: adjust_connections_for_uid (d->connections, uid, -1); if (rollback_pid) adjust_connections_for_pid (d->connections, d->pid, -1); + if (rollback_cgroup) + adjust_connections_for_cgroup (d->connections, &d->cgroup, -1); dbus_free (d->name); d->name = NULL; if (d->policy) @@ -1581,6 +1716,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 +1741,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 +1763,38 @@ bus_connections_check_limits (BusConnections *connections, } } + if (!d->tracking_cgroup && d->pid > 0) + { + d->tracking_cgroup = TRUE; + + if (!_dbus_cgroup_for_pid (d->pid, &d->cgroup, 4096, error)) + { + _dbus_verbose ("Couldn't get connection's cgroup\n"); + dbus_error_free (error); + /* ignore the error: the connection will be accounted for cgroup "" + */ + d->tracking_cgroup = FALSE; + } + } + + _dbus_verbose ("Checking max_connections_per_cgroup? %d\n", d->tracking_cgroup); + if (d->tracking_cgroup) + { + _dbus_verbose ("Checking max_connections_per_cgroup: current count=%d max=%d\n", + get_connections_for_cgroup (connections, &d->cgroup), + bus_context_get_max_connections_per_cgroup (connections->context)); + + count = get_connections_for_cgroup (connections, &d->cgroup); + if (count >= + bus_context_get_max_connections_per_cgroup (connections->context)) + { + dbus_set_error (error, DBUS_ERROR_LIMITS_EXCEEDED, + "The maximum number of active connections for cgroup %s has been reached", + _dbus_string_get_data (&d->cgroup)); + return FALSE; + } + } + return TRUE; } -- 1.8.5.3