[PATCH BlueZ v2 1/2] shared/bap: add bt_bap_cancel_select to cancel ongoing pac select

[PATCH BlueZ v2 1/2] shared/bap: add bt_bap_cancel_select to cancel ongoing pac select

[Pauli Virtanen]

Add function and PAC ops for canceling a previously initiated
SelectProperties() call.
---

Notes:
    v2: cancel the DBus request and callback, instead of ignoring stale cbs

 src/shared/bap.c | 12 ++++++++++++
 src/shared/bap.h |  5 +++++
 2 files changed, 17 insertions(+)

diff --git a/src/shared/bap.c b/src/shared/bap.c
index a1749153b..f553096df 100644
--- a/src/shared/bap.c
+++ b/src/shared/bap.c
@@ -5180,6 +5180,18 @@ int bt_bap_select(struct bt_bap_pac *lpac, struct bt_bap_pac *rpac,
 	return 0;
 }
 
+void bt_bap_cancel_select(struct bt_bap_pac *lpac, bt_bap_pac_select_t func,
+								void *user_data)
+{
+	if (!lpac || !func)
+		return;
+
+	if (!lpac->ops || !lpac->ops->cancel_select)
+		return;
+
+	lpac->ops->cancel_select(lpac, func, user_data, lpac->user_data);
+}
+
 static struct bt_bap_stream *bap_bcast_stream_new(struct bt_bap *bap,
 					struct bt_bap_pac *lpac,
 					struct bt_bap_pac *rpac,
diff --git a/src/shared/bap.h b/src/shared/bap.h
index 9839e3249..62e210485 100644
--- a/src/shared/bap.h
+++ b/src/shared/bap.h
@@ -72,6 +72,8 @@ struct bt_bap_pac_ops {
 	int (*select)(struct bt_bap_pac *lpac, struct bt_bap_pac *rpac,
 			uint32_t chan_alloc, struct bt_bap_pac_qos *qos,
 			bt_bap_pac_select_t cb, void *cb_data, void *user_data);
+	void (*cancel_select)(struct bt_bap_pac *lpac,
+			bt_bap_pac_select_t cb, void *cb_data, void *user_data);
 	int (*config)(struct bt_bap_stream *stream, struct iovec *cfg,
 			struct bt_bap_qos *qos, bt_bap_pac_config_t cb,
 			void *user_data);
@@ -160,6 +162,9 @@ int bt_bap_select(struct bt_bap_pac *lpac, struct bt_bap_pac *rpac,
 			int *count, bt_bap_pac_select_t func,
 			void *user_data);
 
+void bt_bap_cancel_select(struct bt_bap_pac *lpac, bt_bap_pac_select_t func,
+			void *user_data);
+
 struct bt_bap_stream *bt_bap_stream_new(struct bt_bap *bap,
 					struct bt_bap_pac *lpac,
 					struct bt_bap_pac *rpac,
-- 
2.44.0

[PATCH BlueZ v2 2/2] bap: cancel ongoing SelectProperties() before freeing the ep

[Pauli Virtanen]

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.

Fix by implementing cancelling select() callbacks, and doing it before
freeing ep.

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   | 22 ++++++++++++++++++++++
 profiles/audio/media.c | 30 ++++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+)

diff --git a/profiles/audio/bap.c b/profiles/audio/bap.c
index 315eff729..fdce275c9 100644
--- a/profiles/audio/bap.c
+++ b/profiles/audio/bap.c
@@ -1148,11 +1148,15 @@ static const GDBusMethodTable ep_methods[] = {
 	{ },
 };
 
+static void ep_cancel_select(struct bap_ep *ep);
+
 static void ep_free(void *data)
 {
 	struct bap_ep *ep = data;
 	struct queue *setups = ep->setups;
 
+	ep_cancel_select(ep);
+
 	ep->setups = NULL;
 	queue_destroy(setups, setup_free);
 	free(ep->path);
@@ -1426,6 +1430,24 @@ static bool pac_select(struct bt_bap_pac *lpac, struct bt_bap_pac *rpac,
 	return true;
 }
 
+static bool pac_cancel_select(struct bt_bap_pac *lpac, struct bt_bap_pac *rpac,
+							void *user_data)
+{
+	struct bap_ep *ep = user_data;
+
+	bt_bap_cancel_select(lpac, select_cb, ep);
+
+	return true;
+}
+
+static void ep_cancel_select(struct bap_ep *ep)
+{
+	struct bt_bap *bap = ep->data->bap;
+
+	bt_bap_foreach_pac(bap, BT_BAP_SOURCE, pac_cancel_select, ep);
+	bt_bap_foreach_pac(bap, BT_BAP_SINK, pac_cancel_select, ep);
+}
+
 static bool pac_found_bcast(struct bt_bap_pac *lpac, struct bt_bap_pac *rpac,
 							void *user_data)
 {
diff --git a/profiles/audio/media.c b/profiles/audio/media.c
index edaff7867..3c5daf782 100644
--- a/profiles/audio/media.c
+++ b/profiles/audio/media.c
@@ -149,6 +149,11 @@ static void media_endpoint_cancel(struct endpoint_request *request)
 {
 	struct media_endpoint *endpoint = request->endpoint;
 
+	DBG("Canceling %s: name = %s path = %s",
+			dbus_message_get_member(request->msg),
+			dbus_message_get_destination(request->msg),
+			dbus_message_get_path(request->msg));
+
 	if (request->call)
 		dbus_pending_call_cancel(request->call);
 
@@ -1028,6 +1033,30 @@ static int pac_select(struct bt_bap_pac *lpac, struct bt_bap_pac *rpac,
 								data, free);
 }
 
+static void pac_cancel_select(struct bt_bap_pac *lpac, bt_bap_pac_select_t cb,
+						void *cb_data, void *user_data)
+{
+	struct media_endpoint *endpoint = user_data;
+	GSList *l;
+
+	for (l = endpoint->requests; l; l = g_slist_next(l)) {
+		struct endpoint_request *req = l->data;
+		struct pac_select_data *data;
+
+		if (req->cb != pac_select_cb)
+			continue;
+
+		data = req->user_data;
+		if (data->pac != lpac || data->cb != cb ||
+						data->user_data != cb_data)
+			continue;
+
+		req->cb = NULL;
+		media_endpoint_cancel(req);
+		l = endpoint->requests;
+	}
+}
+
 struct pac_config_data {
 	struct bt_bap_stream *stream;
 	bt_bap_pac_config_t cb;
@@ -1195,6 +1224,7 @@ static void pac_clear(struct bt_bap_stream *stream, void *user_data)
 
 static struct bt_bap_pac_ops pac_ops = {
 	.select = pac_select,
+	.cancel_select = pac_cancel_select,
 	.config = pac_config,
 	.clear = pac_clear,
 };
-- 
2.44.0

Re: [PATCH BlueZ v2 2/2] bap: cancel ongoing SelectProperties() before freeing the ep

[Luiz Augusto von Dentz]

Hi Pauli,

On Sat, Mar 23, 2024 at 5:40 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.
>
> Fix by implementing cancelling select() callbacks, and doing it before
> freeing ep.
>
> 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   | 22 ++++++++++++++++++++++
>  profiles/audio/media.c | 30 ++++++++++++++++++++++++++++++
>  2 files changed, 52 insertions(+)
>
> diff --git a/profiles/audio/bap.c b/profiles/audio/bap.c
> index 315eff729..fdce275c9 100644
> --- a/profiles/audio/bap.c
> +++ b/profiles/audio/bap.c
> @@ -1148,11 +1148,15 @@ static const GDBusMethodTable ep_methods[] = {
>         { },
>  };
>
> +static void ep_cancel_select(struct bap_ep *ep);
> +
>  static void ep_free(void *data)
>  {
>         struct bap_ep *ep = data;
>         struct queue *setups = ep->setups;
>
> +       ep_cancel_select(ep);
> +
>         ep->setups = NULL;
>         queue_destroy(setups, setup_free);
>         free(ep->path);
> @@ -1426,6 +1430,24 @@ static bool pac_select(struct bt_bap_pac *lpac, struct bt_bap_pac *rpac,
>         return true;
>  }
>
> +static bool pac_cancel_select(struct bt_bap_pac *lpac, struct bt_bap_pac *rpac,
> +                                                       void *user_data)
> +{
> +       struct bap_ep *ep = user_data;
> +
> +       bt_bap_cancel_select(lpac, select_cb, ep);
> +
> +       return true;
> +}
> +
> +static void ep_cancel_select(struct bap_ep *ep)
> +{
> +       struct bt_bap *bap = ep->data->bap;
> +
> +       bt_bap_foreach_pac(bap, BT_BAP_SOURCE, pac_cancel_select, ep);
> +       bt_bap_foreach_pac(bap, BT_BAP_SINK, pac_cancel_select, ep);
> +}
> +
>  static bool pac_found_bcast(struct bt_bap_pac *lpac, struct bt_bap_pac *rpac,
>                                                         void *user_data)
>  {
> diff --git a/profiles/audio/media.c b/profiles/audio/media.c
> index edaff7867..3c5daf782 100644
> --- a/profiles/audio/media.c
> +++ b/profiles/audio/media.c
> @@ -149,6 +149,11 @@ static void media_endpoint_cancel(struct endpoint_request *request)
>  {
>         struct media_endpoint *endpoint = request->endpoint;
>
> +       DBG("Canceling %s: name = %s path = %s",
> +                       dbus_message_get_member(request->msg),
> +                       dbus_message_get_destination(request->msg),
> +                       dbus_message_get_path(request->msg));
> +
>         if (request->call)
>                 dbus_pending_call_cancel(request->call);
>
> @@ -1028,6 +1033,30 @@ static int pac_select(struct bt_bap_pac *lpac, struct bt_bap_pac *rpac,
>                                                                 data, free);
>  }
>
> +static void pac_cancel_select(struct bt_bap_pac *lpac, bt_bap_pac_select_t cb,
> +                                               void *cb_data, void *user_data)
> +{
> +       struct media_endpoint *endpoint = user_data;
> +       GSList *l;
> +
> +       for (l = endpoint->requests; l; l = g_slist_next(l)) {
> +               struct endpoint_request *req = l->data;
> +               struct pac_select_data *data;
> +
> +               if (req->cb != pac_select_cb)
> +                       continue;
> +
> +               data = req->user_data;
> +               if (data->pac != lpac || data->cb != cb ||
> +                                               data->user_data != cb_data)
> +                       continue;
> +
> +               req->cb = NULL;
> +               media_endpoint_cancel(req);
> +               l = endpoint->requests;

We probably want to do the l = g_gslist_next assignment inside the for
loop to guarantee the requests list in not freed in the process.

> +       }
> +}
> +
>  struct pac_config_data {
>         struct bt_bap_stream *stream;
>         bt_bap_pac_config_t cb;
> @@ -1195,6 +1224,7 @@ static void pac_clear(struct bt_bap_stream *stream, void *user_data)
>
>  static struct bt_bap_pac_ops pac_ops = {
>         .select = pac_select,
> +       .cancel_select = pac_cancel_select,
>         .config = pac_config,
>         .clear = pac_clear,
>  };
> --
> 2.44.0
>
>


-- 
Luiz Augusto von Dentz

RE: [BlueZ,v2,1/2] shared/bap: add bt_bap_cancel_select to cancel ongoing pac select

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 1813 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=837533

---Test result---

Test Summary:
CheckPatch                    PASS      0.96 seconds
GitLint                       PASS      0.64 seconds
BuildEll                      PASS      24.18 seconds
BluezMake                     PASS      1672.48 seconds
MakeCheck                     PASS      12.96 seconds
MakeDistcheck                 PASS      178.60 seconds
CheckValgrind                 PASS      246.92 seconds
CheckSmatch                   WARNING   348.60 seconds
bluezmakeextell               PASS      118.98 seconds
IncrementalBuild              PASS      3039.28 seconds
ScanBuild                     WARNING   986.96 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
##############################
Test: ScanBuild - WARNING
Desc: Run Scan Build
Output:
profiles/audio/media.c:1046:7: warning: Use of memory after it is freed
                if (req->cb != pac_select_cb)
                    ^~~~~~~
1 warning generated.



---
Regards,
Linux Bluetooth