| Message ID | 20200605142541.28412-5-amitx.k.singh@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | bluez:update handle for large database | 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.
While we are preparing for reviewing the patches, we found the following
issue/warning.
Test Result:
checkpatch Failed
Outputs:
ERROR:SPACING: space required before the open parenthesis '('
#32: FILE: src/shared/gatt-client.c:1394:
+ if(gatt_db_attribute_write(op->hash, 0, value, len, 0, NULL,
WARNING:LONG_LINE: line over 80 characters
#33: FILE: src/shared/gatt-client.c:1395:
+ db_hash_write_value_cb, client)) {
ERROR:SPACING: space required after that ',' (ctx:VxV)
#34: FILE: src/shared/gatt-client.c:1396:
+ util_debug(client->debug_callback, client->debug_data,"DB Hash match write: skipping discovery");
^
ERROR:SPACING: space required before the open parenthesis '('
#52: FILE: src/shared/gatt-db.c:348:
+ if(hash.iov[i].iov_base)
ERROR:SPACING: space required before the open parenthesis '('
#56: FILE: src/shared/gatt-db.c:352:
+ if(hash.iov)
- total: 4 errors, 1 warnings, 58 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.
Your patch has style problems, please review.
NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED SPLIT_STRING
NOTE: If any of the errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
---
Regards,
Linux Bluetooth
Hi Amit, On Fri, Jun 5, 2020 at 7:30 AM Amitsi5x <amitx.k.singh@intel.com> wrote: > > From: amit <amitx.k.singh@intel.com> > > Update handle for large database and > added condition before free to avoid double free > > Signed-off-by: amit <amitx.k.singh@intel.com> > --- > src/shared/gatt-client.c | 12 +++++++----- > src/shared/gatt-db.c | 15 +++++++++------ > 2 files changed, 16 insertions(+), 11 deletions(-) > > diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c > index 19ff6ab65..3cb6ae443 100644 > --- a/src/shared/gatt-client.c > +++ b/src/shared/gatt-client.c > @@ -1131,8 +1131,6 @@ static void discover_secondary_cb(bool success, uint8_t att_ecode, > success = false; > goto done; > } > - /* Database has changed adjust last handle */ > - op->last = end; > } > > /* Update pending list */ > @@ -1392,9 +1390,13 @@ static void db_hash_read_cb(bool success, uint8_t att_ecode, > util_hexdump(' ', value, len, client->debug_callback, > client->debug_data); > > - /* Store ithe new hash in the db */ > - gatt_db_attribute_write(op->hash, 0, value, len, 0, NULL, > - db_hash_write_value_cb, client); > + /* Store the new hash in the db */ > + if(gatt_db_attribute_write(op->hash, 0, value, len, 0, NULL, > + db_hash_write_value_cb, client)) { > + util_debug(client->debug_callback, client->debug_data,"DB Hash match write: skipping discovery"); > + queue_remove_all(op->pending_svcs, NULL, NULL, NULL); Not following this change, if we got to write the db hash that means the old value did not match. > + } > + > > discover: > if (!op->success) { > diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c > index b44f7b5e9..15af4c20a 100644 > --- a/src/shared/gatt-db.c > +++ b/src/shared/gatt-db.c > @@ -344,10 +344,15 @@ static bool db_hash_update(void *user_data) > gatt_db_foreach_service(db, NULL, service_gen_hash_m, &hash); > bt_crypto_gatt_hash(db->crypto, hash.iov, db->next_handle, db->hash); > > - for (i = 0; i < hash.i; i++) > - free(hash.iov[i].iov_base); > + for (i = 0; i < hash.i; i++) { > + if(hash.iov[i].iov_base) > + free(hash.iov[i].iov_base); > + } > + > + if(hash.iov) > + free(hash.iov); > > - free(hash.iov); > + hash.iov = NULL; I believe this error was actually introduced by your changes actually, see below. > return false; > } > @@ -689,7 +694,7 @@ struct gatt_db_attribute *gatt_db_insert_service(struct gatt_db *db, > service->num_handles = num_handles; > > /* Fast-forward next_handle if the new service was added to the end */ > - db->next_handle = MAX(handle + num_handles, db->next_handle); > + db->next_handle += num_handles; Note that if the service was not added to the end this starts adding gaps in between, so I'm afraid I will have to nack this change. > return service->attributes[0]; > > @@ -811,8 +816,6 @@ service_insert_characteristic(struct gatt_db_service *service, > * declaration. All characteristic definitions shall have a > * Characteristic Value declaration. > */ > - if (handle == UINT16_MAX) > - return NULL; This perhaps is the real reason, it seems to me that you have more than UINT16_MAX handles so the handles loop around and start over from 0 which is invalid and will most likely cause double frees etc and they can be multiple attributes assigned to the same handle. How big is the database you are trying to test? If that is going past UINT16_MAX it is probably broken and nothing can be done to fix it on the client side which is why we stop adding attributes after it. > i = get_attribute_index(service, 1); > if (!i) > -- > 2.17.1 >
Hi Luiz > -----Original Message----- > From: Luiz Augusto von Dentz <luiz.dentz@gmail.com> > Sent: Friday, June 5, 2020 10:46 PM > To: Singh, AmitX K <amitx.k.singh@intel.com> > Cc: linux-bluetooth@vger.kernel.org > Subject: Re: [PATCH] bluez:update handle for large database > > Hi Amit, > > On Fri, Jun 5, 2020 at 7:30 AM Amitsi5x <amitx.k.singh@intel.com> wrote: > > > > From: amit <amitx.k.singh@intel.com> > > > > Update handle for large database and > > added condition before free to avoid double free > > > > Signed-off-by: amit <amitx.k.singh@intel.com> > > --- > > src/shared/gatt-client.c | 12 +++++++----- > > src/shared/gatt-db.c | 15 +++++++++------ > > 2 files changed, 16 insertions(+), 11 deletions(-) > > > > diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c index > > 19ff6ab65..3cb6ae443 100644 > > --- a/src/shared/gatt-client.c > > +++ b/src/shared/gatt-client.c > > @@ -1131,8 +1131,6 @@ static void discover_secondary_cb(bool success, > uint8_t att_ecode, > > success = false; > > goto done; > > } > > - /* Database has changed adjust last handle */ > > - op->last = end; > > } > > > > /* Update pending list */ @@ -1392,9 +1390,13 @@ > > static void db_hash_read_cb(bool success, uint8_t att_ecode, > > util_hexdump(' ', value, len, client->debug_callback, > > client->debug_data); > > > > - /* Store ithe new hash in the db */ > > - gatt_db_attribute_write(op->hash, 0, value, len, 0, NULL, > > - db_hash_write_value_cb, client); > > + /* Store the new hash in the db */ > > + if(gatt_db_attribute_write(op->hash, 0, value, len, 0, NULL, > > + db_hash_write_value_cb, client)) { > > + util_debug(client->debug_callback, client->debug_data,"DB Hash > match write: skipping discovery"); > > + queue_remove_all(op->pending_svcs, NULL, NULL, NULL); > > Not following this change, if we got to write the db hash that means the old > value did not match. > When we verify the PTS test case GATT/CL/GAD/BV-02-C test case, the Test case demands to perform multiple connections to the PTS device with different database upon each connection, where the current code does not update the database hash on each connect iteration that yields to seg fault. Added support for remove pending service if any after updating new hash in database. > > + } > > + > > > > discover: > > if (!op->success) { > > diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c index > > b44f7b5e9..15af4c20a 100644 > > --- a/src/shared/gatt-db.c > > +++ b/src/shared/gatt-db.c > > @@ -344,10 +344,15 @@ static bool db_hash_update(void *user_data) > > gatt_db_foreach_service(db, NULL, service_gen_hash_m, &hash); > > bt_crypto_gatt_hash(db->crypto, hash.iov, db->next_handle, > > db->hash); > > > > - for (i = 0; i < hash.i; i++) > > - free(hash.iov[i].iov_base); > > + for (i = 0; i < hash.i; i++) { > > + if(hash.iov[i].iov_base) > > + free(hash.iov[i].iov_base); > > + } > > + > > + if(hash.iov) > > + free(hash.iov); > > > > - free(hash.iov); > > + hash.iov = NULL; > > I believe this error was actually introduced by your changes actually, see > below. > > > return false; > > } > > @@ -689,7 +694,7 @@ struct gatt_db_attribute > *gatt_db_insert_service(struct gatt_db *db, > > service->num_handles = num_handles; > > > > /* Fast-forward next_handle if the new service was added to the end > */ > > - db->next_handle = MAX(handle + num_handles, db->next_handle); > > + db->next_handle += num_handles; > > Note that if the service was not added to the end this starts adding gaps in > between, so I'm afraid I will have to nack this change. > > > return service->attributes[0]; > > > > @@ -811,8 +816,6 @@ service_insert_characteristic(struct gatt_db_service > *service, > > * declaration. All characteristic definitions shall have a > > * Characteristic Value declaration. > > */ > > - if (handle == UINT16_MAX) > > - return NULL; > > This perhaps is the real reason, it seems to me that you have more than > UINT16_MAX handles so the handles loop around and start over from > 0 which is invalid and will most likely cause double frees etc and they can be > multiple attributes assigned to the same handle. How big is the database you > are trying to test? If that is going past UINT16_MAX it is probably broken and > nothing can be done to fix it on the client side which is why we stop adding > attributes after it. > When we verify the PTS test case GATT/CL/GAD/BV-02-C test case, the TC demands to perform multiple connections to the PTS device with different database upon each connection. In one connection setup database having handle value is UINT16_MAX, where the current code does not able when handle is max . > > i = get_attribute_index(service, 1); > > if (!i) > > -- > > 2.17.1 > > > > > -- > Luiz Augusto von Dentz
diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c index 19ff6ab65..3cb6ae443 100644 --- a/src/shared/gatt-client.c +++ b/src/shared/gatt-client.c @@ -1131,8 +1131,6 @@ static void discover_secondary_cb(bool success, uint8_t att_ecode, success = false; goto done; } - /* Database has changed adjust last handle */ - op->last = end; } /* Update pending list */ @@ -1392,9 +1390,13 @@ static void db_hash_read_cb(bool success, uint8_t att_ecode, util_hexdump(' ', value, len, client->debug_callback, client->debug_data); - /* Store ithe new hash in the db */ - gatt_db_attribute_write(op->hash, 0, value, len, 0, NULL, - db_hash_write_value_cb, client); + /* Store the new hash in the db */ + if(gatt_db_attribute_write(op->hash, 0, value, len, 0, NULL, + db_hash_write_value_cb, client)) { + util_debug(client->debug_callback, client->debug_data,"DB Hash match write: skipping discovery"); + queue_remove_all(op->pending_svcs, NULL, NULL, NULL); + } + discover: if (!op->success) { diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c index b44f7b5e9..15af4c20a 100644 --- a/src/shared/gatt-db.c +++ b/src/shared/gatt-db.c @@ -344,10 +344,15 @@ static bool db_hash_update(void *user_data) gatt_db_foreach_service(db, NULL, service_gen_hash_m, &hash); bt_crypto_gatt_hash(db->crypto, hash.iov, db->next_handle, db->hash); - for (i = 0; i < hash.i; i++) - free(hash.iov[i].iov_base); + for (i = 0; i < hash.i; i++) { + if(hash.iov[i].iov_base) + free(hash.iov[i].iov_base); + } + + if(hash.iov) + free(hash.iov); - free(hash.iov); + hash.iov = NULL; return false; } @@ -689,7 +694,7 @@ struct gatt_db_attribute *gatt_db_insert_service(struct gatt_db *db, service->num_handles = num_handles; /* Fast-forward next_handle if the new service was added to the end */ - db->next_handle = MAX(handle + num_handles, db->next_handle); + db->next_handle += num_handles; return service->attributes[0]; @@ -811,8 +816,6 @@ service_insert_characteristic(struct gatt_db_service *service, * declaration. All characteristic definitions shall have a * Characteristic Value declaration. */ - if (handle == UINT16_MAX) - return NULL; i = get_attribute_index(service, 1); if (!i)