| Message ID | 20200507141055.Bluez.1.Ie4c027829af0f3ca2ba0e532706584a554a69d38@changeid (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | [Bluez] shared/gatt-db: Check for null pointers | expand |
Hi Abhishek, On Thu, May 7, 2020 at 2:11 PM Abhishek Pandit-Subedi <abhishekpandit@chromium.org> wrote: > > Make sure the attribute, service and service attributes are not null > before accessing them. For this type of problem I would recommend using valgrind to backtrace, since this is not very readable we might not want to add it to the git history, that said the actual problem seem to be related to using gatt_db_attribute_get_service_data while the service is being removed, in that case I guess it is fine to set the service to NULL before removing it to indicate the it is in the process of being removed, we could perhaps introduce a unit test for this type of error so we can detect regressions with the likes of make check and CI when that starts to use it. > The problem was seen with the following stack trace: > > 0 bluetoothd!gatt_db_attribute_get_service_data [gatt-db.c : 1648 + 0x0] > rax = 0x0000000000000000 rdx = 0x00007ffce8cf70ec > rcx = 0x0000000000000000 rbx = 0x0000000000000000 > rsi = 0x00007ffce8cf70ee rdi = 0x00005a56611f05c0 > rbp = 0x00007ffce8cf70d0 rsp = 0x00007ffce8cf70b0 > r8 = 0x0000000000000000 r9 = 0x0000000000000050 > r10 = 0x0000000000000073 r11 = 0x0000000000000246 > r12 = 0x00005a56611cea10 r13 = 0x00005a56611efd90 > r14 = 0x0000000000000000 r15 = 0x00005a565f3eed8d > rip = 0x00005a565f48147e > Found by: given as instruction pointer in context > 1 bluetoothd!discovery_op_complete [gatt-client.c : 386 + 0x14] > rbx = 0x00005a56611e9d30 rbp = 0x00007ffce8cf7120 > rsp = 0x00007ffce8cf70e0 r12 = 0x00005a56611cea10 > r13 = 0x00005a56611efd90 r14 = 0x00007ffce8cf70ec > r15 = 0x00005a565f3eed8d rip = 0x00005a565f47a6bc > Found by: call frame info > 2 bluetoothd!discover_chrcs_cb [gatt-client.c : 1000 + 0xf] > rbx = 0x0000000000000000 rbp = 0x00007ffce8cf71d0 > rsp = 0x00007ffce8cf7130 r12 = 0x000000000000000a > r13 = 0x00005a56611de920 r14 = 0x00005a56611cea10 > r15 = 0x00007ffce8cf7188 rip = 0x00005a565f47b18a > Found by: call frame info > 3 bluetoothd!discovery_op_complete [gatt-helpers.c : 628 + 0xc] > rbx = 0x00005a56611f0430 rbp = 0x00007ffce8cf71f0 > rsp = 0x00007ffce8cf71e0 r12 = 0x00005a56611ea5a0 > r13 = 0x00005a56611cd430 r14 = 0x00005a56611f0430 > r15 = 0x00005a566119bc01 rip = 0x00005a565f47d60e > Found by: call frame info > 4 bluetoothd!discover_chrcs_cb [gatt-helpers.c : 1250 + 0xe] > rbx = 0x00005a56611bf0f1 rbp = 0x00007ffce8cf7240 > rsp = 0x00007ffce8cf7200 r12 = 0x00005a56611ea5a0 > r13 = 0x00005a56611cd430 r14 = 0x00005a56611f0430 > r15 = 0x00005a566119bc01 rip = 0x00005a565f47cc7a > Found by: call frame info > > --- > > src/shared/gatt-db.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c > index b44f7b5e9..2432bdfd4 100644 > --- a/src/shared/gatt-db.c > +++ b/src/shared/gatt-db.c > @@ -513,6 +513,7 @@ bool gatt_db_remove_service(struct gatt_db *db, > return false; > > service = attrib->service; > + attrib->service = NULL; > > queue_remove(db->services, service); > > @@ -1605,7 +1606,7 @@ bool gatt_db_attribute_get_service_data(const struct gatt_db_attribute *attrib, > struct gatt_db_service *service; > struct gatt_db_attribute *decl; > > - if (!attrib) > + if (!(attrib && attrib->service && attrib->service->attributes)) > return false; > > service = attrib->service; > -- > 2.26.2.645.ge9eca65c58-goog >
diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c index b44f7b5e9..2432bdfd4 100644 --- a/src/shared/gatt-db.c +++ b/src/shared/gatt-db.c @@ -513,6 +513,7 @@ bool gatt_db_remove_service(struct gatt_db *db, return false; service = attrib->service; + attrib->service = NULL; queue_remove(db->services, service); @@ -1605,7 +1606,7 @@ bool gatt_db_attribute_get_service_data(const struct gatt_db_attribute *attrib, struct gatt_db_service *service; struct gatt_db_attribute *decl; - if (!attrib) + if (!(attrib && attrib->service && attrib->service->attributes)) return false; service = attrib->service;
Make sure the attribute, service and service attributes are not null before accessing them. The problem was seen with the following stack trace: 0 bluetoothd!gatt_db_attribute_get_service_data [gatt-db.c : 1648 + 0x0] rax = 0x0000000000000000 rdx = 0x00007ffce8cf70ec rcx = 0x0000000000000000 rbx = 0x0000000000000000 rsi = 0x00007ffce8cf70ee rdi = 0x00005a56611f05c0 rbp = 0x00007ffce8cf70d0 rsp = 0x00007ffce8cf70b0 r8 = 0x0000000000000000 r9 = 0x0000000000000050 r10 = 0x0000000000000073 r11 = 0x0000000000000246 r12 = 0x00005a56611cea10 r13 = 0x00005a56611efd90 r14 = 0x0000000000000000 r15 = 0x00005a565f3eed8d rip = 0x00005a565f48147e Found by: given as instruction pointer in context 1 bluetoothd!discovery_op_complete [gatt-client.c : 386 + 0x14] rbx = 0x00005a56611e9d30 rbp = 0x00007ffce8cf7120 rsp = 0x00007ffce8cf70e0 r12 = 0x00005a56611cea10 r13 = 0x00005a56611efd90 r14 = 0x00007ffce8cf70ec r15 = 0x00005a565f3eed8d rip = 0x00005a565f47a6bc Found by: call frame info 2 bluetoothd!discover_chrcs_cb [gatt-client.c : 1000 + 0xf] rbx = 0x0000000000000000 rbp = 0x00007ffce8cf71d0 rsp = 0x00007ffce8cf7130 r12 = 0x000000000000000a r13 = 0x00005a56611de920 r14 = 0x00005a56611cea10 r15 = 0x00007ffce8cf7188 rip = 0x00005a565f47b18a Found by: call frame info 3 bluetoothd!discovery_op_complete [gatt-helpers.c : 628 + 0xc] rbx = 0x00005a56611f0430 rbp = 0x00007ffce8cf71f0 rsp = 0x00007ffce8cf71e0 r12 = 0x00005a56611ea5a0 r13 = 0x00005a56611cd430 r14 = 0x00005a56611f0430 r15 = 0x00005a566119bc01 rip = 0x00005a565f47d60e Found by: call frame info 4 bluetoothd!discover_chrcs_cb [gatt-helpers.c : 1250 + 0xe] rbx = 0x00005a56611bf0f1 rbp = 0x00007ffce8cf7240 rsp = 0x00007ffce8cf7200 r12 = 0x00005a56611ea5a0 r13 = 0x00005a56611cd430 r14 = 0x00005a56611f0430 r15 = 0x00005a566119bc01 rip = 0x00005a565f47cc7a Found by: call frame info --- src/shared/gatt-db.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)