| Message ID | 20240103092816.22952-1-frederic.danis@collabora.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [BlueZ] shared/gatt-db: Fix munmap_chunk invalid pointer | expand |
| Context | Check | Description |
|---|---|---|
| tedd_an/pre-ci_am | success | Success |
| tedd_an/CheckPatch | success | CheckPatch PASS |
| tedd_an/GitLint | success | Gitlint PASS |
| tedd_an/BuildEll | success | Build ELL PASS |
| tedd_an/BluezMake | success | Bluez Make PASS |
| tedd_an/MakeCheck | success | Bluez Make Check PASS |
| tedd_an/MakeDistcheck | success | Make Distcheck PASS |
| tedd_an/CheckValgrind | success | Check Valgrind PASS |
| tedd_an/CheckSmatch | success | CheckSparse PASS |
| tedd_an/bluezmakeextell | success | Make External ELL PASS |
| tedd_an/IncrementalBuild | success | Incremental Build PASS |
| tedd_an/ScanBuild | success | Scan Build PASS |
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=814034 ---Test result--- Test Summary: CheckPatch PASS 0.48 seconds GitLint PASS 0.33 seconds BuildEll PASS 24.35 seconds BluezMake PASS 736.35 seconds MakeCheck PASS 12.28 seconds MakeDistcheck PASS 161.49 seconds CheckValgrind PASS 223.98 seconds CheckSmatch PASS 328.92 seconds bluezmakeextell PASS 107.46 seconds IncrementalBuild PASS 688.48 seconds ScanBuild PASS 934.60 seconds --- Regards, Linux Bluetooth
Hi Frédéric, On Wed, Jan 3, 2024 at 4:28 AM Frédéric Danis <frederic.danis@collabora.com> wrote: > > PTS test GATT/CL/GAD/BV-03-C published a service starting at handle 0xfffd > and ending at 0xffff. Don't we have a test for it under unit/test-gatt.c? Perhaps it would be a good idea to add one while doing this change. > This resets the next_handle to 0 in gatt_db_insert_service() instead of > setting it to 0x10000. Other services are added later. > This could end-up by a crash in db_hash_update() if not enough space has > been allocated for hash.iov and some entries are overwritten. I understand we don't want to loop around but handle 0x10000 is not valid either. > --- > src/shared/gatt-db.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c > index 676f963ec..d32c9a70f 100644 > --- a/src/shared/gatt-db.c > +++ b/src/shared/gatt-db.c > @@ -58,7 +58,7 @@ struct gatt_db { > struct bt_crypto *crypto; > uint8_t hash[16]; > unsigned int hash_id; > - uint16_t next_handle; > + uint32_t next_handle; I wonder if we can just set the next_handle to 0 and then check it when using it, that way it indicates that it had looped around and handle 0 is invalid already so we shouldn't allocate anything on it. > struct queue *services; > > struct queue *notify_list; > -- > 2.34.1 > >
Hi Luiz, On 03/01/2024 16:50, Luiz Augusto von Dentz wrote: > Hi Frédéric, > > On Wed, Jan 3, 2024 at 4:28 AM Frédéric Danis > <frederic.danis@collabora.com> wrote: >> PTS test GATT/CL/GAD/BV-03-C published a service starting at handle 0xfffd >> and ending at 0xffff. > Don't we have a test for it under unit/test-gatt.c? Perhaps it would > be a good idea to add one while doing this change. Yes My idea should be to add a new unordered database and run gatt_db_get_hash() on it. > >> This resets the next_handle to 0 in gatt_db_insert_service() instead of >> setting it to 0x10000. Other services are added later. >> This could end-up by a crash in db_hash_update() if not enough space has >> been allocated for hash.iov and some entries are overwritten. > I understand we don't want to loop around but handle 0x10000 is not > valid either. Afaiu the next_handle is used as: - the next available handle, with special value 0 to define an empty db - and the maximum size to allocate during db_hash_update() So, 0x10000 is not a valid handle but is a valid size. gatt_db_insert_service() is already protected to not use handle > 0xFFFF. > >> --- >> src/shared/gatt-db.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c >> index 676f963ec..d32c9a70f 100644 >> --- a/src/shared/gatt-db.c >> +++ b/src/shared/gatt-db.c >> @@ -58,7 +58,7 @@ struct gatt_db { >> struct bt_crypto *crypto; >> uint8_t hash[16]; >> unsigned int hash_id; >> - uint16_t next_handle; >> + uint32_t next_handle; > I wonder if we can just set the next_handle to 0 and then check it > when using it, that way it indicates that it had looped around and > handle 0 is invalid already so we shouldn't allocate anything on it. Not sure this can work as 0 can mean it's an empty db or a db requesting UINT16_MAX+1 elements. During this test case, it loops to 0, but as other services are added setting next_handle to another value, ending up to allocate less memory than expected (i.e. UINT16_MAX+1 elements). We may replace the next_handle by last_handle, use gatt_db_isempty() instead of the handle 0 to check for empty db, and allocate last_handle+1 in db_hash_update(). Does it seems better? Regards, Fred
diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c index 676f963ec..d32c9a70f 100644 --- a/src/shared/gatt-db.c +++ b/src/shared/gatt-db.c @@ -58,7 +58,7 @@ struct gatt_db { struct bt_crypto *crypto; uint8_t hash[16]; unsigned int hash_id; - uint16_t next_handle; + uint32_t next_handle; struct queue *services; struct queue *notify_list;