2020-05-07 21:11:45

by Abhishek Pandit-Subedi

[permalink] [raw]
Subject: [Bluez PATCH] shared/gatt-db: Check for null pointers

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(-)

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


2020-05-07 22:09:22

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: Re: [Bluez PATCH] shared/gatt-db: Check for null pointers

Hi Abhishek,

On Thu, May 7, 2020 at 2:11 PM Abhishek Pandit-Subedi
<[email protected]> 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
>


--
Luiz Augusto von Dentz