2018-11-17 20:09:18

by Gal Ben Haim

[permalink] [raw]
Subject: [PATCH BlueZ] gatt-client: Fix a segfault that is caused by accessing a destroyed notify_client

Sometimes, when using a pipe that is acquired with AcquireNotify, bluetoothd
exits with segmentation fault when the device disconnects. it happens more
often if "Cache = no" is set in main.conf.

The notify_client is created in characteristic_acquire_notify() and destroyed
in unregister_characteristic(). struct pipe_io contains a pointer to a destroy
function which access the notify_client and also destroy it. this happens
after the notify_client is already destroyed and cause a segmentation fault.

stacktrace:
bluetoothd[1566]: src/gatt-client.c:unregister_characteristic() Removing GATT characteristic: /org/bluez/hci0/dev_EC_25_29_0F_09_AD/service0009/char000c
bluetoothd[1566]: src/gatt-client.c:notify_client_unref() owner :1.8332
bluetoothd[1566]: src/gatt-client.c:notify_client_free() owner :1.8332
bluetoothd[1566]: src/gatt-client.c:unregister_descriptor() Removing GATT descriptor: /org/bluez/hci0/dev_EC_25_29_0F_09_AD/service0009/char000c/desc000e
==1566== Invalid read of size 4
==1566== at 0x63920: notify_io_destroy (gatt-client.c:1461)
==1566== by 0x63EDB: pipe_io_destroy (gatt-client.c:1082)
==1566== by 0x6405B: characteristic_free (gatt-client.c:1663)
==1566== by 0x81F33: remove_interface (object.c:667)
==1566== by 0x826CB: g_dbus_unregister_interface (object.c:1391)
==1566== by 0x85D2B: queue_remove_all (queue.c:354)
==1566== by 0x635F7: unregister_service (gatt-client.c:1893)
==1566== by 0x85CF7: queue_remove_all (queue.c:339)
==1566== by 0x661DF: btd_gatt_client_service_removed (gatt-client.c:2199)
==1566== by 0x695CB: gatt_service_removed (device.c:3747)
==1566== by 0x85B17: queue_foreach (queue.c:220)
==1566== by 0x91283: notify_service_changed (gatt-db.c:280)
==1566== by 0x91283: gatt_db_service_destroy (gatt-db.c:291)
==1566== Address 0x515ed48 is 0 bytes inside a block of size 20 free'd
==1566== at 0x483EAD0: free (vg_replace_malloc.c:530)
==1566== by 0x85D2B: queue_remove_all (queue.c:354)
==1566== by 0x636D3: unregister_characteristic (gatt-client.c:1741)
==1566== by 0x85D2B: queue_remove_all (queue.c:354)
==1566== by 0x635F7: unregister_service (gatt-client.c:1893)
==1566== by 0x85CF7: queue_remove_all (queue.c:339)
==1566== by 0x661DF: btd_gatt_client_service_removed (gatt-client.c:2199)
==1566== by 0x695CB: gatt_service_removed (device.c:3747)
==1566== by 0x85B17: queue_foreach (queue.c:220)
==1566== by 0x91283: notify_service_changed (gatt-db.c:280)
==1566== by 0x91283: gatt_db_service_destroy (gatt-db.c:291)
==1566== by 0x85D2B: queue_remove_all (queue.c:354)
==1566== by 0x91387: gatt_db_clear_range (gatt-db.c:475)
==1566== Block was alloc'd at
==1566== at 0x483D588: malloc (vg_replace_malloc.c:299)
==1566== by 0x85DBB: btd_malloc (util.c:46)
==1566== by 0x645C3: notify_client_create (gatt-client.c:1330)
==1566== by 0x6493B: characteristic_acquire_notify (gatt-client.c:1486)
==1566== by 0x82E33: process_message (object.c:259)
==1566== by 0x496A59F: ??? (in /usr/lib/libdbus-1.so.3.19.8)
==1566==
==1566== Invalid read of size 4
==1566== at 0x85B94: queue_remove (queue.c:256)
==1566== by 0x63937: notify_io_destroy (gatt-client.c:1461)
==1566== by 0x63EDB: pipe_io_destroy (gatt-client.c:1082)
==1566== by 0x6405B: characteristic_free (gatt-client.c:1663)
==1566== by 0x81F33: remove_interface (object.c:667)
==1566== by 0x826CB: g_dbus_unregister_interface (object.c:1391)
==1566== by 0x85D2B: queue_remove_all (queue.c:354)
==1566== by 0x635F7: unregister_service (gatt-client.c:1893)
==1566== by 0x85CF7: queue_remove_all (queue.c:339)
==1566== by 0x661DF: btd_gatt_client_service_removed (gatt-client.c:2199)
==1566== by 0x695CB: gatt_service_removed (device.c:3747)
==1566== by 0x85B17: queue_foreach (queue.c:220)
==1566== Address 0x512391c is 4 bytes inside a block of size 16 free'd
==1566== at 0x483EAD0: free (vg_replace_malloc.c:530)
==1566== by 0x6400B: characteristic_free (gatt-client.c:1654)
==1566== by 0x81F33: remove_interface (object.c:667)
==1566== by 0x826CB: g_dbus_unregister_interface (object.c:1391)
==1566== by 0x85D2B: queue_remove_all (queue.c:354)
==1566== by 0x635F7: unregister_service (gatt-client.c:1893)
==1566== by 0x85CF7: queue_remove_all (queue.c:339)
==1566== by 0x661DF: btd_gatt_client_service_removed (gatt-client.c:2199)
==1566== by 0x695CB: gatt_service_removed (device.c:3747)
==1566== by 0x85B17: queue_foreach (queue.c:220)
==1566== by 0x91283: notify_service_changed (gatt-db.c:280)
==1566== by 0x91283: gatt_db_service_destroy (gatt-db.c:291)
==1566== by 0x85D2B: queue_remove_all (queue.c:354)
==1566== Block was alloc'd at
==1566== at 0x483D588: malloc (vg_replace_malloc.c:299)
==1566== by 0x85DBB: btd_malloc (util.c:46)
==1566== by 0x858F3: queue_new (queue.c:60)
==1566== by 0x65577: characteristic_create (gatt-client.c:1679)
==1566== by 0x65577: export_char (gatt-client.c:1947)
==1566== by 0x91A07: gatt_db_service_foreach (gatt-db.c:1338)
==1566== by 0x65A6F: create_characteristics (gatt-client.c:1972)
==1566== by 0x65A6F: export_service (gatt-client.c:1989)
==1566== by 0x920BF: foreach_service_in_range (gatt-db.c:1292)
==1566== by 0x85B17: queue_foreach (queue.c:220)
==1566== by 0x9197F: gatt_db_foreach_service_in_range (gatt-db.c:1313)
==1566== by 0x9199F: gatt_db_foreach_service (gatt-db.c:1262)
==1566== by 0x6604F: create_services (gatt-client.c:2060)
==1566== by 0x6604F: btd_gatt_client_ready (gatt-client.c:2150)
==1566== by 0x6C63B: gatt_client_ready_cb (device.c:4843)
==1566==
---
src/gatt-client.c | 13 -------------
1 file changed, 13 deletions(-)

