[PATCH BlueZ v1 0/1] Fix memory leak when adding GATT Characteristic

[PATCH BlueZ v1 0/1] Fix memory leak when adding GATT Characteristic

[valorcool]

From: Stanislavs Nilovs <valorcool@gmail.com>

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

-- 
2.48.1

[PATCH BlueZ v1 1/1] Fix memory leak when adding GATT Characteristic

[valorcool]

From: Stanislavs Nilovs <valorcool@gmail.com>

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

diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
index b67bf89f9..02b49e94b 100644
--- a/src/shared/gatt-db.c
+++ b/src/shared/gatt-db.c
@@ -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);
-- 
2.48.1

RE: Fix memory leak when adding GATT Characteristic

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 3989 bytes --]

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