From 263cb2e736ba2eea80b1ab2c62ecf6d945d8ea0e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= Date: Thu, 3 Oct 2024 14:30:32 +0200 Subject: [PATCH] sbus: terminate ongoing chained requests if backend is restarted If there is an outgoing request already chained and backend is restarted, a new outgoing request is chained but not processed, it waits for a timeout. This patch makes sure that all outgoing requests are gracefully terminated if backend restarts. Resolves: https://github.com/SSSD/sssd/issues/7503 Reviewed-by: Alexey Tikhonov Reviewed-by: Justin Stephenson --- src/sbus/connection/sbus_connection.c | 17 +++++++++ src/sbus/connection/sbus_dispatcher.c | 1 + src/sbus/interface/sbus_std_signals.c | 5 +++ src/sbus/request/sbus_request.c | 13 +++++-- src/sbus/request/sbus_request_hash.c | 40 +++++++++++++++++++++ src/sbus/sbus.h | 10 ++++++ src/sbus/sbus_private.h | 10 ++++++ src/tests/system/tests/test_identity.py | 46 ++++++++++++++++++++++++- 8 files changed, 138 insertions(+), 4 deletions(-) diff --git a/src/sbus/connection/sbus_connection.c b/src/sbus/connection/sbus_connection.c index a5d11f67276..59fc2ee6b81 100644 --- a/src/sbus/connection/sbus_connection.c +++ b/src/sbus/connection/sbus_connection.c @@ -471,3 +471,20 @@ void sbus_connection_free(struct sbus_connection *conn) conn); } } + +void +sbus_connection_terminate_member_requests(struct sbus_connection *conn, + const char *member) +{ + DEBUG(SSSDBG_TRACE_FUNC, "Terminating outgoing chained requests for: %s\n", + member); + + sbus_requests_terminate_member(conn->requests->outgoing, member, + ERR_TERMINATED); + + DEBUG(SSSDBG_TRACE_FUNC, "Terminating incoming chained requests from: %s\n", + member); + + sbus_requests_terminate_member(conn->requests->incoming, member, + ERR_TERMINATED); +} diff --git a/src/sbus/connection/sbus_dispatcher.c b/src/sbus/connection/sbus_dispatcher.c index 3631c175472..7d954fe7901 100644 --- a/src/sbus/connection/sbus_dispatcher.c +++ b/src/sbus/connection/sbus_dispatcher.c @@ -38,6 +38,7 @@ sbus_dispatch_reconnect(struct sbus_connection *conn) /* Terminate all outgoing requests associated with this connection. */ DEBUG(SSSDBG_TRACE_FUNC, "Connection lost. Terminating active requests.\n"); sbus_requests_terminate_all(conn->requests->outgoing, ERR_TERMINATED); + sbus_requests_terminate_all(conn->requests->incoming, ERR_TERMINATED); switch (conn->type) { case SBUS_CONNECTION_CLIENT: diff --git a/src/sbus/interface/sbus_std_signals.c b/src/sbus/interface/sbus_std_signals.c index c9afe445d67..231765d4fa6 100644 --- a/src/sbus/interface/sbus_std_signals.c +++ b/src/sbus/interface/sbus_std_signals.c @@ -37,6 +37,11 @@ sbus_name_owner_changed(TALLOC_CTX *mem_ctx, /* Delete any existing sender information since it is now obsolete. */ sbus_senders_delete(conn->senders, name); + /* Terminate active request if the owner has disconnected. */ + if (new_owner == NULL || new_owner[0] == '\0') { + sbus_connection_terminate_member_requests(sbus_req->conn, old_owner); + } + return EOK; } diff --git a/src/sbus/request/sbus_request.c b/src/sbus/request/sbus_request.c index e0ea7c3a73e..845bc441fb9 100644 --- a/src/sbus/request/sbus_request.c +++ b/src/sbus/request/sbus_request.c @@ -449,6 +449,7 @@ static void sbus_incoming_request_sender_done(struct tevent_req *subreq) DBusMessageIter *write_iter = NULL; struct sbus_sender *sender; struct tevent_req *req; + const char *member = NULL; bool key_exists; errno_t ret; @@ -463,6 +464,9 @@ static void sbus_incoming_request_sender_done(struct tevent_req *subreq) } state->request->sender = talloc_steal(state->request, sender); + if (sender != NULL) { + member = sender->name; + } ret = sbus_check_access(state->conn, state->request); if (ret != EOK) { @@ -500,7 +504,7 @@ static void sbus_incoming_request_sender_done(struct tevent_req *subreq) * set a tevent callback that is triggered when the method handler is done. */ ret = sbus_requests_add(state->conn->requests->incoming, state->key, - state->conn, req, true, &key_exists); + state->conn, req, member, true, &key_exists); if (ret != EOK || key_exists) { /* Cancel the sub request. Since there was either an error or the * sub request was chained. */ @@ -617,6 +621,7 @@ sbus_outgoing_request_send(TALLOC_CTX *mem_ctx, struct sbus_outgoing_request_state *state; struct tevent_req *subreq; struct tevent_req *req; + const char *destination; bool key_exists; errno_t ret; @@ -645,6 +650,8 @@ sbus_outgoing_request_send(TALLOC_CTX *mem_ctx, } } + destination = dbus_message_get_destination(msg); + /** * We will search table to see if the same request is not already * in progress. If it is, we register ourselves for notification @@ -654,7 +661,7 @@ sbus_outgoing_request_send(TALLOC_CTX *mem_ctx, * set a tevent callback that is triggered when the method handler is done. */ ret = sbus_requests_add(conn->requests->outgoing, key, - conn, req, true, &key_exists); + conn, req, destination, true, &key_exists); if (ret != EOK) { goto done; } @@ -776,7 +783,7 @@ sbus_request_await_send(TALLOC_CTX *mem_ctx, /* Otherwise attach to this request. */ ret = sbus_requests_add(conn->requests->outgoing, key, conn, - req, false, NULL); + req, member, false, NULL); if (ret != EOK) { DEBUG(SSSDBG_CRIT_FAILURE, "Unable to attach to the request list " "[%d]: %s\n", ret, sss_strerror(ret)); diff --git a/src/sbus/request/sbus_request_hash.c b/src/sbus/request/sbus_request_hash.c index 0ddad03a87b..532c54795a4 100644 --- a/src/sbus/request/sbus_request_hash.c +++ b/src/sbus/request/sbus_request_hash.c @@ -118,6 +118,7 @@ sbus_requests_add(hash_table_t *table, const char *key, struct sbus_connection *conn, struct tevent_req *req, + const char *member, bool is_dbus, bool *_key_exists) { @@ -150,6 +151,11 @@ sbus_requests_add(hash_table_t *table, item->req = req; item->conn = conn; item->is_dbus = is_dbus; + item->member = talloc_strdup(item, member); + if (member != NULL && item->member == NULL) { + ret = ENOMEM; + goto done; + } ret = sbus_requests_attach_spies(item); if (ret != EOK) { @@ -323,3 +329,37 @@ sbus_requests_terminate_all(hash_table_t *table, talloc_free(values); } + +void +sbus_requests_terminate_member(hash_table_t *table, + const char *member, + errno_t error) +{ + struct sbus_request_list *list; + struct sbus_request_list *item; + hash_value_t *values; + unsigned long int num; + unsigned long int i; + int hret; + + hret = hash_values(table, &num, &values); + if (hret != HASH_SUCCESS) { + DEBUG(SSSDBG_CRIT_FAILURE, "Unable to get list of active requests " + "[%d]: %s\n", hret, hash_error_string(hret)); + return; + } + + for (i = 0; i < num; i++) { + list = sss_ptr_get_value(&values[i], struct sbus_request_list); + if ((member == NULL && list->member == NULL) + || (member != NULL && list->member != NULL && strcmp(member, list->member) == 0)) { + DLIST_FOR_EACH(item, list) { + sbus_requests_finish(item, error); + } + } + + sbus_requests_delete(list); + } + + talloc_free(values); +} diff --git a/src/sbus/sbus.h b/src/sbus/sbus.h index c5f564f20ac..9f17b91b747 100644 --- a/src/sbus/sbus.h +++ b/src/sbus/sbus.h @@ -336,6 +336,16 @@ errno_t sbus_connection_add_path_map(struct sbus_connection *conn, struct sbus_path *map); +/** + * Terminate all outgoing requests for given member. + * + * @param conn An sbus connection. + * @param member D-Bus member name (destination) + */ +void +sbus_connection_terminate_member_requests(struct sbus_connection *conn, + const char *member); + /** * Add new signal listener to the router. * diff --git a/src/sbus/sbus_private.h b/src/sbus/sbus_private.h index a55709086bb..b27a3b4c1c4 100644 --- a/src/sbus/sbus_private.h +++ b/src/sbus/sbus_private.h @@ -431,6 +431,9 @@ struct sbus_request_list { struct tevent_req *req; struct sbus_connection *conn; + /* Member part of the key. Destination for outgoing, sender for incoming.*/ + const char *member; + bool is_invalid; bool is_dbus; @@ -462,6 +465,7 @@ sbus_requests_add(hash_table_t *table, const char *key, struct sbus_connection *conn, struct tevent_req *req, + const char *member, bool is_dbus, bool *_key_exists); @@ -484,6 +488,12 @@ void sbus_requests_terminate_all(hash_table_t *table, errno_t error); +/* Terminate requests associated with given member. */ +void +sbus_requests_terminate_member(hash_table_t *table, + const char *member, + errno_t error); + /* Create new sbus request. */ struct sbus_request * sbus_request_create(TALLOC_CTX *mem_ctx, diff --git a/src/tests/system/tests/test_identity.py b/src/tests/system/tests/test_identity.py index bf50a7d0403..e92d10c016c 100644 --- a/src/tests/system/tests/test_identity.py +++ b/src/tests/system/tests/test_identity.py @@ -12,7 +12,8 @@ from sssd_test_framework.roles.client import Client from sssd_test_framework.roles.generic import GenericADProvider, GenericProvider from sssd_test_framework.roles.ipa import IPA -from sssd_test_framework.topology import KnownTopologyGroup +from sssd_test_framework.roles.ldap import LDAP +from sssd_test_framework.topology import KnownTopology, KnownTopologyGroup @pytest.mark.importance("critical") @@ -681,3 +682,46 @@ def test_identity__lookup_when_auto_private_groups_is_set_to_hybrid(client: Clie assert result is not None, "User 'user_group_gid' not found!" assert result.gid == 555555, "gid does not match expected value!" assert client.tools.getent.group(555555) is not None, "auto private group not found!" + + +@pytest.mark.importance("low") +@pytest.mark.topology(KnownTopology.LDAP) +def test_identity__lookup_when_backend_restarts(client: Client, ldap: LDAP): + """ + :title: Look up user when backend is restarted with previous lookup unfinished + :description: + If there is an active lookup for a user and the backend is restarted + before this lookup is finished, the next lookup of the same user after + the restart must not timeout. + :setup: + 1. Add a user "tuser" + 2. Start SSSD + :steps: + 1. Add 10s network traffic delay to the LDAP host + 2. Lookup "tuser" asynchronously + 3. Kill sssd_be with SIGKILL so it is restarted + 4. Remove the network traffic delay + 5. Lookup of "tuser" must yield the user and not timeout + :expectedresults: + 1. Network traffic is delayed + 2. Lookup hangs, does not finish and waits for a timeout + 3. The backend process is restarted + 4. Network traffic is no longer delayed + 5. User lookup returns the user immediately + :customerscenario: False + """ + ldap.user("tuser").add() + + client.sssd.start() + + # Add a delay so the next lookup will hang + client.tc.add_delay(ldap, "10s") + client.host.conn.async_run("getent passwd tuser") + + # Kill backend and remove the delay + client.host.conn.run("kill -KILL $(pidof sssd_be)") + client.tc.remove_delay(ldap) + + # The next lookup should not timeout + result = client.tools.wait_for_condition("getent passwd tuser", timeout=5) + assert "tuser" in result.stdout, "tuser was not found"