| Message ID | 20250527132831.232633-1-luiz.dentz@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [BlueZ,v1] shared/bap: Fix not handling read offset | expand |
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=966677 ---Test result--- Test Summary: CheckPatch PENDING 0.27 seconds GitLint PENDING 0.31 seconds BuildEll PASS 20.32 seconds BluezMake PASS 2679.68 seconds MakeCheck PASS 20.51 seconds MakeDistcheck PASS 201.13 seconds CheckValgrind PASS 274.93 seconds CheckSmatch WARNING 301.75 seconds bluezmakeextell PASS 128.23 seconds IncrementalBuild PENDING 0.35 seconds ScanBuild PASS 914.91 seconds Details ############################## Test: CheckPatch - PENDING Desc: Run checkpatch.pl script Output: ############################## Test: GitLint - PENDING Desc: Run gitlint Output: ############################## Test: CheckSmatch - WARNING Desc: Run smatch tool with source Output: src/shared/bap.c:315: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:315: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:315: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: IncrementalBuild - PENDING Desc: Incremental build with the patches in the series Output: --- Regards, Linux Bluetooth
Hello: This patch was applied to bluetooth/bluez.git (master) by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>: On Tue, 27 May 2025 09:28:31 -0400 you wrote: > From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > > Some attributes may need to handle offset other than 0 in case read long > procedure is used, so this properly handles that for PAC_SINK_CHRC_UUID > and PAC_SOURCE_CHRC_UUID. > > In addition to PAC record this also uses > gatt_db_attribute_set_fixed_length for attributes that are considered of > fixed size so gatt_db can handle offset directly. > > [...] Here is the summary with links: - [BlueZ,v1] shared/bap: Fix not handling read offset https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=088de118d17c You are awesome, thank you!
Hi Luiz, > [ EXTERNAL EMAIL ] > > From: Luiz Augusto von Dentz<luiz.von.dentz@intel.com> > > Some attributes may need to handle offset other than 0 in case read long > procedure is used, so this properly handles that for PAC_SINK_CHRC_UUID > and PAC_SOURCE_CHRC_UUID. > > In addition to PAC record this also uses > gatt_db_attribute_set_fixed_length for attributes that are considered of > fixed size so gatt_db can handle offset directly. > > Fixes:https://github.com/bluez/bluez/issues/1294 > --- > src/shared/bap.c | 28 ++++++++++++++++++++++++---- > 1 file changed, 24 insertions(+), 4 deletions(-) > > diff --git a/src/shared/bap.c b/src/shared/bap.c > index 4bb6e08b5379..f0c6f64855c4 100644 > --- a/src/shared/bap.c > +++ b/src/shared/bap.c > @@ -516,8 +516,15 @@ static void pacs_sink_read(struct gatt_db_attribute *attrib, > queue_foreach(bdb->sinks, pac_foreach, &iov); > queue_foreach(bdb->broadcast_sinks, pac_foreach, &iov); > > - gatt_db_attribute_read_result(attrib, id, 0, iov.iov_base, > - iov.iov_len); > + if (offset > iov.iov_len) { > + gatt_db_attribute_read_result(attrib, id, > + BT_ATT_ERROR_INVALID_OFFSET, > + NULL, 0); > + return; > + } > + > + gatt_db_attribute_read_result(attrib, id, 0, iov.iov_base + offset, > + iov.iov_len - offset); > } > > static void pacs_sink_loc_read(struct gatt_db_attribute *attrib, > @@ -549,8 +556,15 @@ static void pacs_source_read(struct gatt_db_attribute *attrib, > > queue_foreach(bdb->sources, pac_foreach, &iov); > > - gatt_db_attribute_read_result(attrib, id, 0, iov.iov_base, > - iov.iov_len); > + if (offset > iov.iov_len) { > + gatt_db_attribute_read_result(attrib, id, > + BT_ATT_ERROR_INVALID_OFFSET, > + NULL, 0); > + return; > + } > + > + gatt_db_attribute_read_result(attrib, id, 0, iov.iov_base + offset, > + iov.iov_len - offset); > } > > static void pacs_source_loc_read(struct gatt_db_attribute *attrib, > @@ -627,6 +641,7 @@ static struct bt_pacs *pacs_new(struct gatt_db *db) > BT_GATT_CHRC_PROP_NOTIFY, > pacs_sink_loc_read, NULL, > pacs); > + gatt_db_attribute_set_fixed_length(pacs->sink_loc, sizeof(uint32_t)); > > pacs->sink_loc_ccc = gatt_db_service_add_ccc(pacs->service, > BT_ATT_PERM_READ | BT_ATT_PERM_WRITE); > @@ -649,6 +664,7 @@ static struct bt_pacs *pacs_new(struct gatt_db *db) > BT_GATT_CHRC_PROP_NOTIFY, > pacs_source_loc_read, NULL, > pacs); > + gatt_db_attribute_set_fixed_length(pacs->source_loc, sizeof(uint32_t)); > > pacs->source_loc_ccc = gatt_db_service_add_ccc(pacs->service, > BT_ATT_PERM_READ | BT_ATT_PERM_WRITE); > @@ -659,6 +675,8 @@ static struct bt_pacs *pacs_new(struct gatt_db *db) > BT_GATT_CHRC_PROP_READ | > BT_GATT_CHRC_PROP_NOTIFY, > pacs_context_read, NULL, pacs); > + gatt_db_attribute_set_fixed_length(pacs->context, > + sizeof(struct bt_pacs_context)); > > pacs->context_ccc = gatt_db_service_add_ccc(pacs->service, > BT_ATT_PERM_READ | BT_ATT_PERM_WRITE); > @@ -671,6 +689,8 @@ static struct bt_pacs *pacs_new(struct gatt_db *db) > BT_GATT_CHRC_PROP_NOTIFY, > pacs_supported_context_read, NULL, > pacs); > + gatt_db_attribute_set_fixed_length(pacs->supported_context, > + sizeof(struct bt_pacs_context)); > > pacs->supported_context_ccc = gatt_db_service_add_ccc(pacs->service, > BT_ATT_PERM_READ | BT_ATT_PERM_WRITE); > -- > 2.49.0 > > I am concerned about this solution. It can indeed solve the problem of using blob procedure to read sink pacs or source pacs, but if blob procedure is used to read other services, will the problem occur again? I think we should fix the issue at its root.
Hi, On Wed, May 28, 2025 at 1:21 AM Ye He <ye.he@amlogic.com> wrote: > > Hi Luiz, > > > [ EXTERNAL EMAIL ] > > > > From: Luiz Augusto von Dentz<luiz.von.dentz@intel.com> > > > > Some attributes may need to handle offset other than 0 in case read long > > procedure is used, so this properly handles that for PAC_SINK_CHRC_UUID > > and PAC_SOURCE_CHRC_UUID. > > > > In addition to PAC record this also uses > > gatt_db_attribute_set_fixed_length for attributes that are considered of > > fixed size so gatt_db can handle offset directly. > > > > Fixes:https://github.com/bluez/bluez/issues/1294 > > --- > > src/shared/bap.c | 28 ++++++++++++++++++++++++---- > > 1 file changed, 24 insertions(+), 4 deletions(-) > > > > diff --git a/src/shared/bap.c b/src/shared/bap.c > > index 4bb6e08b5379..f0c6f64855c4 100644 > > --- a/src/shared/bap.c > > +++ b/src/shared/bap.c > > @@ -516,8 +516,15 @@ static void pacs_sink_read(struct gatt_db_attribute *attrib, > > queue_foreach(bdb->sinks, pac_foreach, &iov); > > queue_foreach(bdb->broadcast_sinks, pac_foreach, &iov); > > > > - gatt_db_attribute_read_result(attrib, id, 0, iov.iov_base, > > - iov.iov_len); > > + if (offset > iov.iov_len) { > > + gatt_db_attribute_read_result(attrib, id, > > + BT_ATT_ERROR_INVALID_OFFSET, > > + NULL, 0); > > + return; > > + } > > + > > + gatt_db_attribute_read_result(attrib, id, 0, iov.iov_base + offset, > > + iov.iov_len - offset); > > } > > > > static void pacs_sink_loc_read(struct gatt_db_attribute *attrib, > > @@ -549,8 +556,15 @@ static void pacs_source_read(struct gatt_db_attribute *attrib, > > > > queue_foreach(bdb->sources, pac_foreach, &iov); > > > > - gatt_db_attribute_read_result(attrib, id, 0, iov.iov_base, > > - iov.iov_len); > > + if (offset > iov.iov_len) { > > + gatt_db_attribute_read_result(attrib, id, > > + BT_ATT_ERROR_INVALID_OFFSET, > > + NULL, 0); > > + return; > > + } > > + > > + gatt_db_attribute_read_result(attrib, id, 0, iov.iov_base + offset, > > + iov.iov_len - offset); > > } > > > > static void pacs_source_loc_read(struct gatt_db_attribute *attrib, > > @@ -627,6 +641,7 @@ static struct bt_pacs *pacs_new(struct gatt_db *db) > > BT_GATT_CHRC_PROP_NOTIFY, > > pacs_sink_loc_read, NULL, > > pacs); > > + gatt_db_attribute_set_fixed_length(pacs->sink_loc, sizeof(uint32_t)); > > > > pacs->sink_loc_ccc = gatt_db_service_add_ccc(pacs->service, > > BT_ATT_PERM_READ | BT_ATT_PERM_WRITE); > > @@ -649,6 +664,7 @@ static struct bt_pacs *pacs_new(struct gatt_db *db) > > BT_GATT_CHRC_PROP_NOTIFY, > > pacs_source_loc_read, NULL, > > pacs); > > + gatt_db_attribute_set_fixed_length(pacs->source_loc, sizeof(uint32_t)); > > > > pacs->source_loc_ccc = gatt_db_service_add_ccc(pacs->service, > > BT_ATT_PERM_READ | BT_ATT_PERM_WRITE); > > @@ -659,6 +675,8 @@ static struct bt_pacs *pacs_new(struct gatt_db *db) > > BT_GATT_CHRC_PROP_READ | > > BT_GATT_CHRC_PROP_NOTIFY, > > pacs_context_read, NULL, pacs); > > + gatt_db_attribute_set_fixed_length(pacs->context, > > + sizeof(struct bt_pacs_context)); > > > > pacs->context_ccc = gatt_db_service_add_ccc(pacs->service, > > BT_ATT_PERM_READ | BT_ATT_PERM_WRITE); > > @@ -671,6 +689,8 @@ static struct bt_pacs *pacs_new(struct gatt_db *db) > > BT_GATT_CHRC_PROP_NOTIFY, > > pacs_supported_context_read, NULL, > > pacs); > > + gatt_db_attribute_set_fixed_length(pacs->supported_context, > > + sizeof(struct bt_pacs_context)); > > > > pacs->supported_context_ccc = gatt_db_service_add_ccc(pacs->service, > > BT_ATT_PERM_READ | BT_ATT_PERM_WRITE); > > -- > > 2.49.0 > > > > > I am concerned about this solution. It can indeed solve the problem of > using blob procedure to read sink pacs or source pacs, but if blob > procedure is used to read other services, will the problem occur again? > I think we should fix the issue at its root. Well Ive already explained that doesn't work since the offset is already part of the interface, so both internal and external attribute are expected to return that data following the offset: https://github.com/bluez/bluez/blob/master/doc/org.bluez.GattCharacteristic.rst#arraybyte-readvaluedict-options Also note that although normally the offset means the offset of data read with a read long operation I wouldn't be surprised if the vendor does use for something else, e.g. as control point index or something like that. What is perhaps more efficient is to cache the read operations so we can detect when it is a read long operation taking place we just use the cache to respond, that said Read Blob that doesn't continue from previous offset shall still be sent to the callback to respond.
diff --git a/src/shared/bap.c b/src/shared/bap.c index 4bb6e08b5379..f0c6f64855c4 100644 --- a/src/shared/bap.c +++ b/src/shared/bap.c @@ -516,8 +516,15 @@ static void pacs_sink_read(struct gatt_db_attribute *attrib, queue_foreach(bdb->sinks, pac_foreach, &iov); queue_foreach(bdb->broadcast_sinks, pac_foreach, &iov); - gatt_db_attribute_read_result(attrib, id, 0, iov.iov_base, - iov.iov_len); + if (offset > iov.iov_len) { + gatt_db_attribute_read_result(attrib, id, + BT_ATT_ERROR_INVALID_OFFSET, + NULL, 0); + return; + } + + gatt_db_attribute_read_result(attrib, id, 0, iov.iov_base + offset, + iov.iov_len - offset); } static void pacs_sink_loc_read(struct gatt_db_attribute *attrib, @@ -549,8 +556,15 @@ static void pacs_source_read(struct gatt_db_attribute *attrib, queue_foreach(bdb->sources, pac_foreach, &iov); - gatt_db_attribute_read_result(attrib, id, 0, iov.iov_base, - iov.iov_len); + if (offset > iov.iov_len) { + gatt_db_attribute_read_result(attrib, id, + BT_ATT_ERROR_INVALID_OFFSET, + NULL, 0); + return; + } + + gatt_db_attribute_read_result(attrib, id, 0, iov.iov_base + offset, + iov.iov_len - offset); } static void pacs_source_loc_read(struct gatt_db_attribute *attrib, @@ -627,6 +641,7 @@ static struct bt_pacs *pacs_new(struct gatt_db *db) BT_GATT_CHRC_PROP_NOTIFY, pacs_sink_loc_read, NULL, pacs); + gatt_db_attribute_set_fixed_length(pacs->sink_loc, sizeof(uint32_t)); pacs->sink_loc_ccc = gatt_db_service_add_ccc(pacs->service, BT_ATT_PERM_READ | BT_ATT_PERM_WRITE); @@ -649,6 +664,7 @@ static struct bt_pacs *pacs_new(struct gatt_db *db) BT_GATT_CHRC_PROP_NOTIFY, pacs_source_loc_read, NULL, pacs); + gatt_db_attribute_set_fixed_length(pacs->source_loc, sizeof(uint32_t)); pacs->source_loc_ccc = gatt_db_service_add_ccc(pacs->service, BT_ATT_PERM_READ | BT_ATT_PERM_WRITE); @@ -659,6 +675,8 @@ static struct bt_pacs *pacs_new(struct gatt_db *db) BT_GATT_CHRC_PROP_READ | BT_GATT_CHRC_PROP_NOTIFY, pacs_context_read, NULL, pacs); + gatt_db_attribute_set_fixed_length(pacs->context, + sizeof(struct bt_pacs_context)); pacs->context_ccc = gatt_db_service_add_ccc(pacs->service, BT_ATT_PERM_READ | BT_ATT_PERM_WRITE); @@ -671,6 +689,8 @@ static struct bt_pacs *pacs_new(struct gatt_db *db) BT_GATT_CHRC_PROP_NOTIFY, pacs_supported_context_read, NULL, pacs); + gatt_db_attribute_set_fixed_length(pacs->supported_context, + sizeof(struct bt_pacs_context)); pacs->supported_context_ccc = gatt_db_service_add_ccc(pacs->service, BT_ATT_PERM_READ | BT_ATT_PERM_WRITE);
From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> Some attributes may need to handle offset other than 0 in case read long procedure is used, so this properly handles that for PAC_SINK_CHRC_UUID and PAC_SOURCE_CHRC_UUID. In addition to PAC record this also uses gatt_db_attribute_set_fixed_length for attributes that are considered of fixed size so gatt_db can handle offset directly. Fixes: https://github.com/bluez/bluez/issues/1294 --- src/shared/bap.c | 28 ++++++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-)