Fix memory leak when adding GATT Characteristic

PATCH In review [email protected] 2 participants 3 messages Aug 12, 2025 Bluetooth drivers

lore

Share & tools

lore

[email protected]

author

2025-08-12 08:00 UTC #0 lore raw

cover Fix memory leak when adding GATT Characteristic

From: Stanislavs Nilovs <[email protected]>

Hello,

As a self-education of Bluetooth world and future usage at work, I have written small GATT Server project. It doesn't have a lot of stuff (like authentication) yet, but basic functionality like read/write was tested with gatttool on PC and some BLE app on my Android phone.

Recently I started to integrate it into propriatary Bluetooth stack at work. When I started testing with BlueZ daemon - it segfaulted each time I was trying to pair over BLE. When pairing over BLE BlueZ is reading GATT information from remote device. After further debugging turns out I wrongly assumed it is allowed by the specification for GATT Characteristic Value handle to be the same as GATT Characteristic Handle. However, this didn't change the fact that BlueZ doesn't handle this case properly and crashes.

Looking at the master code, the 4465c577778d812702d752dfd2812e25a2f69b31 commit has fixed a segfault as it added a check for Characteristic value to not be NULL. However, there are still problems left after this fix:

First of all, the reason for segfault to appear was the overwrite of the original attribute pointer with new one, which didn't had value pointer allocated. With the above commit it may still happen, but we exit early, before segfault appears in memcmp.

Second, the change of above commit is not freeing memory on error as it is done in other error checks above in the "service_insert_characteristic" function.

Bluetoothd address sanitizer backtrace:
=================================================================
==88967==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 768 byte(s) in 6 object(s) allocated from:

    #0 0x768d1f8b3ec7 in __interceptor_malloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:145
    #1 0x59f0cac1ea07 in util_malloc src/shared/util.c:46

Indirect leak of 576 byte(s) in 18 object(s) allocated from:

    #0 0x768d1f8b3ec7 in __interceptor_malloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:145
    #1 0x59f0cac1ea07 in util_malloc src/shared/util.c:46

Indirect leak of 114 byte(s) in 6 object(s) allocated from:

    #0 0x768d1f8b4097 in __interceptor_calloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:154
    #1 0x59f0cac75d6c in new_attribute src/shared/gatt-db.c:222

SUMMARY: AddressSanitizer: 1458 byte(s) leaked in 30 allocation(s).

The patch fixes abscent cleanup as well as adds additional check to exit early if value handle is below or equal to Characteristic handle.

Stanislavs Nilovs (1):
Fix memory leak when adding GATT Characteristic

 src/shared/gatt-db.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

Signature · 1 line

2.48.1

[email protected]

author

2025-08-12 08:00 UTC #1 lore raw

PATCH 1/1 Fix memory leak when adding GATT Characteristic

From: Stanislavs Nilovs <[email protected]>

Commit 4465c577778d812702d752dfd2812e25a2f69b31 has fixed a segfault which may occur during new GATT Characteristic insertion. However, the cleanup exercise isn't done before returning.

Bluetoothd address sanitizer backtrace:
=================================================================
==88967==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 768 byte(s) in 6 object(s) allocated from:

    #0 0x768d1f8b3ec7 in __interceptor_malloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:145
    #1 0x59f0cac1ea07 in util_malloc src/shared/util.c:46

Indirect leak of 576 byte(s) in 18 object(s) allocated from:

    #0 0x768d1f8b3ec7 in __interceptor_malloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:145
    #1 0x59f0cac1ea07 in util_malloc src/shared/util.c:46

Indirect leak of 114 byte(s) in 6 object(s) allocated from:

    #0 0x768d1f8b4097 in __interceptor_calloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:154
    #1 0x59f0cac75d6c in new_attribute src/shared/gatt-db.c:222

SUMMARY: AddressSanitizer: 1458 byte(s) leaked in 30 allocation(s).

Additionally added an explicit check that current Characteristic handle is not greater or equal to its value handle. If value handle is equal to Characteristic handle, then second "new_attribute" allocation will overwrite the pointer in service->attributes, which will also cause a leak.

---
 src/shared/gatt-db.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

src/shared/gatt-db.c +8 -1

