mirrors/pipewire
1
0
Fork 0
mirror of https://gitlab.freedesktop.org/pipewire/pipewire.git synced 2025-11-01 22:58:50 -04:00
Code Issues Projects Releases Packages Wiki Activity Actions

media-session: fix sm_object ownership

Browse source 
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).
This commit is contained in:
Pauli Virtanen 2021-02-13 15:35:36 +02:00 • committed by Wim Taymans
parent 0dee15d0f6
commit 791a38f3fa
2 changed files with 50 additions and 13 deletions
Download patch file Download diff file Expand all files Collapse all files

61
src/examples/media-session/media-session.c
View file

@ -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;

2
src/examples/media-session/media-session.h
View file

@ -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,