2024-06-03 18:53:30

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: [PATCH BlueZ v2 1/3] shared/csip: Fix memory leak

From: Luiz Augusto von Dentz <[email protected]>

This fixes the following leak:

102 bytes in 6 blocks are definitely lost in loss record 660 of 909
at 0x484282F: malloc (vg_replace_malloc.c:446)
by 0x5A078B: util_malloc (util.c:46)
by 0x649162: read_sirk (csip.c:485)
by 0x5C74FA: read_cb (gatt-client.c:2713)
by 0x5C4137: handle_rsp (att.c:880)
by 0x5C4137: can_read_data (att.c:1072)
by 0x65DDA4: watch_callback (io-glib.c:157)
by 0x49656AB: ??? (in /usr/lib64/libglib-2.0.so.0.8000.2)
by 0x49C6707: ??? (in /usr/lib64/libglib-2.0.so.0.8000.2)
by 0x496B666: g_main_loop_run (in /usr/lib64/libglib-2.0.so.0.8000.2)
by 0x65FE3D: mainloop_run (mainloop-glib.c:66)
by 0x6605A3: mainloop_run_with_signal (mainloop-notify.c:188)
by 0x31DEFA: main (main.c:1468)
---
src/shared/csip.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/src/shared/csip.c b/src/shared/csip.c
index e13efb6ce634..87b4590d926d 100644
--- a/src/shared/csip.c
+++ b/src/shared/csip.c
@@ -128,6 +128,15 @@ void bt_csip_detach(struct bt_csip *csip)
queue_foreach(csip_cbs, csip_detached, csip);
}

+static void csis_free(struct bt_csis *csis)
+{
+ if (!csis)
+ return;
+
+ free(csis->sirk_val);
+ free(csis);
+}
+
static void csip_db_free(void *data)
{
struct bt_csip_db *cdb = data;
@@ -137,7 +146,7 @@ static void csip_db_free(void *data)

gatt_db_unref(cdb->db);

- free(cdb->csis);
+ csis_free(cdb->csis);
free(cdb);
}

--
2.45.1



2024-06-03 18:53:33

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: [PATCH BlueZ v2 2/3] shared/gatt-db: Introduce gatt_db_clone

From: Luiz Augusto von Dentz <[email protected]>

This introduces gatt_db_clone which can be used to clonse/deep copy and
existing database.
---
src/shared/gatt-db.c | 54 ++++++++++++++++++++++++++++++++++++++++++++
src/shared/gatt-db.h | 1 +
2 files changed, 55 insertions(+)

diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
index 2c8e7d31eda1..d8d21392fee6 100644
--- a/src/shared/gatt-db.c
+++ b/src/shared/gatt-db.c
@@ -260,6 +260,60 @@ struct gatt_db *gatt_db_new(void)
return gatt_db_ref(db);
}

