[PATCH BlueZ 1/2] shared/bap: clean up requests for a stream before freeing it

[PATCH BlueZ 1/2] shared/bap: clean up requests for a stream before freeing it

[Pauli Virtanen]

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:
=======================================================================
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
=======================================================================
---
 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 *stream);
+
 static void bap_stream_detach(struct bt_bap_stream *stream)
 {
 	struct bt_bap_endpoint *ep = 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 = data;
+
+	return pend->stream == match_data;
+}
+
+static void bap_req_abort(void *data)
+{
+	struct bt_bap_req *req = data;
+	struct bt_bap *bap = 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 *stream)
+{
+	queue_remove_all(bap->reqs, match_req_stream, stream, bap_req_abort);
+}
+
 static bool bap_queue_req(struct bt_bap *bap, struct bt_bap_req *req)
 {
 	struct bt_bap_req *pend;
-- 
2.44.0

[PATCH BlueZ 2/2] bap: cancel stream operation before freeing setup

[Pauli Virtanen]

Before freeing setup, cancel any ongoing stream operations, and indicate
failure for pending DBus replies.

Fixes:
=======================================================================
ERROR: AddressSanitizer: heap-use-after-free on address 0x60d000004758
WRITE of size 4 at 0x60d000004758 thread T0
    #0 0x557159 in qos_cb profiles/audio/bap.c:753
    #1 0x89c38f in bap_req_complete src/shared/bap.c:1191
    #2 0x8cb7fc in bap_req_detach src/shared/bap.c:4789
    #3 0x8cb9bb in bt_bap_detach src/shared/bap.c:4801
    #4 0x571e25 in bap_disconnect profiles/audio/bap.c:3011
    ...
freed by thread T0 here:
    #1 0x558f2b in setup_free profiles/audio/bap.c:890
    #2 0x7f34e8 in queue_remove_all src/shared/queue.c:341
    #3 0x7f0105 in queue_destroy src/shared/queue.c:60
    #4 0x55cdc8 in ep_free profiles/audio/bap.c:1167
=======================================================================
---
 profiles/audio/bap.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/profiles/audio/bap.c b/profiles/audio/bap.c
index 30049f0fb..ff6d6d881 100644
--- a/profiles/audio/bap.c
+++ b/profiles/audio/bap.c
@@ -879,9 +879,22 @@ static struct bap_setup *setup_new(struct bap_ep *ep)
 static void setup_free(void *data)
 {
 	struct bap_setup *setup = data;
+	DBusMessage *reply;
 
 	DBG("%p", setup);
 
+	if (setup->stream && setup->id) {
+		bt_bap_stream_cancel(setup->stream, setup->id);
+		setup->id = 0;
+	}
+
+	if (setup->msg) {
+		reply = btd_error_failed(setup->msg, "Canceled");
+		g_dbus_send_message(btd_get_dbus_connection(), reply);
+		dbus_message_unref(setup->msg);
+		setup->msg = NULL;
+	}
+
 	if (setup->ep)
 		queue_remove(setup->ep->setups, setup);
 
-- 
2.44.0

RE: [BlueZ,1/2] shared/bap: clean up requests for a stream before freeing it

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 1560 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=844165

---Test result---

Test Summary:
CheckPatch                    PASS      0.95 seconds
GitLint                       PASS      0.65 seconds
BuildEll                      PASS      24.39 seconds
BluezMake                     PASS      1692.37 seconds
MakeCheck                     PASS      12.80 seconds
MakeDistcheck                 PASS      181.80 seconds
CheckValgrind                 PASS      251.55 seconds
CheckSmatch                   WARNING   351.20 seconds
bluezmakeextell               PASS      119.41 seconds
IncrementalBuild              PASS      2961.19 seconds
ScanBuild                     PASS      1021.32 seconds

Details
##############################
Test: CheckSmatch - WARNING
Desc: Run smatch tool with source
Output:
src/shared/bap.c:282:25: warning: array of flexible structuressrc/shared/bap.c: note: in included file:./src/shared/ascs.h:88:25: warning: array of flexible structuressrc/shared/bap.c:282:25: warning: array of flexible structuressrc/shared/bap.c: note: in included file:./src/shared/ascs.h:88:25: warning: array of flexible structuressrc/shared/bap.c:282:25: warning: array of flexible structuressrc/shared/bap.c: note: in included file:./src/shared/ascs.h:88:25: warning: array of flexible structures


---
Regards,
Linux Bluetooth

Re: [PATCH BlueZ 1/2] shared/bap: clean up requests for a stream before freeing it

[Luiz Augusto von Dentz]

Hi Pauli,

On Fri, Apr 12, 2024 at 3:58 PM 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:
> =======================================================================
> 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
> =======================================================================
> ---
>  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 *stream);

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 = 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 = data;
> +
> +       return pend->stream == match_data;
> +}
> +
> +static void bap_req_abort(void *data)
> +{
> +       struct bt_bap_req *req = data;
> +       struct bt_bap *bap = 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 *stream)
> +{
> +       queue_remove_all(bap->reqs, match_req_stream, stream, bap_req_abort);
> +}
> +
>  static bool bap_queue_req(struct bt_bap *bap, struct bt_bap_req *req)
>  {
>         struct bt_bap_req *pend;
> --
> 2.44.0
>
>


-- 
Luiz Augusto von Dentz

Re: [PATCH BlueZ 1/2] shared/bap: clean up requests for a stream before freeing it

[patchwork-bot+bluetooth]

Hello:

This series was applied to bluetooth/bluez.git (master)
by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>:

On Fri, 12 Apr 2024 22:55:55 +0300 you 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:
> =======================================================================
> 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
> =======================================================================
> 
> [...]

Here is the summary with links:
  - [BlueZ,1/2] shared/bap: clean up requests for a stream before freeing it
    (no matching commit)
  - [BlueZ,2/2] bap: cancel stream operation before freeing setup
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=d3a6a6459cbd

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html