Hi Pauli,
On Sat, Mar 23, 2024 at 5:40 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.
>
> Fix by implementing cancelling select() callbacks, and doing it before
> freeing ep.
>
> 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 | 22 ++++++++++++++++++++++
> profiles/audio/media.c | 30 ++++++++++++++++++++++++++++++
> 2 files changed, 52 insertions(+)
>
> diff --git a/profiles/audio/bap.c b/profiles/audio/bap.c
> index 315eff729..fdce275c9 100644
> --- a/profiles/audio/bap.c
> +++ b/profiles/audio/bap.c
> @@ -1148,11 +1148,15 @@ static const GDBusMethodTable ep_methods[] = {
> { },
> };
>
> +static void ep_cancel_select(struct bap_ep *ep);
> +
> static void ep_free(void *data)
> {
> struct bap_ep *ep = data;
> struct queue *setups = ep->setups;
>
> + ep_cancel_select(ep);
> +
> ep->setups = NULL;
> queue_destroy(setups, setup_free);
> free(ep->path);
> @@ -1426,6 +1430,24 @@ static bool pac_select(struct bt_bap_pac *lpac, struct bt_bap_pac *rpac,
> return true;
> }
>
> +static bool pac_cancel_select(struct bt_bap_pac *lpac, struct bt_bap_pac *rpac,
> + void *user_data)
> +{
> + struct bap_ep *ep = user_data;
> +
> + bt_bap_cancel_select(lpac, select_cb, ep);
> +
> + return true;
> +}
> +
> +static void ep_cancel_select(struct bap_ep *ep)
> +{
> + struct bt_bap *bap = ep->data->bap;
> +
> + bt_bap_foreach_pac(bap, BT_BAP_SOURCE, pac_cancel_select, ep);
> + bt_bap_foreach_pac(bap, BT_BAP_SINK, pac_cancel_select, ep);
> +}
> +
> static bool pac_found_bcast(struct bt_bap_pac *lpac, struct bt_bap_pac *rpac,
> void *user_data)
> {
> diff --git a/profiles/audio/media.c b/profiles/audio/media.c
> index edaff7867..3c5daf782 100644
> --- a/profiles/audio/media.c
> +++ b/profiles/audio/media.c
> @@ -149,6 +149,11 @@ static void media_endpoint_cancel(struct endpoint_request *request)
> {
> struct media_endpoint *endpoint = request->endpoint;
>
> + DBG("Canceling %s: name = %s path = %s",
> + dbus_message_get_member(request->msg),
> + dbus_message_get_destination(request->msg),
> + dbus_message_get_path(request->msg));
> +
> if (request->call)
> dbus_pending_call_cancel(request->call);
>
> @@ -1028,6 +1033,30 @@ static int pac_select(struct bt_bap_pac *lpac, struct bt_bap_pac *rpac,
> data, free);
> }
>
> +static void pac_cancel_select(struct bt_bap_pac *lpac, bt_bap_pac_select_t cb,
> + void *cb_data, void *user_data)
> +{
> + struct media_endpoint *endpoint = user_data;
> + GSList *l;
> +
> + for (l = endpoint->requests; l; l = g_slist_next(l)) {
> + struct endpoint_request *req = l->data;
> + struct pac_select_data *data;
> +
> + if (req->cb != pac_select_cb)
> + continue;
> +
> + data = req->user_data;
> + if (data->pac != lpac || data->cb != cb ||
> + data->user_data != cb_data)
> + continue;
> +
> + req->cb = NULL;
> + media_endpoint_cancel(req);
> + l = endpoint->requests;
We probably want to do the l = g_gslist_next assignment inside the for
loop to guarantee the requests list in not freed in the process.
> + }
> +}
> +
> struct pac_config_data {
> struct bt_bap_stream *stream;
> bt_bap_pac_config_t cb;
> @@ -1195,6 +1224,7 @@ static void pac_clear(struct bt_bap_stream *stream, void *user_data)
>
> static struct bt_bap_pac_ops pac_ops = {
> .select = pac_select,
> + .cancel_select = pac_cancel_select,
> .config = pac_config,
> .clear = pac_clear,
> };
> --
> 2.44.0
>
>
--
Luiz Augusto von Dentz
