Hi Pauli, On Mon, Feb 13, 2023 at 1:28 PM Pauli Virtanen <pav@xxxxxx> wrote: > > The BAP stream user data used for transport paths in media.c is set both > in media.c:pac_config and in profiles/bap.c. In the latter, it gets set > to a string owned by bap_ep, whose lifetime can be shorter than that of > the transport. > > Under some conditions, bap.c:bap_disconnect is hit while there are > transports and the user data is owned by the endpoint. In this case, the > path string owned by the endpoints gets freed first, and ASAN > use-after-free crash is encountered when clearing the transports. Hmm, I didn't hit while testing, anyway that means the transport is being freed before we clean up? It might be a good idea to run this under valgrind to check where the path is freed. > Fix this by matching the transports by the stream pointer, and not by > the transport/endpoint path. > > Fixes: 7b1b1a499cf3 ("media: clear the right transport when clearing BAP endpoint") > --- > > Notes: > The v2 version of the patch in commit 7b1b1a499cf334 was buggy, and its > v3 sent to the list was supposed to replace it. However, I resubmitted > only that patch and not the full series, which maybe would have been the > right thing. Sorry for the mess. > > profiles/audio/media.c | 21 +++++++++++---------- > profiles/audio/transport.c | 14 ++++++++++++++ > profiles/audio/transport.h | 1 + > 3 files changed, 26 insertions(+), 10 deletions(-) > > diff --git a/profiles/audio/media.c b/profiles/audio/media.c > index 3eb038cb7..8728b69e0 100644 > --- a/profiles/audio/media.c > +++ b/profiles/audio/media.c > @@ -1085,19 +1085,20 @@ static int pac_config(struct bt_bap_stream *stream, struct iovec *cfg, > static void pac_clear(struct bt_bap_stream *stream, void *user_data) > { > struct media_endpoint *endpoint = user_data; > - struct media_transport *transport; > - const char *path; > + GSList *item; > > - path = bt_bap_stream_get_user_data(stream); > - if (!path) > - return; > + DBG("endpoint %p stream %p", endpoint, stream); > > - DBG("endpoint %p path %s", endpoint, path); > + item = endpoint->transports; > + while (item) { > + struct media_transport *transport = item->data; > > - transport = find_transport(endpoint, path); > - if (transport) { > - clear_configuration(endpoint, transport); > - bt_bap_stream_set_user_data(stream, NULL); > + if (media_transport_get_stream(transport) == stream) { > + clear_configuration(endpoint, transport); > + item = endpoint->transports; > + } else { > + item = item->next; > + } > } > } > > diff --git a/profiles/audio/transport.c b/profiles/audio/transport.c > index 5e057e2a5..cd91669c6 100644 > --- a/profiles/audio/transport.c > +++ b/profiles/audio/transport.c > @@ -1483,6 +1483,20 @@ const char *media_transport_get_path(struct media_transport *transport) > return transport->path; > } > > +void *media_transport_get_stream(struct media_transport *transport) > +{ > + struct bap_transport *bap; > + const char *uuid; > + > + uuid = media_endpoint_get_uuid(transport->endpoint); > + if (strcasecmp(uuid, PAC_SINK_UUID) && > + strcasecmp(uuid, PAC_SOURCE_UUID)) > + return NULL; This should probably be made generic, perhaps with a get_stream callback so we don't have to check the UUID like above. > + > + bap = transport->data; > + return bap->stream; > +} > + > void media_transport_update_delay(struct media_transport *transport, > uint16_t delay) > { > diff --git a/profiles/audio/transport.h b/profiles/audio/transport.h > index 102fc3cf1..5ca9b8f9e 100644 > --- a/profiles/audio/transport.h > +++ b/profiles/audio/transport.h > @@ -19,6 +19,7 @@ struct media_transport *media_transport_create(struct btd_device *device, > > void media_transport_destroy(struct media_transport *transport); > const char *media_transport_get_path(struct media_transport *transport); > +void *media_transport_get_stream(struct media_transport *transport); > struct btd_device *media_transport_get_dev(struct media_transport *transport); > int8_t media_transport_get_volume(struct media_transport *transport); > void media_transport_update_delay(struct media_transport *transport, > -- > 2.39.1 > -- Luiz Augusto von Dentz