diff --git a/src/gatt-client.c b/src/gatt-client.c
index 234f46ed7..04fa5cf26 100644
--- a/src/gatt-client.c
+++ b/src/gatt-client.c
@@ -93,7 +93,6 @@ struct async_dbus_op {
struct pipe_io {
DBusMessage *msg;
struct io *io;
- void (*destroy)(void *data);
void *data;
};

@@ -1078,9 +1077,6 @@ static bool chrc_pipe_read(struct io *io, void *user_data)

static void pipe_io_destroy(struct pipe_io *io)
{
- if (io->destroy)
- io->destroy(io->data);
-
if (io->msg)
dbus_message_unref(io->msg);

@@ -1454,14 +1450,6 @@ static void register_notify_io_cb(uint16_t att_ecode, void *user_data)
characteristic_ready(true, 0, chrc);
}

-static void notify_io_destroy(void *data)
-{
- struct notify_client *client = data;
-
- if (queue_remove(client->chrc->notify_clients, client))
- notify_client_unref(client);
-}
-
static DBusMessage *characteristic_acquire_notify(DBusConnection *conn,
DBusMessage *msg, void *user_data)
{
@@ -1502,7 +1490,6 @@ static DBusMessage *characteristic_acquire_notify(DBusConnection *conn,
chrc->notify_io = new0(struct pipe_io, 1);
chrc->notify_io->data = client;
chrc->notify_io->msg = dbus_message_ref(msg);
- chrc->notify_io->destroy = notify_io_destroy;

return NULL;
}
--
2.19.1