Hi Pauli,
On Sat, Mar 16, 2024 at 11:24 AM Pauli Virtanen <pav@xxxxxx> wrote:
>
> select_cb() callback is called when the sound server replies. However,
> at that point the ep or session for which it was made may already be
> gone if e.g. device disconnects or adapter is powered off.
>
> Track pending select_cb() callbacks, and mark them canceled when ep /
> session goes away.
Instead of marking them as canceled I guess we could actually cancel
the ongoing operations which shall resulting in calling
media_endpoint_cancel->dbus_pending_call_cancel which is what seems to
trigger the crash, so perhaps we need cancel select or make
pac->ops->clear cancel it implicitly, that said select doesn't involve
the creation of any stream so we might need to introduce
bt_bap_select_cancel which would invoke pac->ops->clear then
pac_clear->media_endpoint_cancel->dbus_pending_call_cancel, etc.
> Fixes crash:
>
> ==889897==ERROR: AddressSanitizer: heap-use-after-free
> READ of size 8 at 0x60400006b098 thread T0
> #0 0x55aeba in setup_new profiles/audio/bap.c:840
> #1 0x562158 in select_cb profiles/audio/bap.c:1361
> #2 0x47ad66 in pac_select_cb profiles/audio/media.c:920
> #3 0x47661b in endpoint_reply profiles/audio/media.c:375
> ...
> freed by thread T0 here:
> #0 0x7fd20bcd7fb8 in __interceptor_free.part.0
> #1 0x55f913 in ep_free profiles/audio/bap.c:1156
> #2 0x7d696e in remove_interface gdbus/object.c:660
> #3 0x7de622 in g_dbus_unregister_interface gdbus/object.c:1394
> #4 0x554536 in ep_unregister profiles/audio/bap.c:193
> #5 0x574455 in ep_remove profiles/audio/bap.c:2963
> #6 0x7f5341 in queue_remove_if src/shared/queue.c:279
> #7 0x7f5aba in queue_remove_all src/shared/queue.c:321
> #8 0x57452b in bap_disconnect profiles/audio/bap.c:2972
> #9 0x6cd107 in btd_service_disconnect src/service.c:305
> ...
> previously allocated by thread T0 here:
> #0 0x7fd20bcd92ef in malloc
> #1 0x7f6e98 in util_malloc src/shared/util.c:46
> #2 0x560d28 in ep_register profiles/audio/bap.c:1282
> #3 0x562bdf in pac_register profiles/audio/bap.c:1386
> #4 0x8cc834 in bap_foreach_pac src/shared/bap.c:4950
> #5 0x8cccfc in bt_bap_foreach_pac src/shared/bap.c:4964
> #6 0x56330b in bap_ready profiles/audio/bap.c:1457
> ...
> ---
> profiles/audio/bap.c | 81 ++++++++++++++++++++++++++++++++++++++------
> 1 file changed, 70 insertions(+), 11 deletions(-)
>
> diff --git a/profiles/audio/bap.c b/profiles/audio/bap.c
> index 964ba9c21..6c74760c2 100644
> --- a/profiles/audio/bap.c
> +++ b/profiles/audio/bap.c
> @@ -95,6 +95,12 @@ struct bap_ep {
> struct queue *setups;
> };
>
> +struct bap_select {
> + int ref_count;
> + struct bap_data *data;
> + struct bap_ep *ep;
> +};
> +
> struct bap_data {
> struct btd_device *device;
> struct btd_adapter *adapter;
> @@ -107,8 +113,8 @@ struct bap_data {
> struct queue *snks;
> struct queue *bcast;
> struct queue *streams;
> + struct queue *selecting;
> GIOChannel *listen_io;
> - int selecting;
> void *user_data;
> };
>
> @@ -194,6 +200,14 @@ static void ep_unregister(void *data)
> MEDIA_ENDPOINT_INTERFACE);
> }
>
> +static void bap_cancel_select(void *data)
> +{
> + struct bap_select *sel = data;
> +
> + sel->data = NULL;
> + sel->ep = NULL;
> +}
> +
> static void bap_data_free(struct bap_data *data)
> {
> if (data->listen_io) {
> @@ -210,6 +224,7 @@ static void bap_data_free(struct bap_data *data)
> queue_destroy(data->srcs, ep_unregister);
> queue_destroy(data->bcast, ep_unregister);
> queue_destroy(data->streams, NULL);
> + queue_destroy(data->selecting, bap_cancel_select);
> bt_bap_ready_unregister(data->bap, data->ready_id);
> bt_bap_state_unregister(data->bap, data->state_id);
> bt_bap_pac_unregister(data->bap, data->pac_id);
> @@ -1148,11 +1163,21 @@ static const GDBusMethodTable ep_methods[] = {
> { },
> };
>
> +static void ep_cancel_select(void *data, void *user_data)
> +{
> + struct bap_select *sel = data;
> +
> + if (sel->ep == user_data)
> + sel->ep = NULL;
> +}
> +
> static void ep_free(void *data)
> {
> struct bap_ep *ep = data;
> struct queue *setups = ep->setups;
>
> + queue_foreach(ep->data->selecting, ep_cancel_select, ep);
> +
> ep->setups = NULL;
> queue_destroy(setups, setup_free);
> free(ep->path);
> @@ -1348,16 +1373,29 @@ static void bap_config(void *data, void *user_data)
> queue_foreach(ep->setups, setup_config, NULL);
> }
>
> +static void select_count(void *data, void *user_data)
> +{
> + struct bap_select *sel = data;
> + int *count = user_data;
> +
> + *count += sel->ref_count;
> +}
> +
> static void select_cb(struct bt_bap_pac *pac, int err, struct iovec *caps,
> struct iovec *metadata, struct bt_bap_qos *qos,
> void *user_data)
> {
> - struct bap_ep *ep = user_data;
> + struct bap_select *sel = user_data;
> + struct bap_data *data = sel->data;
> + struct bap_ep *ep = sel->ep;
> struct bap_setup *setup;
> + int count = 0;
> +
> + if (!data || !ep)
> + err = ECANCELED;
>
> if (err) {
> error("err %d", err);
> - ep->data->selecting--;
> goto done;
> }
>
> @@ -1366,16 +1404,23 @@ static void select_cb(struct bt_bap_pac *pac, int err, struct iovec *caps,
> setup->metadata = util_iov_dup(metadata, 1);
> setup->qos = *qos;
>
> - DBG("selecting %d", ep->data->selecting);
> - ep->data->selecting--;
> + queue_foreach(data->selecting, select_count, &count);
> +
> + DBG("selecting %d", count);
>
> done:
> - if (ep->data->selecting)
> + if (sel->ref_count-- <= 1) {
> + if (data)
> + queue_remove(data->selecting, sel);
> + free(sel);
> + }
> +
> + if (!data || !queue_isempty(data->selecting))
> return;
>
> - queue_foreach(ep->data->srcs, bap_config, NULL);
> - queue_foreach(ep->data->snks, bap_config, NULL);
> - queue_foreach(ep->data->bcast, bap_config, NULL);
> + queue_foreach(data->srcs, bap_config, NULL);
> + queue_foreach(data->snks, bap_config, NULL);
> + queue_foreach(data->bcast, bap_config, NULL);
> }
>
> static bool pac_register(struct bt_bap_pac *lpac, struct bt_bap_pac *rpac,
> @@ -1420,8 +1465,21 @@ static bool pac_select(struct bt_bap_pac *lpac, struct bt_bap_pac *rpac,
> }
>
> /* TODO: Cache LRU? */
> - if (btd_service_is_initiator(service))
> - bt_bap_select(lpac, rpac, &ep->data->selecting, select_cb, ep);
> + if (btd_service_is_initiator(service)) {
> + struct bap_select *sel;
> +
> + sel = new0(struct bap_select, 1);
> + sel->ep = ep;
> + sel->data = ep->data;
> + sel->ref_count = 0;
> +
> + bt_bap_select(lpac, rpac, &sel->ref_count, select_cb, sel);
> +
> + if (sel->ref_count)
> + queue_push_tail(ep->data->selecting, sel);
> + else
> + free(sel);
> + }
>
> return true;
> }
> @@ -2495,6 +2553,7 @@ static struct bap_data *bap_data_new(struct btd_device *device)
> data->srcs = queue_new();
> data->snks = queue_new();
> data->bcast = queue_new();
> + data->selecting = queue_new();
>
> return data;
> }
> --
> 2.44.0
>
>
--
Luiz Augusto von Dentz
