[PATCH BlueZ v3 0/3] Fixed the crash observed with VOCS

[PATCH BlueZ v3 0/3] Fixed the crash observed with VOCS

[Nitin Jadhav]

Hello Maintainers

This series of patch handles the fix for the crash observed with VOCS when trying to pair with LE Audio TWS earbuds and few other issues. 

A crash was reported for the following patch by Pauli Virtanen <pav@iki.fi>. 
Patch Link: https://patchwork.kernel.org/project/bluetooth/patch/20230612133251.194-4-nitin.jadhav@nxp.com/

Root cause: 
- There are two types of database- Remote and Local (rdb and ldb)
- In client mode currently the code was written to access ldb

Fix:
- Correcting it to access rdb has resolved the problem in VOCS
- Same correction is done for VCS.

Later to fixing the above crash few more issues were observed, these are fixed in subsequent patches.

Thanks,
Warm Regards,
Nitin Jadhav

Nitin Jadhav (3):
  shared/vcp: Fixed the crash observed with VOCS
  shared/vcp: Fixed issues related to read audio location and descriptor
  shared/vcp: Corrected handle size to accommodate included service

 src/shared/vcp.c | 47 +++++++++++++++++++++++++----------------------
 1 file changed, 25 insertions(+), 22 deletions(-)

-- 
2.34.1

[PATCH BlueZ v3 1/3] shared/vcp: Fixed the crash observed with VOCS

[Nitin Jadhav]

Root cause
- There are two types of database- Remote and Local (rdb and ldb).
- In client mode currently the code was written to access ldb.

Fix
- Correcting it, to access rdb has resolved the problem in VOCS.
- Same correction is done for VCS.

Reported-by: Pauli Virtanen <pav@iki.fi>
---
 src/shared/vcp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/shared/vcp.c b/src/shared/vcp.c
index 74bd01729..314618664 100644
--- a/src/shared/vcp.c
+++ b/src/shared/vcp.c
@@ -1719,10 +1719,10 @@ bool bt_vcp_attach(struct bt_vcp *vcp, struct bt_gatt_client *client)
 		return false;
 
 	bt_uuid16_create(&uuid, VCS_UUID);
-	gatt_db_foreach_service(vcp->ldb->db, &uuid, foreach_vcs_service, vcp);
+	gatt_db_foreach_service(vcp->rdb->db, &uuid, foreach_vcs_service, vcp);
 
 	bt_uuid16_create(&uuid, VOL_OFFSET_CS_UUID);
-	gatt_db_foreach_service(vcp->ldb->db, &uuid, foreach_vocs_service, vcp);
+	gatt_db_foreach_service(vcp->rdb->db, &uuid, foreach_vocs_service, vcp);
 
 	return true;
 }
-- 
2.34.1

RE: Fixed the crash observed with VOCS

[bluez.test.bot]

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

---Test result---

Test Summary:
CheckPatch                    FAIL      1.47 seconds
GitLint                       FAIL      1.02 seconds
BuildEll                      PASS      34.62 seconds
BluezMake                     PASS      1221.61 seconds
MakeCheck                     PASS      13.02 seconds
MakeDistcheck                 PASS      200.94 seconds
CheckValgrind                 PASS      327.52 seconds
CheckSmatch                   PASS      460.63 seconds
bluezmakeextell               PASS      134.82 seconds
IncrementalBuild              PASS      3132.74 seconds
ScanBuild                     PASS      1394.25 seconds

Details
##############################
Test: CheckPatch - FAIL
Desc: Run checkpatch.pl script
Output:
[BlueZ,v3,2/3] shared/vcp: Fixed issues related to read audio location and descriptor
WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#99: 
- Audio output descriptor & location read functions, requires code correction

/github/workspace/src/src/13326329.patch total: 0 errors, 1 warnings, 76 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

/github/workspace/src/src/13326329.patch has style problems, please review.

NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED SPDX_LICENSE_TAG SPLIT_STRING SSCANF_TO_KSTRTO

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.


##############################
Test: GitLint - FAIL
Desc: Run gitlint
Output:
[BlueZ,v3,2/3] shared/vcp: Fixed issues related to read audio location and descriptor

WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search
1: T1 Title exceeds max length (85>80): "[BlueZ,v3,2/3] shared/vcp: Fixed issues related to read audio location and descriptor"


---
Regards,
Linux Bluetooth

[PATCH BlueZ v3 2/3] shared/vcp: Fixed issues related to read audio location and descriptor

[Nitin Jadhav]

Issue
- Audio output descriptor & location read functions, requires code correction
- During reading audio location descriptor value was checked instead of pointer.
DBG statement wrongly accessed value to print the log that caused crash.