index b67bf89f9..02b49e94b 100644
@@ -969,6 +969,10 @@ service_insert_characteristic(struct gatt_db_service *service,
 	if (handle == UINT16_MAX)
 		return NULL;
 
+	/* Value handle can't be the same or less than Characteristic handle per reasons above */
+	if(value_handle <= handle)
+		return NULL;
+
 	i = service_get_attribute_index(service, &handle, 1);
 	if (!i)
 		return NULL;

@@ -1009,8 +1013,11 @@ service_insert_characteristic(struct gatt_db_service *service,
 	/* Update handle of characteristic value_handle if it has changed */
 	put_le16(value_handle, &value[1]);
 
-	if (!(*chrc) || !(*chrc)->value)
+	if (!(*chrc)->value) {
+		free(*chrc);
+		*chrc = NULL;
 		return NULL;
+	}
 
 	if (memcmp((*chrc)->value, value, len))
 		memcpy((*chrc)->value, value, len);

Signature · 1 line

2.48.1

[email protected]

2025-08-12 09:39 UTC #2 lore raw

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=990407

---Test result---

Test Summary:

CheckPatch                    PENDING   0.31 seconds
GitLint                       PENDING   0.23 seconds
BuildEll                      PASS      20.24 seconds
BluezMake                     PASS      2527.25 seconds
MakeCheck                     FAIL      11.69 seconds
MakeDistcheck                 FAIL      161.35 seconds
CheckValgrind                 FAIL      199.82 seconds
CheckSmatch                   PASS      309.44 seconds
bluezmakeextell               PASS      128.07 seconds
IncrementalBuild              PENDING   0.40 seconds
ScanBuild                     PASS      920.54 seconds

Details
##############################
Test: CheckPatch - PENDING
Desc: Run checkpatch.pl script
Output:

##############################
Test: GitLint - PENDING
Desc: Run gitlint
Output:

##############################
Test: MakeCheck - FAIL
Desc: Run Bluez Make Check
Output:

./test-driver: line 107: 30584 Aborted                 (core dumped) "$@" > $log_file 2>&1
./test-driver: line 107: 30613 Aborted                 (core dumped) "$@" > $log_file 2>&1
./test-driver: line 107: 30622 Aborted                 (core dumped) "$@" > $log_file 2>&1
./test-driver: line 107: 30631 Aborted                 (core dumped) "$@" > $log_file 2>&1
./test-driver: line 107: 30640 Aborted                 (core dumped) "$@" > $log_file 2>&1

make[3]: *** [Makefile:9937: test-suite.log] Error 1 make[2]: *** [Makefile:10045: check-TESTS] Error 2 make[1]: *** [Makefile:10467: check-am] Error 2
make: *** [Makefile:10469: check] Error 2
##############################
Test: MakeDistcheck - FAIL
Desc: Run Bluez Make Distcheck
Output:

Package cups was not found in the pkg-config search path. Perhaps you should add the directory containing `cups.pc' to the PKG_CONFIG_PATH environment variable
No package 'cups' found

../../test-driver: line 107: 51044 Aborted                 (core dumped) "$@" > $log_file 2>&1
../../test-driver: line 107: 51071 Aborted                 (core dumped) "$@" > $log_file 2>&1
../../test-driver: line 107: 51080 Aborted                 (core dumped) "$@" > $log_file 2>&1
../../test-driver: line 107: 51274 Aborted                 (core dumped) "$@" > $log_file 2>&1
../../test-driver: line 107: 51342 Aborted                 (core dumped) "$@" > $log_file 2>&1

make[4]: *** [Makefile:9937: test-suite.log] Error 1 make[3]: *** [Makefile:10045: check-TESTS] Error 2 make[2]: *** [Makefile:10467: check-am] Error 2 make[1]: *** [Makefile:10469: check] Error 2
make: *** [Makefile:10390: distcheck] Error 1
##############################
Test: CheckValgrind - FAIL
Desc: Run Bluez Make Check with Valgrind Output:

tools/mgmt-tester.c: In function ‘main’:
tools/mgmt-tester.c:12907:5: note: variable tracking size limit exceeded with ‘-fvar-tracking-assignments’, retrying without 12907 | int main(int argc, char *argv[])

      |     ^~~~
./test-driver: line 107: 69463 Aborted                 (core dumped) "$@" > $log_file 2>&1
./test-driver: line 107: 69499 Aborted                 (core dumped) "$@" > $log_file 2>&1
./test-driver: line 107: 69508 Aborted                 (core dumped) "$@" > $log_file 2>&1
./test-driver: line 107: 69490 Aborted                 (core dumped) "$@" > $log_file 2>&1
./test-driver: line 107: 69517 Aborted                 (core dumped) "$@" > $log_file 2>&1

make[3]: *** [Makefile:9937: test-suite.log] Error 1 make[2]: *** [Makefile:10045: check-TESTS] Error 2 make[1]: *** [Makefile:10467: check-am] Error 2
make: *** [Makefile:10469: check] Error 2
##############################
Test: IncrementalBuild - PENDING
Desc: Incremental build with the patches in the series Output:

---
Regards,
Linux Bluetooth