From 10ee1413f6205a20b34ed745633af3e4c69b0040 Mon Sep 17 00:00:00 2001 From: David Tardon Date: Fri, 23 Jun 2023 16:00:44 +0200 Subject: [PATCH 1/8] bus-polkit: use automatic cleanup --- src/shared/bus-polkit.c | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/src/shared/bus-polkit.c b/src/shared/bus-polkit.c index e3e327198c..879d3c2890 100644 --- a/src/shared/bus-polkit.c +++ b/src/shared/bus-polkit.c @@ -198,6 +198,8 @@ static AsyncPolkitQuery *async_polkit_query_free(AsyncPolkitQuery *q) { return mfree(q); } +DEFINE_TRIVIAL_CLEANUP_FUNC(AsyncPolkitQuery*, async_polkit_query_free); + static int async_polkit_defer(sd_event_source *s, void *userdata) { AsyncPolkitQuery *q = ASSERT_PTR(userdata); @@ -351,6 +353,7 @@ int bus_verify_polkit_async( #if ENABLE_POLKIT _cleanup_(sd_bus_message_unrefp) sd_bus_message *pk = NULL; + _cleanup_(async_polkit_query_freep) AsyncPolkitQuery *q_new = NULL; int c = sd_bus_message_get_allow_interactive_authorization(call); if (c < 0) @@ -366,7 +369,7 @@ int bus_verify_polkit_async( if (r < 0) return r; - q = new(AsyncPolkitQuery, 1); + q = q_new = new(AsyncPolkitQuery, 1); if (!q) return -ENOMEM; @@ -375,30 +378,24 @@ int bus_verify_polkit_async( }; q->action = strdup(action); - if (!q->action) { - async_polkit_query_free(q); + if (!q->action) return -ENOMEM; - } q->details = strv_copy((char**) details); - if (!q->details) { - async_polkit_query_free(q); + if (!q->details) return -ENOMEM; - } r = hashmap_put(*registry, call, q); - if (r < 0) { - async_polkit_query_free(q); + if (r < 0) return r; - } q->registry = *registry; r = sd_bus_call_async(call->bus, &q->slot, pk, async_polkit_callback, q, 0); - if (r < 0) { - async_polkit_query_free(q); + if (r < 0) return r; - } + + TAKE_PTR(q_new); return 0; #endif From 8f7195ef3c1caebf4d55d9436b32ff81cb928b6c Mon Sep 17 00:00:00 2001 From: David Tardon Date: Tue, 27 Jun 2023 14:39:33 +0200 Subject: [PATCH 2/8] bus-polkit: drop unused argument --- src/shared/bus-polkit.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/shared/bus-polkit.c b/src/shared/bus-polkit.c index 879d3c2890..904d775c17 100644 --- a/src/shared/bus-polkit.c +++ b/src/shared/bus-polkit.c @@ -266,7 +266,6 @@ static int process_polkit_response( sd_bus_message *call, const char *action, const char **details, - Hashmap **registry, sd_bus_error *ret_error) { int authorized, challenge, r; @@ -274,7 +273,6 @@ static int process_polkit_response( assert(q); assert(call); assert(action); - assert(registry); assert(ret_error); assert(q->action); @@ -342,7 +340,7 @@ int bus_verify_polkit_async( /* This is the second invocation of this function, and there's already a response from * polkit, let's process it */ if (q) - return process_polkit_response(q, call, action, details, registry, ret_error); + return process_polkit_response(q, call, action, details, ret_error); #endif r = sd_bus_query_sender_privilege(call, capability); From 0df66e42fab70a27bbf0475ab32526c033500491 Mon Sep 17 00:00:00 2001 From: David Tardon Date: Tue, 27 Jun 2023 14:52:57 +0200 Subject: [PATCH 3/8] bus-polkit: refactor a bit to avoid goto --- src/shared/bus-polkit.c | 31 ++++++++++++++++++++----------- 1 file changed, 20 insertions(+), 11 deletions(-) diff --git a/src/shared/bus-polkit.c b/src/shared/bus-polkit.c index 904d775c17..8bfae41e03 100644 --- a/src/shared/bus-polkit.c +++ b/src/shared/bus-polkit.c @@ -212,11 +212,11 @@ static int async_polkit_defer(sd_event_source *s, void *userdata) { return 0; } -static int async_polkit_callback(sd_bus_message *reply, void *userdata, sd_bus_error *error) { - AsyncPolkitQuery *q = ASSERT_PTR(userdata); +static int async_polkit_process_reply(sd_bus_message *reply, AsyncPolkitQuery *q) { int r; assert(reply); + assert(q); assert(q->slot); q->slot = sd_bus_slot_unref(q->slot); @@ -234,30 +234,39 @@ static int async_polkit_callback(sd_bus_message *reply, void *userdata, sd_bus_e assert(!q->defer_event_source); r = sd_event_add_defer(sd_bus_get_event(sd_bus_message_get_bus(reply)), &q->defer_event_source, async_polkit_defer, q); if (r < 0) - goto fail; + return r; r = sd_event_source_set_priority(q->defer_event_source, SD_EVENT_PRIORITY_IDLE); if (r < 0) - goto fail; + return r; r = sd_event_source_set_enabled(q->defer_event_source, SD_EVENT_ONESHOT); if (r < 0) - goto fail; + return r; r = sd_bus_message_rewind(q->request, true); if (r < 0) - goto fail; + return r; r = sd_bus_enqueue_for_read(sd_bus_message_get_bus(q->request), q->request); if (r < 0) - goto fail; + return r; return 1; +} -fail: - log_debug_errno(r, "Processing asynchronous PolicyKit reply failed, ignoring: %m"); - (void) sd_bus_reply_method_errno(q->request, r, NULL); - async_polkit_query_free(q); +static int async_polkit_callback(sd_bus_message *reply, void *userdata, sd_bus_error *error) { + AsyncPolkitQuery *q = ASSERT_PTR(userdata); + int r; + + assert(reply); + + r = async_polkit_process_reply(reply, q); + if (r < 0) { + log_debug_errno(r, "Processing asynchronous PolicyKit reply failed, ignoring: %m"); + (void) sd_bus_reply_method_errno(q->request, r, NULL); + async_polkit_query_free(q); + } return r; } From 361c4ee82f9be52570bcdc91d6b3ebe3fbadf2f3 Mon Sep 17 00:00:00 2001 From: David Tardon Date: Fri, 28 Apr 2023 16:31:27 +0200 Subject: [PATCH 4/8] bus-polkit: describe async. polkit verification --- src/shared/bus-polkit.c | 74 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 74 insertions(+) diff --git a/src/shared/bus-polkit.c b/src/shared/bus-polkit.c index 8bfae41e03..90f8d71f86 100644 --- a/src/shared/bus-polkit.c +++ b/src/shared/bus-polkit.c @@ -324,6 +324,80 @@ static int process_polkit_response( #endif +/* bus_verify_polkit_async() handles verification of D-Bus calls with polkit. Because the polkit API + * is asynchronous, the whole thing is a bit complex and requires some support in the code that uses + * it. It relies on sd-bus's support for interrupting the processing of a message. + * + * Requirements: + * + * * bus_verify_polkit_async() must be called before any changes to internal state. + * * If bus_verify_polkit_async() has made a new polkit query (signaled by return value 0), + * processing of the message should be interrupted. This is done by returning 1--which sd-bus + * handles specially--and is usually accompanied by a comment. (The message will be queued for + * processing again later when a reply from polkit is received.) + * * The code needs to keep a hashmap, here called registry, in which bus_verify_polkit_async() + * stores active queries. This hashmap's lifetime must be larger than the method handler's; + * e.g., it can be a member of some "manager" object or a global variable. + * + * Return value: + * + * * 0 - a new polkit call has been made, which means the processing of the message should be + * interrupted; + * * 1 - the action has been allowed; + * * -EACCES - the action has been denied; + * * < 0 - an unspecified error. + * + * A step-by-step description of how it works: + * + * 1. A D-Bus method handler calls bus_verify_polkit_async(), passing it the D-Bus message being + * processed and the polkit action to verify. + * 2. bus_verify_polkit_async() checks the registry for the message and action combination. Let's + * assume this is the first call, so it finds nothing. + * 3. A new AsyncPolkitQuery object is created and an async. D-Bus call to polkit is made. The + * function then returns 0. The method handler returns 1 to tell sd-bus that the processing of + * the message has been interrupted. + * 4. (Later) A reply from polkit is received and async_polkit_callback() is called. + * 5. async_polkit_callback() reads the reply and stores result into the passed query. + * 6. async_polkit_callback() enqueues the original message again. + * 7. (Later) The same D-Bus method handler is called for the same message. It calls + * bus_verify_polkit_async() again. + * 8. bus_verify_polkit_async() checks the registry for the message and action combination. It finds + * an existing query and returns its result. + * 9. The method handler continues processing of the message. + * + * Memory handling: + * + * async_polkit_callback() registers a deferred call of async_polkit_defer() for the query, which + * causes the query to be removed from the registry and freed. Deferred events are run with idle + * priority, so this will happen after processing of the D-Bus message, when the query is no longer + * needed. + * + * Schematically: + * + * (m - D-Bus message, a - polkit action, q - polkit query) + * + * -> foo_method(m) + * -> bus_verify_polkit_async(m, a) + * -> bus_call_method_async(q) + * <- bus_verify_polkit_async(m, a) = 0 + * <- foo_method(m) = 1 + * ... + * -> async_polkit_callback(q) + * -> sd_event_add_defer(async_polkit_defer, q) + * -> sd_bus_enqueue_for_read(m) + * <- async_polkit_callback(q) + * ... + * -> foo_method(m) + * -> bus_verify_polkit_async(m, a) + * <- bus_verify_polkit_async(m, a) = 1/-EACCES/error + * ... + * <- foo_method(m) + * ... + * -> async_polkit_defer(q) + * -> async_polkit_query_free(q) + * <- async_polkit_defer(q) + */ + int bus_verify_polkit_async( sd_bus_message *call, int capability, From c71901b247710c80bf79639d1e705270c812542e Mon Sep 17 00:00:00 2001 From: David Tardon Date: Thu, 29 Jun 2023 16:03:30 +0200 Subject: [PATCH 5/8] bus-polkit: extract action into a separate struct This is a preparation for later commits. --- src/shared/bus-polkit.c | 31 ++++++++++++++++++++++++------- 1 file changed, 24 insertions(+), 7 deletions(-) diff --git a/src/shared/bus-polkit.c b/src/shared/bus-polkit.c index 90f8d71f86..656da7a103 100644 --- a/src/shared/bus-polkit.c +++ b/src/shared/bus-polkit.c @@ -167,9 +167,23 @@ int bus_test_polkit( #if ENABLE_POLKIT -typedef struct AsyncPolkitQuery { +typedef struct AsyncPolkitQueryAction { char *action; char **details; +} AsyncPolkitQueryAction; + +static AsyncPolkitQueryAction *async_polkit_query_action_free(AsyncPolkitQueryAction *a) { + if (!a) + return NULL; + + free(a->action); + strv_free(a->details); + + return mfree(a); +} + +typedef struct AsyncPolkitQuery { + AsyncPolkitQueryAction *action; sd_bus_message *request, *reply; sd_bus_slot *slot; @@ -190,8 +204,7 @@ static AsyncPolkitQuery *async_polkit_query_free(AsyncPolkitQuery *q) { sd_bus_message_unref(q->request); sd_bus_message_unref(q->reply); - free(q->action); - strv_free(q->details); + async_polkit_query_action_free(q->action); sd_event_source_disable_unref(q->defer_event_source); @@ -285,12 +298,13 @@ static int process_polkit_response( assert(ret_error); assert(q->action); + assert(q->action->action); assert(q->reply); /* If the operation we want to authenticate changed between the first and the second time, * let's not use this authentication, it might be out of date as the object and context we * operate on might have changed. */ - if (!streq(q->action, action) || !strv_equal(q->details, (char**) details)) + if (!streq(q->action->action, action) || !strv_equal(q->action->details, (char**) details)) return -ESTALE; if (sd_bus_message_is_method_error(q->reply, NULL)) { @@ -458,12 +472,15 @@ int bus_verify_polkit_async( .request = sd_bus_message_ref(call), }; - q->action = strdup(action); + q->action = new(AsyncPolkitQueryAction, 1); if (!q->action) return -ENOMEM; - q->details = strv_copy((char**) details); - if (!q->details) + *q->action = (AsyncPolkitQueryAction) { + .action = strdup(action), + .details = strv_copy((char**) details), + }; + if (!q->action->action || !q->action->details) return -ENOMEM; r = hashmap_put(*registry, call, q); From 959301cf9f42418314abf027183dc25c08731b82 Mon Sep 17 00:00:00 2001 From: David Tardon Date: Thu, 29 Jun 2023 16:20:25 +0200 Subject: [PATCH 6/8] bus-polkit: parse reply from polkit on receive ... and store just the result. --- src/shared/bus-polkit.c | 121 +++++++++++++++++++++++++--------------- 1 file changed, 77 insertions(+), 44 deletions(-) diff --git a/src/shared/bus-polkit.c b/src/shared/bus-polkit.c index 656da7a103..2a61d9180d 100644 --- a/src/shared/bus-polkit.c +++ b/src/shared/bus-polkit.c @@ -182,14 +182,21 @@ static AsyncPolkitQueryAction *async_polkit_query_action_free(AsyncPolkitQueryAc return mfree(a); } +DEFINE_TRIVIAL_CLEANUP_FUNC(AsyncPolkitQueryAction*, async_polkit_query_action_free); + typedef struct AsyncPolkitQuery { AsyncPolkitQueryAction *action; - sd_bus_message *request, *reply; + sd_bus_message *request; sd_bus_slot *slot; Hashmap *registry; sd_event_source *defer_event_source; + + AsyncPolkitQueryAction *authorized_action; + AsyncPolkitQueryAction *denied_action; + AsyncPolkitQueryAction *error_action; + sd_bus_error error; } AsyncPolkitQuery; static AsyncPolkitQuery *async_polkit_query_free(AsyncPolkitQuery *q) { @@ -202,12 +209,17 @@ static AsyncPolkitQuery *async_polkit_query_free(AsyncPolkitQuery *q) { hashmap_remove(q->registry, q->request); sd_bus_message_unref(q->request); - sd_bus_message_unref(q->reply); async_polkit_query_action_free(q->action); sd_event_source_disable_unref(q->defer_event_source); + async_polkit_query_action_free(q->authorized_action); + async_polkit_query_action_free(q->denied_action); + async_polkit_query_action_free(q->error_action); + + sd_bus_error_free(&q->error); + return mfree(q); } @@ -225,6 +237,56 @@ static int async_polkit_defer(sd_event_source *s, void *userdata) { return 0; } +static int async_polkit_read_reply(sd_bus_message *reply, AsyncPolkitQuery *q) { + _cleanup_(async_polkit_query_action_freep) AsyncPolkitQueryAction *a = NULL; + int authorized, challenge, r; + + assert(reply); + assert(q); + + assert(q->action); + a = TAKE_PTR(q->action); + + if (sd_bus_message_is_method_error(reply, NULL)) { + const sd_bus_error *e; + + e = sd_bus_message_get_error(reply); + + /* Save error from polkit reply, so it can be returned when the same authorization is + * attempted for second time */ + if (!bus_error_is_unknown_service(e)) { + q->error_action = TAKE_PTR(a); + return sd_bus_error_copy(&q->error, e); + } + + /* Treat no PK available as access denied */ + q->denied_action = TAKE_PTR(a); + + return 0; + } + + r = sd_bus_message_enter_container(reply, 'r', "bba{ss}"); + if (r >= 0) + r = sd_bus_message_read(reply, "bb", &authorized, &challenge); + if (r < 0) + return r; + + assert(!q->authorized_action); + assert(!q->denied_action); + assert(!q->error_action); + assert(!sd_bus_error_is_set(&q->error)); + + if (authorized) + q->authorized_action = TAKE_PTR(a); + else if (challenge) { + q->error_action = TAKE_PTR(a); + return sd_bus_error_set(&q->error, SD_BUS_ERROR_INTERACTIVE_AUTHORIZATION_REQUIRED, "Interactive authentication required."); + } else + q->denied_action = TAKE_PTR(a); + + return 0; +} + static int async_polkit_process_reply(sd_bus_message *reply, AsyncPolkitQuery *q) { int r; @@ -234,8 +296,9 @@ static int async_polkit_process_reply(sd_bus_message *reply, AsyncPolkitQuery *q assert(q->slot); q->slot = sd_bus_slot_unref(q->slot); - assert(!q->reply); - q->reply = sd_bus_message_ref(reply); + r = async_polkit_read_reply(reply, q); + if (r < 0) + return r; /* Now, let's dispatch the original message a second time be re-enqueing. This will then traverse the * whole message processing again, and thus re-validating and re-retrieving the "userdata" field @@ -283,56 +346,26 @@ static int async_polkit_callback(sd_bus_message *reply, void *userdata, sd_bus_e return r; } -static int process_polkit_response( +_pure_ static int async_polkit_query_check_action( AsyncPolkitQuery *q, - sd_bus_message *call, const char *action, const char **details, sd_bus_error *ret_error) { - int authorized, challenge, r; - assert(q); - assert(call); assert(action); assert(ret_error); - assert(q->action); - assert(q->action->action); - assert(q->reply); + if (q->authorized_action) + /* If the operation we want to authenticate changed between the first and the second time, + * let's not use this authentication, it might be out of date as the object and context we + * operate on might have changed. */ + return streq(q->authorized_action->action, action) && strv_equal(q->action->details, (char**) details) ? 1 : -ESTALE; - /* If the operation we want to authenticate changed between the first and the second time, - * let's not use this authentication, it might be out of date as the object and context we - * operate on might have changed. */ - if (!streq(q->action->action, action) || !strv_equal(q->action->details, (char**) details)) - return -ESTALE; - - if (sd_bus_message_is_method_error(q->reply, NULL)) { - const sd_bus_error *e; - - e = sd_bus_message_get_error(q->reply); - - /* Treat no PK available as access denied */ - if (bus_error_is_unknown_service(e)) - return -EACCES; - - /* Copy error from polkit reply */ - sd_bus_error_copy(ret_error, e); - return -sd_bus_error_get_errno(e); - } - - r = sd_bus_message_enter_container(q->reply, 'r', "bba{ss}"); - if (r >= 0) - r = sd_bus_message_read(q->reply, "bb", &authorized, &challenge); - if (r < 0) - return r; - - if (authorized) - return 1; - - if (challenge) - return sd_bus_error_set(ret_error, SD_BUS_ERROR_INTERACTIVE_AUTHORIZATION_REQUIRED, "Interactive authentication required."); + if (q->error_action) + return sd_bus_error_copy(ret_error, &q->error); + assert(q->denied_action); return -EACCES; } @@ -437,7 +470,7 @@ int bus_verify_polkit_async( /* This is the second invocation of this function, and there's already a response from * polkit, let's process it */ if (q) - return process_polkit_response(q, call, action, details, ret_error); + return async_polkit_query_check_action(q, action, details, ret_error); #endif r = sd_bus_query_sender_privilege(call, capability); From 2f50a4f38fd347364f3072eb820cea2b91420543 Mon Sep 17 00:00:00 2001 From: David Tardon Date: Thu, 29 Jun 2023 16:35:21 +0200 Subject: [PATCH 7/8] bus-polkit: allow to auth. a bus call for multiple actions In #20155, verify_shutdown_creds() needs to authenticate for both org.freedesktop.login1.hibernate-multiple-sessions and org.freedesktop.login1.hibernate-ignore-inhibit . Previously, the second authentication attempt would fail with -ESTALE. Fixes #20155. --- src/shared/bus-polkit.c | 140 +++++++++++++++++++++++++--------------- 1 file changed, 87 insertions(+), 53 deletions(-) diff --git a/src/shared/bus-polkit.c b/src/shared/bus-polkit.c index 2a61d9180d..cc90d82c8b 100644 --- a/src/shared/bus-polkit.c +++ b/src/shared/bus-polkit.c @@ -170,6 +170,8 @@ int bus_test_polkit( typedef struct AsyncPolkitQueryAction { char *action; char **details; + + LIST_FIELDS(struct AsyncPolkitQueryAction, authorized); } AsyncPolkitQueryAction; static AsyncPolkitQueryAction *async_polkit_query_action_free(AsyncPolkitQueryAction *a) { @@ -185,6 +187,8 @@ static AsyncPolkitQueryAction *async_polkit_query_action_free(AsyncPolkitQueryAc DEFINE_TRIVIAL_CLEANUP_FUNC(AsyncPolkitQueryAction*, async_polkit_query_action_free); typedef struct AsyncPolkitQuery { + unsigned n_ref; + AsyncPolkitQueryAction *action; sd_bus_message *request; @@ -193,13 +197,15 @@ typedef struct AsyncPolkitQuery { Hashmap *registry; sd_event_source *defer_event_source; - AsyncPolkitQueryAction *authorized_action; + LIST_HEAD(AsyncPolkitQueryAction, authorized_actions); AsyncPolkitQueryAction *denied_action; AsyncPolkitQueryAction *error_action; sd_bus_error error; } AsyncPolkitQuery; static AsyncPolkitQuery *async_polkit_query_free(AsyncPolkitQuery *q) { + AsyncPolkitQueryAction *a; + if (!q) return NULL; @@ -214,7 +220,11 @@ static AsyncPolkitQuery *async_polkit_query_free(AsyncPolkitQuery *q) { sd_event_source_disable_unref(q->defer_event_source); - async_polkit_query_action_free(q->authorized_action); + while ((a = q->authorized_actions)) { + LIST_REMOVE(authorized, q->authorized_actions, a); + async_polkit_query_action_free(a); + } + async_polkit_query_action_free(q->denied_action); async_polkit_query_action_free(q->error_action); @@ -223,7 +233,8 @@ static AsyncPolkitQuery *async_polkit_query_free(AsyncPolkitQuery *q) { return mfree(q); } -DEFINE_TRIVIAL_CLEANUP_FUNC(AsyncPolkitQuery*, async_polkit_query_free); +DEFINE_PRIVATE_TRIVIAL_REF_UNREF_FUNC(AsyncPolkitQuery, async_polkit_query, async_polkit_query_free); +DEFINE_TRIVIAL_CLEANUP_FUNC(AsyncPolkitQuery*, async_polkit_query_unref); static int async_polkit_defer(sd_event_source *s, void *userdata) { AsyncPolkitQuery *q = ASSERT_PTR(userdata); @@ -233,7 +244,7 @@ static int async_polkit_defer(sd_event_source *s, void *userdata) { /* This is called as idle event source after we processed the async polkit reply, hopefully after the * method call we re-enqueued has been properly processed. */ - async_polkit_query_free(q); + async_polkit_query_unref(q); return 0; } @@ -271,13 +282,14 @@ static int async_polkit_read_reply(sd_bus_message *reply, AsyncPolkitQuery *q) { if (r < 0) return r; - assert(!q->authorized_action); + /* It's currently expected that processing of a DBus message shall be interrupted on the first + * auth. error */ assert(!q->denied_action); assert(!q->error_action); assert(!sd_bus_error_is_set(&q->error)); if (authorized) - q->authorized_action = TAKE_PTR(a); + LIST_PREPEND(authorized, q->authorized_actions, TAKE_PTR(a)); else if (challenge) { q->error_action = TAKE_PTR(a); return sd_bus_error_set(&q->error, SD_BUS_ERROR_INTERACTIVE_AUTHORIZATION_REQUIRED, "Interactive authentication required."); @@ -307,14 +319,19 @@ static int async_polkit_process_reply(sd_bus_message *reply, AsyncPolkitQuery *q * We install an idle event loop event to clean-up the PolicyKit request data when we are idle again, * i.e. after the second time the message is processed is complete. */ - assert(!q->defer_event_source); - r = sd_event_add_defer(sd_bus_get_event(sd_bus_message_get_bus(reply)), &q->defer_event_source, async_polkit_defer, q); - if (r < 0) - return r; + if (!q->defer_event_source) { + r = sd_event_add_defer( + sd_bus_get_event(sd_bus_message_get_bus(reply)), + &q->defer_event_source, + async_polkit_defer, + q); + if (r < 0) + return r; - r = sd_event_source_set_priority(q->defer_event_source, SD_EVENT_PRIORITY_IDLE); - if (r < 0) - return r; + r = sd_event_source_set_priority(q->defer_event_source, SD_EVENT_PRIORITY_IDLE); + if (r < 0) + return r; + } r = sd_event_source_set_enabled(q->defer_event_source, SD_EVENT_ONESHOT); if (r < 0) @@ -341,7 +358,7 @@ static int async_polkit_callback(sd_bus_message *reply, void *userdata, sd_bus_e if (r < 0) { log_debug_errno(r, "Processing asynchronous PolicyKit reply failed, ignoring: %m"); (void) sd_bus_reply_method_errno(q->request, r, NULL); - async_polkit_query_free(q); + async_polkit_query_unref(q); } return r; } @@ -356,17 +373,17 @@ _pure_ static int async_polkit_query_check_action( assert(action); assert(ret_error); - if (q->authorized_action) - /* If the operation we want to authenticate changed between the first and the second time, - * let's not use this authentication, it might be out of date as the object and context we - * operate on might have changed. */ - return streq(q->authorized_action->action, action) && strv_equal(q->action->details, (char**) details) ? 1 : -ESTALE; + LIST_FOREACH(authorized, a, q->authorized_actions) + if (streq(a->action, action) && strv_equal(a->details, (char**) details)) + return 1; - if (q->error_action) + if (q->error_action && streq(q->error_action->action, action)) return sd_bus_error_copy(ret_error, &q->error); - assert(q->denied_action); - return -EACCES; + if (q->denied_action && streq(q->denied_action->action, action)) + return -EACCES; + + return 0; } #endif @@ -396,21 +413,26 @@ _pure_ static int async_polkit_query_check_action( * * A step-by-step description of how it works: * - * 1. A D-Bus method handler calls bus_verify_polkit_async(), passing it the D-Bus message being - * processed and the polkit action to verify. - * 2. bus_verify_polkit_async() checks the registry for the message and action combination. Let's - * assume this is the first call, so it finds nothing. - * 3. A new AsyncPolkitQuery object is created and an async. D-Bus call to polkit is made. The - * function then returns 0. The method handler returns 1 to tell sd-bus that the processing of + * 1. A D-Bus method handler calls bus_verify_polkit_async(), passing it the D-Bus message being + * processed and the polkit action to verify. + * 2. bus_verify_polkit_async() checks the registry for an existing query object associated with the + * message. Let's assume this is the first call, so it finds nothing. + * 3. A new AsyncPolkitQuery object is created and an async. D-Bus call to polkit is made. The + * function then returns 0. The method handler returns 1 to tell sd-bus that the processing of * the message has been interrupted. - * 4. (Later) A reply from polkit is received and async_polkit_callback() is called. - * 5. async_polkit_callback() reads the reply and stores result into the passed query. - * 6. async_polkit_callback() enqueues the original message again. - * 7. (Later) The same D-Bus method handler is called for the same message. It calls - * bus_verify_polkit_async() again. - * 8. bus_verify_polkit_async() checks the registry for the message and action combination. It finds - * an existing query and returns its result. - * 9. The method handler continues processing of the message. + * 4. (Later) A reply from polkit is received and async_polkit_callback() is called. + * 5. async_polkit_callback() reads the reply and stores its result in the passed query. + * 6. async_polkit_callback() enqueues the original message again. + * 7. (Later) The same D-Bus method handler is called for the same message. It calls + * bus_verify_polkit_async() again. + * 8. bus_verify_polkit_async() checks the registry for an existing query object associated with the + * message. It finds one and returns the result for the action. + * 9. The method handler continues processing of the message. If there's another action that needs + * to be verified: + * 10. bus_verify_polkit_async() is called again for the new action. The registry already contains a + * query for the message, but the new action hasn't been seen yet, hence steps 4-8 are repeated. + * 11. (In the method handler again.) bus_verify_polkit_async() returns query results for both + * actions and the processing continues as in step 9. * * Memory handling: * @@ -425,6 +447,7 @@ _pure_ static int async_polkit_query_check_action( * * -> foo_method(m) * -> bus_verify_polkit_async(m, a) + * -> async_polkit_query_ref(q) * -> bus_call_method_async(q) * <- bus_verify_polkit_async(m, a) = 0 * <- foo_method(m) = 1 @@ -438,10 +461,11 @@ _pure_ static int async_polkit_query_check_action( * -> bus_verify_polkit_async(m, a) * <- bus_verify_polkit_async(m, a) = 1/-EACCES/error * ... + * // possibly another call to bus_verify_polkit_async with action a2 * <- foo_method(m) * ... * -> async_polkit_defer(q) - * -> async_polkit_query_free(q) + * -> async_polkit_query_unref(q) * <- async_polkit_defer(q) */ @@ -467,10 +491,13 @@ int bus_verify_polkit_async( #if ENABLE_POLKIT AsyncPolkitQuery *q = hashmap_get(*registry, call); - /* This is the second invocation of this function, and there's already a response from - * polkit, let's process it */ - if (q) - return async_polkit_query_check_action(q, action, details, ret_error); + /* This is a repeated invocation of this function, hence let's check if we've already got + * a response from polkit for this action */ + if (q) { + r = async_polkit_query_check_action(q, action, details, ret_error); + if (r != 0) + return r; + } #endif r = sd_bus_query_sender_privilege(call, capability); @@ -481,7 +508,7 @@ int bus_verify_polkit_async( #if ENABLE_POLKIT _cleanup_(sd_bus_message_unrefp) sd_bus_message *pk = NULL; - _cleanup_(async_polkit_query_freep) AsyncPolkitQuery *q_new = NULL; + _cleanup_(async_polkit_query_unrefp) AsyncPolkitQuery *q_new = NULL; int c = sd_bus_message_get_allow_interactive_authorization(call); if (c < 0) @@ -497,14 +524,19 @@ int bus_verify_polkit_async( if (r < 0) return r; - q = q_new = new(AsyncPolkitQuery, 1); - if (!q) - return -ENOMEM; + if (!q) { + q = q_new = new(AsyncPolkitQuery, 1); + if (!q) + return -ENOMEM; - *q = (AsyncPolkitQuery) { - .request = sd_bus_message_ref(call), - }; + *q = (AsyncPolkitQuery) { + .n_ref = 1, + .request = sd_bus_message_ref(call), + }; + } else + async_polkit_query_ref(q); + assert(!q->action); q->action = new(AsyncPolkitQueryAction, 1); if (!q->action) return -ENOMEM; @@ -516,11 +548,13 @@ int bus_verify_polkit_async( if (!q->action->action || !q->action->details) return -ENOMEM; - r = hashmap_put(*registry, call, q); - if (r < 0) - return r; + if (!q->registry) { + r = hashmap_put(*registry, call, q); + if (r < 0) + return r; - q->registry = *registry; + q->registry = *registry; + } r = sd_bus_call_async(call->bus, &q->slot, pk, async_polkit_callback, q, 0); if (r < 0) @@ -536,7 +570,7 @@ int bus_verify_polkit_async( Hashmap *bus_verify_polkit_async_registry_free(Hashmap *registry) { #if ENABLE_POLKIT - return hashmap_free_with_destructor(registry, async_polkit_query_free); + return hashmap_free_with_destructor(registry, async_polkit_query_unref); #else assert(hashmap_isempty(registry)); return hashmap_free(registry); From b1ebc20137e40e267ea3a51c01ada33d857b55a2 Mon Sep 17 00:00:00 2001 From: David Tardon Date: Tue, 27 Jun 2023 15:31:51 +0200 Subject: [PATCH 8/8] bus-polkit: avoid extra variable --- src/shared/bus-polkit.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/shared/bus-polkit.c b/src/shared/bus-polkit.c index cc90d82c8b..b7f7c2b47a 100644 --- a/src/shared/bus-polkit.c +++ b/src/shared/bus-polkit.c @@ -490,7 +490,9 @@ int bus_verify_polkit_async( return r; #if ENABLE_POLKIT - AsyncPolkitQuery *q = hashmap_get(*registry, call); + _cleanup_(async_polkit_query_unrefp) AsyncPolkitQuery *q = NULL; + + q = async_polkit_query_ref(hashmap_get(*registry, call)); /* This is a repeated invocation of this function, hence let's check if we've already got * a response from polkit for this action */ if (q) { @@ -508,7 +510,6 @@ int bus_verify_polkit_async( #if ENABLE_POLKIT _cleanup_(sd_bus_message_unrefp) sd_bus_message *pk = NULL; - _cleanup_(async_polkit_query_unrefp) AsyncPolkitQuery *q_new = NULL; int c = sd_bus_message_get_allow_interactive_authorization(call); if (c < 0) @@ -525,7 +526,7 @@ int bus_verify_polkit_async( return r; if (!q) { - q = q_new = new(AsyncPolkitQuery, 1); + q = new(AsyncPolkitQuery, 1); if (!q) return -ENOMEM; @@ -533,8 +534,7 @@ int bus_verify_polkit_async( .n_ref = 1, .request = sd_bus_message_ref(call), }; - } else - async_polkit_query_ref(q); + } assert(!q->action); q->action = new(AsyncPolkitQueryAction, 1); @@ -560,7 +560,7 @@ int bus_verify_polkit_async( if (r < 0) return r; - TAKE_PTR(q_new); + TAKE_PTR(q); return 0; #endif