+static void service_clone(void *data, void *user_data)
+{
+ struct gatt_db_service *service = data;
+ struct gatt_db *db = user_data;
+ struct gatt_db_service *clone;
+ int i;
+
+ clone = new0(struct gatt_db_service, 1);
+ clone->db = db;
+ clone->active = service->active;
+ clone->num_handles = service->num_handles;
+ clone->attributes = new0(struct gatt_db_attribute *,
+ service->num_handles);
+
+ /* Clone attributes */
+ for (i = 0; i < service->num_handles; i++) {
+ struct gatt_db_attribute *attr = service->attributes[i];
+
+ /* Only clone values for characteristics since that is
+ * cacheable.
+ */
+ if (bt_uuid_len(&attr->uuid) == 2 &&
+ attr->uuid.value.u16 == GATT_CHARAC_UUID)
+ clone->attributes[i] = new_attribute(clone,
+ attr->handle,
+ &attr->uuid,
+ attr->value,
+ attr->value_len);
+ else
+ clone->attributes[i] = new_attribute(clone,
+ attr->handle,
+ &attr->uuid,
+ NULL, 0);
+ }
+
+ queue_push_tail(db->services, clone);
+}
+
+struct gatt_db *gatt_db_clone(struct gatt_db *db)
+{
+ struct gatt_db *clone;
+
+ if (!db)
+ return NULL;
+
+ clone = gatt_db_new();
+ if (!clone)
+ return NULL;
+
+ queue_foreach(db->services, service_clone, clone);
+
+ return clone;
+}
+
static void notify_destroy(void *data)
{
struct notify *notify = data;
diff --git a/src/shared/gatt-db.h b/src/shared/gatt-db.h
index f7596e33529a..dc2daf7fc1ba 100644
--- a/src/shared/gatt-db.h
+++ b/src/shared/gatt-db.h
@@ -12,6 +12,7 @@ struct gatt_db;
struct gatt_db_attribute;

struct gatt_db *gatt_db_new(void);
+struct gatt_db *gatt_db_clone(struct gatt_db *db);

struct gatt_db *gatt_db_ref(struct gatt_db *db);
void gatt_db_unref(struct gatt_db *db);
--
2.45.1


2024-06-03 18:53:35

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: [PATCH BlueZ v2 3/3] set: Attempt to use existing set gatt-db

From: Luiz Augusto von Dentz <[email protected]>

Most sets should be clone of each other, or at least very similar, so
this attempts to clone the existing gatt-db of the first member found
when connecting new sets, this substantially speed up the process of
bonding sets if their database matches which is something that is
currently ranging from 20-30 seconds depending on the manufacturer and
with this changes it cuts 5-10 seconds by bypassing discovery all
procedure of other members.

If the dbs don't really match bt_gatt_client instance will attempt to
rediscover the ranges that don't match.
---
src/device.c | 21 +++++++++++++++++++++
src/device.h | 1 +
src/set.c | 22 ++++++++++++++++++++--
src/shared/gatt-db.c | 4 ++--
4 files changed, 44 insertions(+), 4 deletions(-)

diff --git a/src/device.c b/src/device.c
index 620bbd55ebad..5dc1cd0cdbf2 100644
--- a/src/device.c
+++ b/src/device.c
@@ -6994,6 +6994,27 @@ struct gatt_db *btd_device_get_gatt_db(struct btd_device *device)
return device->db;
}

+bool btd_device_set_gatt_db(struct btd_device *device, struct gatt_db *db)
+{
+ struct gatt_db *clone;
+
+ if (!device)
+ return false;
+
+ clone = gatt_db_clone(db);
+ if (clone)
+ return false;
+
+ gatt_db_unregister(device->db, device->db_id);
+ gatt_db_unref(device->db);
+
+ device->db = clone;
+ device->db_id = gatt_db_register(device->db, gatt_service_added,
+ gatt_service_removed, device, NULL);
+
+ return true;
+}
+
struct bt_gatt_client *btd_device_get_gatt_client(struct btd_device *device)
{
if (!device)
diff --git a/src/device.h b/src/device.h
index a2b7bb15d200..0794f92d0178 100644
--- a/src/device.h
+++ b/src/device.h
@@ -66,6 +66,7 @@ struct gatt_primary *btd_device_get_primary(struct btd_device *device,
const char *uuid);
GSList *btd_device_get_primaries(struct btd_device *device);
struct gatt_db *btd_device_get_gatt_db(struct btd_device *device);
+bool btd_device_set_gatt_db(struct btd_device *device, struct gatt_db *db);
struct bt_gatt_client *btd_device_get_gatt_client(struct btd_device *device);
struct bt_gatt_server *btd_device_get_gatt_server(struct btd_device *device);
bool btd_device_is_initiator(struct btd_device *device);
diff --git a/src/set.c b/src/set.c
index bf35ee403b39..4ca2f78c3702 100644
--- a/src/set.c
+++ b/src/set.c
@@ -28,6 +28,8 @@
#include "src/shared/queue.h"
#include "src/shared/ad.h"
#include "src/shared/crypto.h"
+#include "src/shared/att.h"
+#include "src/shared/gatt-db.h"

#include "log.h"
#include "error.h"
@@ -277,8 +279,24 @@ static void foreach_rsi(void *data, void *user_data)

bt_crypto_unref(crypto);

- if (!memcmp(ad->data, res, sizeof(res)))
- device_connect_le(set->device);
+ if (memcmp(ad->data, res, sizeof(res)))
+ return;
+
+ /* Attempt to use existing gatt_db from set if device has never been
+ * connected before.
+ *
+ * If dbs don't really match bt_gatt_client will attempt to rediscover
+ * the ranges that don't match.
+ */
+ if (gatt_db_isempty(btd_device_get_gatt_db(set->device))) {
+ struct btd_device *device;
+
+ device = queue_get_entries(set->devices)->data;
+ btd_device_set_gatt_db(set->device,
+ btd_device_get_gatt_db(device));
+ }
+
+ device_connect_le(set->device);
}

