From 4b5fd19a7428e3ce42a00bef1336938bf64303fd Mon Sep 17 00:00:00 2001 From: Alban Crequy 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 #include +#ifdef HAVE_SYSTEMD +#include +#include +#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 (©)) + return FALSE; + + if (!_dbus_string_copy (systemd_unit, 0, ©, 0)) + { + _dbus_string_free (©); + return FALSE; + } + + if (!_dbus_string_steal_data (©, &systemd_unit_str)) + { + _dbus_string_free (©); + return FALSE; + } + + _dbus_string_free (©); + + 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