Fix
- Corrected audio output descriptor read variable
- Corrected the audio location debug printing function
- Allocated the memory as char pointer to the audio descriptor variable and
copied the string to be displayed.
- Corrected the audio descriptor pointer checking condition
---
 src/shared/vcp.c | 41 ++++++++++++++++++++++-------------------
 1 file changed, 22 insertions(+), 19 deletions(-)

diff --git a/src/shared/vcp.c b/src/shared/vcp.c
index 314618664..0408531b2 100644
--- a/src/shared/vcp.c
+++ b/src/shared/vcp.c
@@ -973,7 +973,7 @@ static void vocs_voaodec_read(struct gatt_db_attribute *attrib,
 	struct bt_vocs *vocs = user_data;
 	struct iovec iov;
 
-	iov.iov_base = &vocs->vocs_ao_dec;
+	iov.iov_base = vocs->vocs_ao_dec;
 	iov.iov_len = strlen(vocs->vocs_ao_dec);
 
 	gatt_db_attribute_read_result(attrib, id, 0, iov.iov_base,
@@ -1385,11 +1385,12 @@ static void read_vocs_audio_location(struct bt_vcp *vcp, bool success,
 				     const uint8_t *value, uint16_t length,
 				     void *user_data)
 {
-	uint32_t *vocs_audio_loc;
-	struct iovec iov = {
-		.iov_base = (void *) value,
-		.iov_len = length,
-	};
+	uint32_t vocs_audio_loc;
+
+	if (!value) {
+		DBG(vcp, "Unable to get VOCS Audio Location");
+		return;
+	}
 
 	if (!success) {
 		DBG(vcp, "Unable to read VOCS Audio Location: error 0x%02x",
@@ -1397,13 +1398,9 @@ static void read_vocs_audio_location(struct bt_vcp *vcp, bool success,
 		return;
 	}
 
-	vocs_audio_loc = iov_pull_mem(&iov, sizeof(uint32_t));
-	if (!*vocs_audio_loc) {
-		DBG(vcp, "Unable to get VOCS Audio Location");
-		return;
-	}
+	memcpy(&vocs_audio_loc, value, length);
 
-	DBG(vcp, "VOCS Audio Loc:%x", *vocs_audio_loc);
+	DBG(vcp, "VOCS Audio Loc: %x", vocs_audio_loc);
 }
 
 
@@ -1413,10 +1410,11 @@ static void read_vocs_audio_descriptor(struct bt_vcp *vcp, bool success,
 				       void *user_data)
 {
 	char *vocs_ao_dec_r;
-	struct iovec iov = {
-		.iov_base = (void *) value,
-		.iov_len = length,
-	};
+
+	if (!value) {
+		DBG(vcp, "Unable to get VOCS Audio Location");
+		return;
+	}
 
 	if (!success) {
 		DBG(vcp, "Unable to read VOCS Audio Descriptor: error 0x%02x",
@@ -1424,13 +1422,18 @@ static void read_vocs_audio_descriptor(struct bt_vcp *vcp, bool success,
 		return;
 	}
 
-	vocs_ao_dec_r = iov_pull_mem(&iov, length);
-	if (!*vocs_ao_dec_r) {
+	vocs_ao_dec_r = malloc(length+1);
+	memset(vocs_ao_dec_r, 0, length+1);
+	memcpy(vocs_ao_dec_r, value, length);
+
+	if (!vocs_ao_dec_r) {
 		DBG(vcp, "Unable to get VOCS Audio Descriptor");
 		return;
 	}
 
-	DBG(vcp, "VOCS Audio Descriptor:%s", *vocs_ao_dec_r);
+	DBG(vcp, "VOCS Audio Descriptor: %s", vocs_ao_dec_r);
+	free(vocs_ao_dec_r);
+	vocs_ao_dec_r = NULL;
 }
 
 static void vcp_pending_destroy(void *data)
-- 
2.34.1

[PATCH BlueZ v3 3/3] shared/vcp: Corrected handle size to accommodate included service

[Nitin Jadhav]

Fix: As per specs VOCS is secondary services hence it should be included
under a primary service VCS. Current number of handles should be increase
to accommodate the included service.
---
 src/shared/vcp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/shared/vcp.c b/src/shared/vcp.c
index 0408531b2..263da4c30 100644
--- a/src/shared/vcp.c
+++ b/src/shared/vcp.c
@@ -998,7 +998,7 @@ static struct bt_vcs *vcs_new(struct gatt_db *db, struct bt_vcp_db *vdb)
 
 	/* Populate DB with VCS attributes */
 	bt_uuid16_create(&uuid, VCS_UUID);
-	vcs->service = gatt_db_add_service(db, &uuid, true, 9);
+	vcs->service = gatt_db_add_service(db, &uuid, true, 10);
 	gatt_db_service_add_included(vcs->service, vdb->vocs->service);
 	gatt_db_service_set_active(vdb->vocs->service, true);
 
-- 
2.34.1

Re: [PATCH BlueZ v3 3/3] shared/vcp: Corrected handle size to accommodate included service

[Paul Menzel]

Dear Nitin,


Thank you for your patch.

Am 25.07.23 um 13:21 schrieb Nitin Jadhav:
> Fix: As per specs VOCS is secondary services hence it should be included

singular: service?

> under a primary service VCS. Current number of handles should be increase

increase*d*

> to accommodate the included service.

I’d always use imperative mood (also in the commit message summary):

Correct handle size to accommodate included service

or

Reserve one more handler in VCS service for VOCS

> ---
>   src/shared/vcp.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/shared/vcp.c b/src/shared/vcp.c
> index 0408531b2..263da4c30 100644
> --- a/src/shared/vcp.c
> +++ b/src/shared/vcp.c
> @@ -998,7 +998,7 @@ static struct bt_vcs *vcs_new(struct gatt_db *db, struct bt_vcp_db *vdb)
>   
>   	/* Populate DB with VCS attributes */
>   	bt_uuid16_create(&uuid, VCS_UUID);
> -	vcs->service = gatt_db_add_service(db, &uuid, true, 9);
> +	vcs->service = gatt_db_add_service(db, &uuid, true, 10);
>   	gatt_db_service_add_included(vcs->service, vdb->vocs->service);
>   	gatt_db_service_set_active(vdb->vocs->service, true);
>   


Kind regards,

Paul

Re: [PATCH BlueZ v3 0/3] Fixed the crash observed with VOCS

[patchwork-bot+bluetooth]

Hello:

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

On Tue, 25 Jul 2023 14:21:23 +0300 you wrote:
> Hello Maintainers
> 
> This series of patch handles the fix for the crash observed with VOCS when trying to pair with LE Audio TWS earbuds and few other issues.
> 
> A crash was reported for the following patch by Pauli Virtanen <pav@iki.fi>.
> Patch Link: https://patchwork.kernel.org/project/bluetooth/patch/20230612133251.194-4-nitin.jadhav@nxp.com/
> 
> [...]

Here is the summary with links:
  - [BlueZ,v3,1/3] shared/vcp: Fixed the crash observed with VOCS
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=06ec4f61000a
  - [BlueZ,v3,2/3] shared/vcp: Fixed issues related to read audio location and descriptor
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=2a0e392b40fa
  - [BlueZ,v3,3/3] shared/vcp: Corrected handle size to accommodate included service
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=4d714becb7df

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

[PATCH BlueZ v2 1/1] shared/vcp: Fixed the crash observed with VOCS

[Nitin Jadhav]

Root cause
- There are two types of database- Remote and Local (rdb and ldb).
- In client mode currently the code was written to access ldb.

Fix
- Correcting it, to access rdb has resolved the problem in VOCS.
- Same correction is done for VCS.

Reported-by: Pauli Virtanen <pav@iki.fi>
---
v2: Fixed GitLint and ScanBuild warnings
---
 src/shared/vcp.c | 52 +++++++++++++++++++++++++++---------------------
 1 file changed, 29 insertions(+), 23 deletions(-)

diff --git a/src/shared/vcp.c b/src/shared/vcp.c
index 74bd01729..80d4dfcd4 100644
--- a/src/shared/vcp.c
+++ b/src/shared/vcp.c
@@ -973,7 +973,7 @@ static void vocs_voaodec_read(struct gatt_db_attribute *attrib,
 	struct bt_vocs *vocs = user_data;
 	struct iovec iov;
 
-	iov.iov_base = &vocs->vocs_ao_dec;
+	iov.iov_base = vocs->vocs_ao_dec;
 	iov.iov_len = strlen(vocs->vocs_ao_dec);
 
 	gatt_db_attribute_read_result(attrib, id, 0, iov.iov_base,
@@ -998,10 +998,12 @@ static struct bt_vcs *vcs_new(struct gatt_db *db, struct bt_vcp_db *vdb)
 
 	/* Populate DB with VCS attributes */
 	bt_uuid16_create(&uuid, VCS_UUID);
-	vcs->service = gatt_db_add_service(db, &uuid, true, 9);
+
+	vcs->service = gatt_db_add_service(db, &uuid, true, 10);
 	gatt_db_service_add_included(vcs->service, vdb->vocs->service);
 	gatt_db_service_set_active(vdb->vocs->service, true);
 
+
 	bt_uuid16_create(&uuid, VOL_STATE_CHRC_UUID);
 	vcs->vs = gatt_db_service_add_characteristic(vcs->service,
 					&uuid,
@@ -1385,11 +1387,12 @@ static void read_vocs_audio_location(struct bt_vcp *vcp, bool success,
 				     const uint8_t *value, uint16_t length,
 				     void *user_data)
 {
-	uint32_t *vocs_audio_loc;
-	struct iovec iov = {
-		.iov_base = (void *) value,
-		.iov_len = length,
-	};
+	uint32_t vocs_audio_loc;
+
+	if (!value) {
+		DBG(vcp, "Unable to get VOCS Audio Location");
+		return;
+	}
 
 	if (!success) {
 		DBG(vcp, "Unable to read VOCS Audio Location: error 0x%02x",
@@ -1397,26 +1400,22 @@ static void read_vocs_audio_location(struct bt_vcp *vcp, bool success,
 		return;
 	}
 
-	vocs_audio_loc = iov_pull_mem(&iov, sizeof(uint32_t));
-	if (!*vocs_audio_loc) {
-		DBG(vcp, "Unable to get VOCS Audio Location");
-		return;
-	}
+	memcpy(&vocs_audio_loc, value, length);
 
-	DBG(vcp, "VOCS Audio Loc:%x", *vocs_audio_loc);
+	DBG(vcp, "VOCS Audio Loc:%x", vocs_audio_loc);
 }
 
-
 static void read_vocs_audio_descriptor(struct bt_vcp *vcp, bool success,
 				       uint8_t att_ecode,
 				       const uint8_t *value, uint16_t length,
 				       void *user_data)
 {
 	char *vocs_ao_dec_r;
-	struct iovec iov = {
-		.iov_base = (void *) value,
-		.iov_len = length,
-	};
+
+	if (!value) {
+		DBG(vcp, "Unable to get VOCS Audio Descriptor");
+		return;
+	}
 
 	if (!success) {
 		DBG(vcp, "Unable to read VOCS Audio Descriptor: error 0x%02x",
@@ -1424,13 +1423,20 @@ static void read_vocs_audio_descriptor(struct bt_vcp *vcp, bool success,
 		return;
 	}
 
-	vocs_ao_dec_r = iov_pull_mem(&iov, length);
-	if (!*vocs_ao_dec_r) {
+	vocs_ao_dec_r = malloc(length+1);
+	memset(vocs_ao_dec_r, 0, length+1);
+
+	memcpy(vocs_ao_dec_r, value, length);
+
+	if (!vocs_ao_dec_r) {
 		DBG(vcp, "Unable to get VOCS Audio Descriptor");
 		return;
 	}
 
-	DBG(vcp, "VOCS Audio Descriptor:%s", *vocs_ao_dec_r);
+	DBG(vcp, "VOCS Audio Descriptor: %s", vocs_ao_dec_r);
+
+	free(vocs_ao_dec_r);
+	vocs_ao_dec_r = NULL;
 }
 
 static void vcp_pending_destroy(void *data)
@@ -1719,10 +1725,10 @@ bool bt_vcp_attach(struct bt_vcp *vcp, struct bt_gatt_client *client)
 		return false;
 
 	bt_uuid16_create(&uuid, VCS_UUID);
-	gatt_db_foreach_service(vcp->ldb->db, &uuid, foreach_vcs_service, vcp);
+	gatt_db_foreach_service(vcp->rdb->db, &uuid, foreach_vcs_service, vcp);
 
 	bt_uuid16_create(&uuid, VOL_OFFSET_CS_UUID);
-	gatt_db_foreach_service(vcp->ldb->db, &uuid, foreach_vocs_service, vcp);
+	gatt_db_foreach_service(vcp->rdb->db, &uuid, foreach_vocs_service, vcp);
 
 	return true;
 }
-- 
2.34.1

RE: Fixed the crash observed with VOCS

[bluez.test.bot]

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

---Test result---

Test Summary:
CheckPatch                    PASS      0.51 seconds
GitLint                       PASS      0.32 seconds
BuildEll                      PASS      33.95 seconds
BluezMake                     PASS      977.84 seconds
MakeCheck                     PASS      13.43 seconds
MakeDistcheck                 PASS      196.12 seconds
CheckValgrind                 PASS      317.14 seconds
CheckSmatch                   PASS      418.65 seconds
bluezmakeextell               PASS      129.27 seconds
IncrementalBuild              PASS      811.74 seconds
ScanBuild                     PASS      1304.39 seconds



---
Regards,
Linux Bluetooth