| Message ID | 90670acf017c4fb5a4ef4dba4b666c1307e9de5c.1710602216.git.pav@iki.fi (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [BlueZ] bap: handle ep or session freed before select_cb returns | expand |
| Context | Check | Description |
|---|---|---|
| tedd_an/pre-ci_am | success | Success |
| tedd_an/CheckPatch | success | CheckPatch PASS |
| tedd_an/GitLint | success | Gitlint PASS |
| tedd_an/BuildEll | success | Build ELL PASS |
| tedd_an/BluezMake | success | Bluez Make PASS |
Hi Pauli, On Sat, Mar 16, 2024 at 11:24 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. > > 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 > >
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@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. > > > > 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 > > > > > >
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; }
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. 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(-)