From d44bf0ffc0b8464727b5616cf2a3c702090417bb Mon Sep 17 00:00:00 2001 From: Pauli Virtanen Date: Mon, 8 Nov 2021 12:26:37 +0100 Subject: [PATCH] impl-core/protocol-native: use generation counter for global registry Some client messages have bare ids (as opposed to proxies/resources), eg. as in pw_registry_bind/destroy. If the client is processing messages late, these may refer to an object that was already removed, and the id may now refers to a differnt objects. I.e. the following race condition needs to be resolved: server client Global 1 (gen. 1) Global 1 Global 1 remove Global 1 (gen. 2) Bind/destroy 1 Where the client would bind/destroy the wrong global, since it did not yet see the messages for the second one. To keep track of which object the client means, the server keeps track of the "generation number" of its global registry, and what generation the client is at. Each global remembers at what generation of registry they were registered. When processing the messages that use bare ids, check the registry generation of the client, to know whether the message refers to a stale global that was already removed. Messages where client sends bare ids to server are: pw_registry_bind, pw_registry_destroy, metadata_set_property In pw_registry_* do the staleness check directly. Also add staleness check in pw_impl_client_check_permissions, so that also the metadata case is handled. The generation numbers are passed around in message footers, but only if they have changed. When the generation number changes on server, we send the updated value to the client in a message footer. When client has received an update value, it will send the value back in the footer of the next message it sends to the server. Based on: Wim Taymans "impl-core: check serial number" --- .../module-protocol-native/protocol-footer.c | 56 +++++++++++++++++++ .../module-protocol-native/protocol-footer.h | 8 ++- src/pipewire/global.c | 5 +- src/pipewire/impl-client.c | 3 + src/pipewire/impl-core.c | 19 +++++++ src/pipewire/private.h | 4 ++ 6 files changed, 91 insertions(+), 4 deletions(-) diff --git a/src/modules/module-protocol-native/protocol-footer.c b/src/modules/module-protocol-native/protocol-footer.c index 550114944..6b360a4b0 100644 --- a/src/modules/module-protocol-native/protocol-footer.c +++ b/src/modules/module-protocol-native/protocol-footer.c @@ -76,6 +76,17 @@ void marshal_proxy_footers(struct footer_proxy_global_state *state, struct pw_pr { struct footer_builder fb = FOOTER_BUILDER_INIT(builder); + if (proxy->core->recv_generation != state->last_recv_generation) { + state->last_recv_generation = proxy->core->recv_generation; + + pw_log_trace("core %p: send client registry generation:%"PRIu64, + proxy->core, proxy->core->recv_generation); + + start_footer_entry(&fb, FOOTER_RESOURCE_OPCODE_GENERATION); + spa_pod_builder_long(fb.builder, proxy->core->recv_generation); + end_footer_entry(&fb); + } + end_footer(&fb); } @@ -84,11 +95,56 @@ void marshal_resource_footers(struct footer_resource_global_state *state, struct { struct footer_builder fb = FOOTER_BUILDER_INIT(builder); + if (resource->context->generation != state->last_sent_generation) { + state->last_sent_generation = resource->context->generation; + + pw_log_trace("impl-client %p: send server registry generation:%"PRIu64, + resource->client, resource->context->generation); + + start_footer_entry(&fb, FOOTER_RESOURCE_OPCODE_GENERATION); + spa_pod_builder_long(fb.builder, resource->context->generation); + end_footer_entry(&fb); + } + end_footer(&fb); } +int demarshal_proxy_generation(void *object, struct spa_pod_parser *parser) +{ + struct pw_proxy *proxy = object; + int64_t generation; + + if (spa_pod_parser_get_long(parser, &generation) < 0) + return -EINVAL; + + proxy->core->recv_generation = (uint64_t)generation; + + pw_log_trace("core %p: recv server registry generation:%"PRIu64, + proxy->core, generation); + + return 0; +} + +int demarshal_resource_generation(void *object, struct spa_pod_parser *parser) +{ + struct pw_resource *resource = object; + int64_t generation; + + if (spa_pod_parser_get_long(parser, &generation) < 0) + return -EINVAL; + + resource->client->recv_generation = (uint64_t)generation; + + pw_log_trace("impl-client %p: recv client registry generation:%"PRIu64, + resource->client, generation); + + return 0; +} + const struct footer_demarshal footer_proxy_demarshal[FOOTER_PROXY_OPCODE_LAST] = { + [FOOTER_PROXY_OPCODE_GENERATION] = (struct footer_demarshal){ .demarshal = demarshal_proxy_generation }, }; const struct footer_demarshal footer_resource_demarshal[FOOTER_RESOURCE_OPCODE_LAST] = { + [FOOTER_RESOURCE_OPCODE_GENERATION] = (struct footer_demarshal){ .demarshal = demarshal_resource_generation }, }; diff --git a/src/modules/module-protocol-native/protocol-footer.h b/src/modules/module-protocol-native/protocol-footer.h index f6232d5e9..45d3a1d92 100644 --- a/src/modules/module-protocol-native/protocol-footer.h +++ b/src/modules/module-protocol-native/protocol-footer.h @@ -30,17 +30,21 @@ */ enum { - FOOTER_PROXY_OPCODE_LAST = 0, + FOOTER_PROXY_OPCODE_GENERATION = 0, + FOOTER_PROXY_OPCODE_LAST }; enum { - FOOTER_RESOURCE_OPCODE_LAST = 0, + FOOTER_RESOURCE_OPCODE_GENERATION = 0, + FOOTER_RESOURCE_OPCODE_LAST }; struct footer_proxy_global_state { + uint64_t last_recv_generation; }; struct footer_resource_global_state { + uint64_t last_sent_generation; }; struct footer_demarshal { diff --git a/src/pipewire/global.c b/src/pipewire/global.c index 6fcaf3669..e67053073 100644 --- a/src/pipewire/global.c +++ b/src/pipewire/global.c @@ -146,6 +146,8 @@ int pw_global_register(struct pw_global *global) spa_list_append(&context->global_list, &global->link); global->registered = true; + global->generation = ++context->generation; + spa_list_for_each(registry, &context->registry_resource_list, link) { uint32_t permissions = pw_global_get_permissions(global, registry->client); pw_log_debug("registry %p: global %d %08x serial:%"PRIu64, @@ -352,8 +354,7 @@ int pw_global_update_permissions(struct pw_global *global, struct pw_impl_client } else if (do_show) { pw_log_debug("client %p: resource %p show global %d serial:%"PRIu64, - client, resource, global->id, - global->serial); + client, resource, global->id, global->serial); pw_registry_resource_global(resource, global->id, new_permissions, diff --git a/src/pipewire/impl-client.c b/src/pipewire/impl-client.c index 7ccff283f..b4a3c2a2e 100644 --- a/src/pipewire/impl-client.c +++ b/src/pipewire/impl-client.c @@ -737,6 +737,9 @@ int pw_impl_client_check_permissions(struct pw_impl_client *client, if ((global = pw_context_find_global(context, global_id)) == NULL) return -ENOENT; + if (client->recv_generation != 0 && global->generation > client->recv_generation) + return -ESTALE; + perms = pw_global_get_permissions(global, client); if ((perms & permissions) != permissions) return -EPERM; diff --git a/src/pipewire/impl-core.c b/src/pipewire/impl-core.c index 85d41e7bb..8f0e37dc4 100644 --- a/src/pipewire/impl-core.c +++ b/src/pipewire/impl-core.c @@ -64,6 +64,9 @@ static void * registry_bind(void *object, uint32_t id, if (!PW_PERM_IS_R(permissions)) goto error_no_id; + if (resource->client->recv_generation != 0 && global->generation > resource->client->recv_generation) + goto error_stale_id; + if (!spa_streq(global->type, type)) goto error_wrong_interface; @@ -75,6 +78,12 @@ static void * registry_bind(void *object, uint32_t id, return NULL; +error_stale_id: + pw_log_debug("registry %p: not binding stale global " + "id %u to %u, generation:%"PRIu64" recv-generation:%"PRIu64, + resource, id, new_id, global->generation, resource->client->recv_generation); + pw_resource_errorf_id(resource, new_id, -ESTALE, "no global %u any more", id); + goto error_exit_clean; error_no_id: pw_log_debug("registry %p: no global with id %u to bind to %u", resource, id, new_id); pw_resource_errorf_id(resource, new_id, -ENOENT, "no global %u", id); @@ -110,6 +119,9 @@ static int registry_destroy(void *object, uint32_t id) if (!PW_PERM_IS_R(permissions)) goto error_no_id; + if (resource->client->recv_generation != 0 && global->generation > resource->client->recv_generation) + goto error_stale_id; + if (id == PW_ID_CORE || !PW_PERM_IS_X(permissions)) goto error_not_allowed; @@ -118,6 +130,13 @@ static int registry_destroy(void *object, uint32_t id) pw_global_destroy(global); return 0; +error_stale_id: + pw_log_debug("registry %p: not destroying stale global " + "id %u, generation:%"PRIu64" recv-generation:%"PRIu64, + resource, id, global->generation, resource->client->recv_generation); + pw_resource_errorf(resource, -ESTALE, "no global %u any more", id); + res = -ESTALE; + goto error_exit; error_no_id: pw_log_debug("registry %p: no global with id %u to destroy", resource, id); pw_resource_errorf(resource, -ENOENT, "no global %u", id); diff --git a/src/pipewire/private.h b/src/pipewire/private.h index 9434b8629..ea166dac0 100644 --- a/src/pipewire/private.h +++ b/src/pipewire/private.h @@ -299,6 +299,7 @@ struct pw_impl_client { struct pw_protocol *protocol; /**< protocol in use */ int recv_seq; /**< last received sequence number */ int send_seq; /**< last sender sequence number */ + uint64_t recv_generation; /**< last received registry generation */ void *user_data; /**< extra user data */ @@ -334,6 +335,7 @@ struct pw_global { pw_global_bind_func_t func; /**< bind function */ void *object; /**< object associated with the interface */ uint64_t serial; /**< increasing serial number */ + uint64_t generation; /**< registry generation number */ struct spa_list resource_list; /**< The list of resources of this global */ @@ -430,6 +432,7 @@ struct pw_context { uint64_t stamp; uint64_t serial; + uint64_t generation; /**< registry generation number */ struct pw_map globals; /**< map of globals */ struct spa_list core_impl_list; /**< list of core_imp */ @@ -1001,6 +1004,7 @@ struct pw_core { struct pw_protocol_client *conn; /**< the protocol client connection */ int recv_seq; /**< last received sequence number */ int send_seq; /**< last protocol result code */ + uint64_t recv_generation; /**< last received registry generation */ unsigned int removed:1; unsigned int destroyed:1;