static void foreach_device(struct btd_device *device, void *data)
diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
index d8d21392fee6..16abcba2ec1c 100644
--- a/src/shared/gatt-db.c
+++ b/src/shared/gatt-db.c
@@ -278,8 +278,8 @@ static void service_clone(void *data, void *user_data)
for (i = 0; i < service->num_handles; i++) {
struct gatt_db_attribute *attr = service->attributes[i];

- /* Only clone values for characteristics since that is
- * cacheable.
+ /* Only clone values for characteristics declaration since that
+ * is considered when calculating the db hash.
*/
if (bt_uuid_len(&attr->uuid) == 2 &&
attr->uuid.value.u16 == GATT_CHARAC_UUID)
--
2.45.1


2024-06-03 20:30:36

by patchwork-bot+bluetooth

[permalink] [raw]
Subject: Re: [PATCH BlueZ v2 1/3] shared/csip: Fix memory leak

Hello:

This series was applied to bluetooth/bluez.git (master)
by Luiz Augusto von Dentz <[email protected]>:

On Mon, 3 Jun 2024 14:53:10 -0400 you wrote:
> From: Luiz Augusto von Dentz <[email protected]>
>
> This fixes the following leak:
>
> 102 bytes in 6 blocks are definitely lost in loss record 660 of 909
> at 0x484282F: malloc (vg_replace_malloc.c:446)
> by 0x5A078B: util_malloc (util.c:46)
> by 0x649162: read_sirk (csip.c:485)
> by 0x5C74FA: read_cb (gatt-client.c:2713)
> by 0x5C4137: handle_rsp (att.c:880)
> by 0x5C4137: can_read_data (att.c:1072)
> by 0x65DDA4: watch_callback (io-glib.c:157)
> by 0x49656AB: ??? (in /usr/lib64/libglib-2.0.so.0.8000.2)
> by 0x49C6707: ??? (in /usr/lib64/libglib-2.0.so.0.8000.2)
> by 0x496B666: g_main_loop_run (in /usr/lib64/libglib-2.0.so.0.8000.2)
> by 0x65FE3D: mainloop_run (mainloop-glib.c:66)
> by 0x6605A3: mainloop_run_with_signal (mainloop-notify.c:188)
> by 0x31DEFA: main (main.c:1468)
>
> [...]

Here is the summary with links:
- [BlueZ,v2,1/3] shared/csip: Fix memory leak
https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=09e39a7d6dca
- [BlueZ,v2,2/3] shared/gatt-db: Introduce gatt_db_clone
https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=7e9816dd8c21
- [BlueZ,v2,3/3] set: Attempt to use existing set gatt-db
https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=cbe4144dea6f

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html



2024-06-03 21:37:25

by bluez.test.bot

[permalink] [raw]
Subject: RE: [BlueZ,v2,1/3] shared/csip: Fix memory leak

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

---Test result---

Test Summary:
CheckPatch PASS 1.33 seconds
GitLint PASS 0.90 seconds
BuildEll PASS 25.26 seconds
BluezMake PASS 1737.95 seconds
MakeCheck PASS 13.43 seconds
MakeDistcheck PASS 183.16 seconds
CheckValgrind PASS 256.79 seconds
CheckSmatch PASS 361.66 seconds
bluezmakeextell PASS 124.04 seconds
IncrementalBuild PASS 4783.17 seconds
ScanBuild PASS 1055.55 seconds



---
Regards,
Linux Bluetooth