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
