From 19e3e20c47398b1a2cd5f2ce027ca27e905f68e7 Mon Sep 17 00:00:00 2001 From: Wim Taymans Date: Fri, 6 May 2022 13:23:24 +0200 Subject: [PATCH] protocol-native: improve client and resource refcounts Add a refcount to resource and client so that we can keep them alive while the native protocol is using them. One problem might be that the protocol destroys the client or resource while handling it and that would cause errors. Fixes #565 --- src/modules/module-protocol-native.c | 26 +++++++++----- src/pipewire/impl-client.c | 48 +++++++++++++++++-------- src/pipewire/private.h | 7 ++++ src/pipewire/resource.c | 52 ++++++++++++++++++++++++---- src/pipewire/resource.h | 4 +++ 5 files changed, 108 insertions(+), 29 deletions(-) diff --git a/src/modules/module-protocol-native.c b/src/modules/module-protocol-native.c index 3295ed4fc..6869f5c1d 100644 --- a/src/modules/module-protocol-native.c +++ b/src/modules/module-protocol-native.c @@ -347,9 +347,12 @@ process_messages(struct client_data *data) continue; } + resource->refcount++; pw_protocol_native_connection_enter(conn); res = demarshal[msg->opcode].func(resource, msg); pw_protocol_native_connection_leave(conn); + pw_resource_unref(resource); + if (res < 0) { pw_resource_errorf_id(resource, msg->id, res, "invalid message id:%u op:%u (%s)", @@ -360,6 +363,7 @@ process_messages(struct client_data *data) res = 0; done: context->current_client = NULL; + return res; error: @@ -387,14 +391,15 @@ client_busy_changed(void *data, bool busy) pw_loop_signal_event(s->loop, s->resume); } -static void handle_client_error(struct pw_impl_client *client, int res) +static void handle_client_error(struct pw_impl_client *client, int res, const char *msg) { if (res == -EPIPE || res == -ECONNRESET) - pw_log_info("%p: client %p disconnected", client->protocol, client); + pw_log_info("%p: %s: client %p disconnected", client->protocol, msg, client); else - pw_log_error("%p: client %p error %d (%s)", client->protocol, + pw_log_error("%p: %s: client %p error %d (%s)", client->protocol, msg, client, res, spa_strerror(res)); - pw_impl_client_destroy(client); + if (!client->destroyed) + pw_impl_client_destroy(client); } static void @@ -404,6 +409,8 @@ connection_data(void *data, int fd, uint32_t mask) struct pw_impl_client *client = this->client; int res; + client->refcount++; + if (mask & SPA_IO_HUP) { res = -EPIPE; goto error; @@ -425,9 +432,12 @@ connection_data(void *data, int fd, uint32_t mask) } else if (res != -EAGAIN) goto error; } +done: + pw_impl_client_unref(client); return; error: - handle_client_error(client, res); + handle_client_error(client, res, "connection_data"); + goto done; } static void client_free(void *data) @@ -1215,12 +1225,12 @@ static void do_resume(void *_data, uint64_t count) pw_log_debug("flush"); spa_list_for_each_safe(data, tmp, &this->client_list, protocol_link) { + data->client->refcount++; if ((res = process_messages(data)) < 0) - goto error; + handle_client_error(data->client, res, "do_resume"); + pw_impl_client_unref(data->client); } return; -error: - handle_client_error(data->client, res); } static const char * diff --git a/src/pipewire/impl-client.c b/src/pipewire/impl-client.c index cd566a566..f45e3827f 100644 --- a/src/pipewire/impl-client.c +++ b/src/pipewire/impl-client.c @@ -24,6 +24,7 @@ #include #include +#include #include @@ -415,6 +416,7 @@ struct pw_impl_client *pw_context_create_client(struct pw_impl_core *core, this = &impl->this; pw_log_debug("%p: new", this); + this->refcount = 1; this->context = core->context; this->core = core; this->protocol = protocol; @@ -584,6 +586,33 @@ static int destroy_resource(void *object, void *data) } +SPA_EXPORT +void pw_impl_client_unref(struct pw_impl_client *client) +{ + struct impl *impl = SPA_CONTAINER_OF(client, struct impl, this); + + assert(client->refcount > 0); + if (--client->refcount > 0) + return; + + pw_log_debug("%p: free", impl); + assert(client->destroyed); + + pw_impl_client_emit_free(client); + + spa_hook_list_clean(&client->listener_list); + + pw_map_clear(&client->objects); + pw_array_clear(&impl->permissions); + + spa_hook_remove(&impl->pool_listener); + pw_mempool_destroy(client->pool); + + pw_properties_free(client->properties); + + free(impl); +} + /** Destroy a client object * * \param client the client to destroy @@ -595,6 +624,10 @@ void pw_impl_client_destroy(struct pw_impl_client *client) struct impl *impl = SPA_CONTAINER_OF(client, struct impl, this); pw_log_debug("%p: destroy", client); + + assert(!client->destroyed); + client->destroyed = true; + pw_impl_client_emit_destroy(client); spa_hook_remove(&impl->context_listener); @@ -609,20 +642,7 @@ void pw_impl_client_destroy(struct pw_impl_client *client) pw_global_destroy(client->global); } - pw_log_debug("%p: free", impl); - pw_impl_client_emit_free(client); - - spa_hook_list_clean(&client->listener_list); - - pw_map_clear(&client->objects); - pw_array_clear(&impl->permissions); - - spa_hook_remove(&impl->pool_listener); - pw_mempool_destroy(client->pool); - - pw_properties_free(client->properties); - - free(impl); + pw_impl_client_unref(client); } SPA_EXPORT diff --git a/src/pipewire/private.h b/src/pipewire/private.h index 05c42f6c7..52629f4a3 100644 --- a/src/pipewire/private.h +++ b/src/pipewire/private.h @@ -308,6 +308,9 @@ struct pw_impl_client { unsigned int registered:1; unsigned int ucred_valid:1; /**< if the ucred member is valid */ unsigned int busy:1; + unsigned int destroyed:1; + + int refcount; /* v2 compatibility data */ void *compat_v2; @@ -945,8 +948,10 @@ struct pw_resource { const char *type; /**< type of the client interface */ uint32_t version; /**< version of the client interface */ uint32_t bound_id; /**< global id we are bound to */ + int refcount; unsigned int removed:1; /**< resource was removed from server */ + unsigned int destroyed:1; /**< resource was destroyed */ struct spa_hook_list listener_list; struct spa_hook_list object_listener_list; @@ -1266,6 +1271,8 @@ int pw_control_remove_link(struct pw_control_link *link); void pw_control_destroy(struct pw_control *control); +void pw_impl_client_unref(struct pw_impl_client *client); + #define PW_LOG_OBJECT_POD (1<<0) void pw_log_log_object(enum spa_log_level level, const char *file, int line, const char *func, uint32_t flags, const void *object); diff --git a/src/pipewire/resource.c b/src/pipewire/resource.c index efb82a41e..0a6231c2e 100644 --- a/src/pipewire/resource.c +++ b/src/pipewire/resource.c @@ -23,6 +23,7 @@ */ #include +#include #include "pipewire/private.h" #include "pipewire/protocol.h" @@ -57,6 +58,7 @@ struct pw_resource *pw_resource_new(struct pw_impl_client *client, return NULL; this = &impl->this; + this->refcount = 1; this->context = client->context; this->client = client; this->permissions = permissions; @@ -278,17 +280,58 @@ void pw_resource_error(struct pw_resource *resource, int res, const char *error) } } +SPA_EXPORT +void pw_resource_ref(struct pw_resource *resource) +{ + assert(resource->refcount > 0); + resource->refcount++; +} + +SPA_EXPORT +void pw_resource_unref(struct pw_resource *resource) +{ + assert(resource->refcount > 0); + if (--resource->refcount > 0) + return; + + pw_log_debug("%p: free %u", resource, resource->id); + assert(resource->destroyed); + +#if DEBUG_LISTENERS + { + struct spa_hook *h; + spa_list_for_each(h, &resource->object_listener_list.list, link) { + pw_log_warn("%p: resource %u: leaked object listener %p", + resource, resource->id, h); + break; + } + spa_list_for_each(h, &resource->listener_list.list, link) { + pw_log_warn("%p: resource %u: leaked listener %p", + resource, resource->id, h); + break; + } + } +#endif + spa_hook_list_clean(&resource->listener_list); + spa_hook_list_clean(&resource->object_listener_list); + + free(resource); +} + SPA_EXPORT void pw_resource_destroy(struct pw_resource *resource) { struct pw_impl_client *client = resource->client; + pw_log_debug("%p: destroy %u", resource, resource->id); + assert(!resource->destroyed); + resource->destroyed = true; + if (resource->global) { spa_list_remove(&resource->link); resource->global = NULL; } - pw_log_debug("%p: destroy %u", resource, resource->id); pw_resource_emit_destroy(resource); pw_map_insert_at(&client->objects, resource->id, NULL); @@ -297,12 +340,7 @@ void pw_resource_destroy(struct pw_resource *resource) if (client->core_resource && !resource->removed) pw_core_resource_remove_id(client->core_resource, resource->id); - pw_log_debug("%p: free %u", resource, resource->id); - - spa_hook_list_clean(&resource->listener_list); - spa_hook_list_clean(&resource->object_listener_list); - - free(resource); + pw_resource_unref(resource); } SPA_EXPORT diff --git a/src/pipewire/resource.h b/src/pipewire/resource.h index dd15c1a8f..24d458cb0 100644 --- a/src/pipewire/resource.h +++ b/src/pipewire/resource.h @@ -121,6 +121,10 @@ void pw_resource_add_object_listener(struct pw_resource *resource, * with the same \a sequence number in the return value. */ int pw_resource_ping(struct pw_resource *resource, int seq); +/** ref/unref a resource, Since 0.3.52 */ +void pw_resource_ref(struct pw_resource *resource); +void pw_resource_unref(struct pw_resource *resource); + /** Notify global id this resource is bound to */ int pw_resource_set_bound_id(struct pw_resource *resource, uint32_t global_id);