Received-SPF: pass (google.com: domain of linux-bluetooth+bounces-3636-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Precedence: bulk
MIME-Version: 1.0
References: <d52ddf4759720a2879677fca0129d3fd1a32dda0.1712951445.git.pav@iki.fi>
In-Reply-To: <d52ddf4759720a2879677fca0129d3fd1a32dda0.1712951445.git.pav@iki.fi>
From: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date: Tue, 16 Apr 2024 11:16:17 -0400
Message-ID: <CABBYNZLHfS95O=0QWbitCpQRhcn8smez0wEAXzouonPFsra5nw@mail.gmail.com>
Subject: Re: [PATCH BlueZ 1/2] shared/bap: clean up requests for a stream
 before freeing it
To: Pauli Virtanen <pav@iki.fi>
Cc: linux-bluetooth@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Pauli,

On Fri, Apr 12, 2024 at 3:58=E2=80=AFPM Pauli Virtanen <pav@iki.fi> wrote:
>
> Cancel stream's queued requests before freeing the stream.
>
> As the callbacks may do some cleanup on error, be sure to call them
> before removing the requests.
>
> Fixes:
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> ERROR: AddressSanitizer: heap-use-after-free on address 0x60d000013430
> READ of size 8 at 0x60d000013430 thread T0
>     #0 0x89cb9f in stream_stop_complete src/shared/bap.c:1211
>     #1 0x89c997 in bap_req_complete src/shared/bap.c:1192
>     #2 0x8a105f in bap_process_queue src/shared/bap.c:1474
>     #3 0x93c93f in timeout_callback src/shared/timeout-glib.c:25
> ...
> freed by thread T0 here:
>     #1 0x89b744 in bap_stream_free src/shared/bap.c:1105
>     #2 0x89bac8 in bap_stream_detach src/shared/bap.c:1122
>     #3 0x89dbfc in bap_stream_state_changed src/shared/bap.c:1261
>     #4 0x8a2169 in bap_ucast_set_state src/shared/bap.c:1554
>     #5 0x89e0d5 in stream_set_state src/shared/bap.c:1291
>     #6 0x8a78b6 in bap_ucast_release src/shared/bap.c:1927
>     #7 0x8d45bb in bt_bap_stream_release src/shared/bap.c:5516
>     #8 0x8ba63f in remove_streams src/shared/bap.c:3538
>     #9 0x7f23d0 in queue_foreach src/shared/queue.c:207
>     #10 0x8bb875 in bt_bap_remove_pac src/shared/bap.c:3593
>     #11 0x47416c in media_endpoint_destroy profiles/audio/media.c:185
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> ---
>  src/shared/bap.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
>
> diff --git a/src/shared/bap.c b/src/shared/bap.c
> index 5fee7b4c5..ccde26431 100644
> --- a/src/shared/bap.c
> +++ b/src/shared/bap.c
> @@ -1105,6 +1105,9 @@ static void bap_stream_free(void *data)
>         free(stream);
>  }
>
> +static void bap_abort_stream_req(struct bt_bap *bap,
> +                                               struct bt_bap_stream *str=
eam);

Normally we suggest just to move up the function definitions to avoid
forward declarations like the above.

>  static void bap_stream_detach(struct bt_bap_stream *stream)
>  {
>         struct bt_bap_endpoint *ep =3D stream->ep;
> @@ -1114,6 +1117,8 @@ static void bap_stream_detach(struct bt_bap_stream =
*stream)
>
>         DBG(stream->bap, "stream %p ep %p", stream, ep);
>
> +       bap_abort_stream_req(stream->bap, stream);
> +
>         queue_remove(stream->bap->streams, stream);
>         bap_stream_clear_cfm(stream);
>
> @@ -1477,6 +1482,28 @@ static bool bap_process_queue(void *data)
>         return false;
>  }
>
> +static bool match_req_stream(const void *data, const void *match_data)
> +{
> +       const struct bt_bap_req *pend =3D data;
> +
> +       return pend->stream =3D=3D match_data;
> +}
> +
> +static void bap_req_abort(void *data)
> +{
> +       struct bt_bap_req *req =3D data;
> +       struct bt_bap *bap =3D req->stream->bap;
> +
> +       DBG(bap, "req %p", req);
> +       bap_req_complete(req, NULL);
> +}
> +
> +static void bap_abort_stream_req(struct bt_bap *bap,
> +                                               struct bt_bap_stream *str=
eam)
> +{
> +       queue_remove_all(bap->reqs, match_req_stream, stream, bap_req_abo=
rt);
> +}
> +
>  static bool bap_queue_req(struct bt_bap *bap, struct bt_bap_req *req)
>  {
>         struct bt_bap_req *pend;
> --
> 2.44.0
>
>


--=20
Luiz Augusto von Dentz