Hi Luiz,
su, 2024-03-17 kello 14:59 -0400, Luiz Augusto von Dentz kirjoitti:
> 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.
This probably needs adding pac_ops->cancel, and pac_ops->select shall
return unique id by which pac_ops->cancel can remove requests.
Removing all requests for LPAC e.g. as pac_ops->clear side effect
probably doesn't work, there can be simultaneous select() on same LPAC
for different devices and it should not cancel those.
I did not want to start with API redesign here, so it was not done like
that here.
>
> > 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
> >
> >
>
>
