| Message ID | ad76621e8c2d414989b4eb384cb9077ca55cb16d.1711186824.git.pav@iki.fi |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [BlueZ,v2,1/2] shared/bap: add bt_bap_cancel_select to cancel ongoing pac select | expand |
Hi Pauli, On Sat, Mar 23, 2024 at 5:40 AM Pauli Virtanen <pav@iki.fi> 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 > >
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; + } +} + 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, };
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(+)