From 791a38f3fa77d4cf381f9823e78b7e3a2badf64a Mon Sep 17 00:00:00 2001 From: Pauli Virtanen Date: Sat, 13 Feb 2021 15:35:36 +0200 Subject: [PATCH] media-session: fix sm_object ownership sm_object may be owned by either (i) monitors, created via sm_media_session_create/export*, or (ii) registry, via registry_global+bind_object. However, registry adds the objects to its globals list when their proxy appears, even if it does not own them. Only owner should call sm_object_destroy which unrefs obj->handle, because the sm_object structure is stored inside the handle's user_data and becomes invalid afterward. The sm_object_destroy call removes the object from the registry globals map, so if monitor calls first, there is no problem. However, sometimes the registry wins the race. Previously, registry did sm_object_destroy regardless of whether it owns the object or not, possibly causing the monitor's sm_object_destroy to refer to freed memory. This could cause segfaults, e.g. CARD=XX:XX:XX:XX:XX:XX bluetootctl connect $CARD while true; do pactl set-card-profile bluez_card.$CARD a2dp-sink; pactl set-card-profile bluez_card.$CARD off; done leads to a race between bluez5_remove_node and registry_global_remove, and problems appear when the latter wins. (As usual, if it doesn't segfault, a heisenbug appears instead.) Fix this by keeping track who owns the objects, and having registry destroy the objects only if it owns them. Otherwise, it just removes them from its lists. Also call pw_proxy_unref unconditionally in sm_object_destroy, so its asserts catch refcounting errors (although now there shouldn't be any). *** Another problem is conflict between bound_proxy and register_global, which generates duplicate objects with the same id. We resolve this by keeping the object not owned by the registry and discarding the other one. This fixes a memory leak, and possible consistency problems in session modules (due to session_create events for different objects with same id; now there will be paired session_remove ones in between). --- src/examples/media-session/media-session.c | 61 +++++++++++++++++----- src/examples/media-session/media-session.h | 2 + 2 files changed, 50 insertions(+), 13 deletions(-) diff --git a/src/examples/media-session/media-session.c b/src/examples/media-session/media-session.c index 5dac20d41..6f3cb0fce 100644 --- a/src/examples/media-session/media-session.c +++ b/src/examples/media-session/media-session.c @@ -188,11 +188,37 @@ struct object_info { void (*destroy) (void *object); }; +static void remove_object_and_destroy_if_owned(struct impl *impl, struct sm_object *obj); + static void add_object(struct impl *impl, struct sm_object *obj, uint32_t id) { size_t size = pw_map_get_size(&impl->globals); + struct sm_object *old_obj; + obj->id = id; pw_log_debug("add %u %p", obj->id, obj); + + if ((old_obj = pw_map_lookup(&impl->globals, id)) != NULL) { + /* Duplicate objects for same id: + * + * Can occur if registry_global arrives while another object is pending + * for bound_proxy for the same id. + * + * In this case, we should keep the object not owned by the registry, + * because it is referenced from elsewhere. + * + * Other cases are a bug, and we fail with assert. + */ + pw_log_debug(NAME " %p: duplicate objects for global '%d': removing old:%p owned_by_registry:%d (new:%p owned_by_registry:%d)", + impl, id, old_obj, old_obj->owned_by_registry, obj, obj->owned_by_registry); + + spa_assert(obj != old_obj); + spa_assert(!obj->owned_by_registry); + spa_assert(old_obj->owned_by_registry); + + remove_object_and_destroy_if_owned(impl, old_obj); + } + while (obj->id > size) pw_map_insert_at(&impl->globals, size++, NULL); pw_map_insert_at(&impl->globals, obj->id, obj); @@ -209,6 +235,16 @@ static void remove_object(struct impl *impl, struct sm_object *obj) obj->id = SPA_ID_INVALID; } +static void remove_object_and_destroy_if_owned(struct impl *impl, struct sm_object *obj) +{ + /* If object is owned by us, destroy it, otherwise just remove it */ + if (obj->owned_by_registry) { + sm_object_destroy(obj); + } else if (obj->id != SPA_ID_INVALID) { + remove_object(impl, obj); + } +} + static void *find_object(struct impl *impl, uint32_t id, const char *type) { struct sm_object *obj; @@ -319,11 +355,10 @@ int sm_object_destroy(struct sm_object *obj) } obj->proxy = NULL; - obj->handle = NULL; if (p) pw_proxy_unref(p); - if (h) - pw_proxy_unref(h); /* may free obj, if from init_object */ + + pw_proxy_unref(h); /* may free obj */ return 0; } @@ -1149,9 +1184,6 @@ static struct sm_object *init_object(struct impl *impl, const struct object_info if (info->init) info->init(obj); - if (id != SPA_ID_INVALID) - add_object(impl, obj, id); - return obj; } @@ -1182,7 +1214,7 @@ create_object(struct impl *impl, struct pw_proxy *proxy, struct pw_proxy *handle return obj; } -static struct sm_object * +static int bind_object(struct impl *impl, const struct object_info *info, uint32_t id, uint32_t permissions, const char *type, uint32_t version, const struct spa_dict *props) @@ -1191,6 +1223,8 @@ bind_object(struct impl *impl, const struct object_info *info, uint32_t id, struct pw_proxy *proxy; struct sm_object *obj; + spa_assert(id != SPA_ID_INVALID); + proxy = pw_registry_bind(impl->registry, id, type, info->version, info->size); if (proxy == NULL) { @@ -1198,15 +1232,16 @@ bind_object(struct impl *impl, const struct object_info *info, uint32_t id, goto error; } obj = init_object(impl, info, proxy, proxy, id, props); + obj->owned_by_registry = true; + add_object(impl, obj, id); pw_log_debug(NAME" %p: bound new object %p proxy %p id:%d", impl, obj, obj->proxy, obj->id); - return obj; + return 0; error: pw_log_warn(NAME" %p: can't handle global %d: %s", impl, id, spa_strerror(res)); - errno = -res; - return NULL; + return res; } static int @@ -1220,7 +1255,7 @@ update_object(struct impl *impl, const struct object_info *info, if (obj->proxy != NULL) return 0; - pw_log_debug(NAME" %p: update type:%s", impl, obj->type); + pw_log_debug(NAME" %p: update object:%p id:%d type:%s", impl, obj, obj->id, obj->type); obj->proxy = pw_registry_bind(impl->registry, id, info->type, info->version, 0); @@ -1396,7 +1431,7 @@ registry_global_remove(void *data, uint32_t id) if ((obj = find_object(impl, id, NULL)) == NULL) return; - sm_object_destroy(obj); + remove_object_and_destroy_if_owned(impl, obj); } static const struct pw_registry_events registry_events = { @@ -1966,7 +2001,7 @@ static void session_shutdown(struct impl *impl) sm_media_session_emit_shutdown(impl); spa_list_consume(obj, &impl->global_list, link) - sm_object_destroy(obj); + remove_object_and_destroy_if_owned(impl, obj); impl->this.metadata = NULL; diff --git a/src/examples/media-session/media-session.h b/src/examples/media-session/media-session.h index f3b85697d..4f6c6781f 100644 --- a/src/examples/media-session/media-session.h +++ b/src/examples/media-session/media-session.h @@ -83,6 +83,8 @@ struct sm_object { struct spa_callbacks methods; struct spa_list data; + + unsigned int owned_by_registry:1; }; int sm_object_add_listener(struct sm_object *obj, struct spa_hook *listener,