* [PATCH BlueZ 1/3] shared/att: Fix crash when calling disconnect handlers @ 2017-10-23 11:17 Luiz Augusto von Dentz 2017-10-23 11:17 ` [PATCH BlueZ 2/3] gatt: Clear subscriptions for device not paired Luiz Augusto von Dentz 2017-10-23 11:17 ` [PATCH BlueZ 3/3] test/example-gatt-server: Don't change measuments if not notifying Luiz Augusto von Dentz 0 siblings, 2 replies; 12+ messages in thread From: Luiz Augusto von Dentz @ 2017-10-23 11:17 UTC (permalink / raw) To: linux-bluetooth From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> When calling disconnect handlers the callback itself may remove items from the queue causing the following crash: Invalid read of size 8 at 0x4D1D3C: queue_foreach (queue.c:219) by 0x4D8369: disconnect_cb (att.c:590) by 0x4E4FAA: watch_callback (io-glib.c:170) by 0x50CD246: g_main_context_dispatch (in /usr/lib64/libglib-2.0.so.0.5200.3) by 0x50CD5E7: ??? (in /usr/lib64/libglib-2.0.so.0.5200.3) by 0x50CD901: g_main_loop_run (in /usr/lib64/libglib-2.0.so.0.5200.3) by 0x40CCC0: main (main.c:770) Address 0x888a888 is 8 bytes inside a block of size 16 free'd at 0x4C2FD18: free (vg_replace_malloc.c:530) by 0x4D1F9B: queue_remove_if (queue.c:302) by 0x4D763B: bt_att_unregister_disconnect (att.c:1206) by 0x4DC11E: bt_gatt_client_free (gatt-client.c:1762) by 0x4DC270: bt_gatt_client_unref (gatt-client.c:1903) by 0x4A316F: gatt_client_cleanup (device.c:573) by 0x4A326E: attio_cleanup (device.c:598) by 0x4A5EB9: att_disconnected_cb (device.c:4679) by 0x4D66D5: disconn_handler (att.c:538) by 0x4D1D4F: queue_foreach (queue.c:220) by 0x4D8369: disconnect_cb (att.c:590) by 0x4E4FAA: watch_callback (io-glib.c:170) --- src/shared/att.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/shared/att.c b/src/shared/att.c index 4670de74f..8d58156c1 100644 --- a/src/shared/att.c +++ b/src/shared/att.c @@ -1203,6 +1203,17 @@ bool bt_att_unregister_disconnect(struct bt_att *att, unsigned int id) if (!att || !id) return false; + /* Check if disconnect is running */ + if (!att->io) { + disconn = queue_find(att->disconn_list, match_disconn_id, + UINT_TO_PTR(id)); + if (!disconn) + return false; + + disconn->removed = true; + return true; + } + disconn = queue_remove_if(att->disconn_list, match_disconn_id, UINT_TO_PTR(id)); if (!disconn) -- 2.13.6 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH BlueZ 2/3] gatt: Clear subscriptions for device not paired 2017-10-23 11:17 [PATCH BlueZ 1/3] shared/att: Fix crash when calling disconnect handlers Luiz Augusto von Dentz @ 2017-10-23 11:17 ` Luiz Augusto von Dentz 2017-10-23 20:26 ` Yunhan Wang 2017-10-23 11:17 ` [PATCH BlueZ 3/3] test/example-gatt-server: Don't change measuments if not notifying Luiz Augusto von Dentz 1 sibling, 1 reply; 12+ messages in thread From: Luiz Augusto von Dentz @ 2017-10-23 11:17 UTC (permalink / raw) To: linux-bluetooth From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> If the device is no longer valid or is not considered bonded anymore clear its CCC states before removing otherwise application may continue to notify when there are no devices listening. --- src/gatt-database.c | 155 ++++++++++++++++++++++++++++++++++------------------ 1 file changed, 103 insertions(+), 52 deletions(-) diff --git a/src/gatt-database.c b/src/gatt-database.c index 47304704a..6784998c3 100644 --- a/src/gatt-database.c +++ b/src/gatt-database.c @@ -150,8 +150,10 @@ struct pending_op { }; struct device_state { + struct btd_gatt_database *db; bdaddr_t bdaddr; uint8_t bdaddr_type; + unsigned int disc_id; struct queue *ccc_states; }; @@ -245,12 +247,14 @@ static struct ccc_state *find_ccc_state(struct device_state *dev_state, UINT_TO_PTR(handle)); } -static struct device_state *device_state_create(bdaddr_t *bdaddr, +static struct device_state *device_state_create(struct btd_gatt_database *db, + bdaddr_t *bdaddr, uint8_t bdaddr_type) { struct device_state *dev_state; dev_state = new0(struct device_state, 1); + dev_state->db = db; dev_state->ccc_states = queue_new(); bacpy(&dev_state->bdaddr, bdaddr); dev_state->bdaddr_type = bdaddr_type; @@ -258,36 +262,117 @@ static struct device_state *device_state_create(bdaddr_t *bdaddr, return dev_state; } +static void device_state_free(void *data) +{ + struct device_state *state = data; + + queue_destroy(state->ccc_states, free); + free(state); +} + +static void clear_ccc_state(void *data, void *user_data) +{ + struct ccc_state *ccc = data; + struct btd_gatt_database *db = user_data; + struct ccc_cb_data *ccc_cb; + + if (!ccc->value[0]) + return; + + ccc_cb = queue_find(db->ccc_callbacks, ccc_cb_match_handle, + UINT_TO_PTR(ccc->handle)); + if (!ccc_cb) + return; + + ccc_cb->callback(NULL, 0, ccc_cb->user_data); +} + +static void att_disconnected(int err, void *user_data) +{ + struct device_state *state = user_data; + struct btd_device *device; + + DBG(""); + + state->disc_id = 0; + + device = btd_adapter_get_device(state->db->adapter, &state->bdaddr, + state->bdaddr_type); + if (!device) + goto remove; + + if (device_is_paired(device, state->bdaddr_type)) + return; + +remove: + /* Remove device state if device no longer exists or is not paired */ + if (queue_remove(state->db->device_states, state)) { + queue_foreach(state->ccc_states, clear_ccc_state, state->db); + device_state_free(state); + } +} + +static bool get_dst_info(struct bt_att *att, bdaddr_t *dst, uint8_t *dst_type) +{ + GIOChannel *io = NULL; + GError *gerr = NULL; + + io = g_io_channel_unix_new(bt_att_get_fd(att)); + if (!io) + return false; + + bt_io_get(io, &gerr, BT_IO_OPT_DEST_BDADDR, dst, + BT_IO_OPT_DEST_TYPE, dst_type, + BT_IO_OPT_INVALID); + if (gerr) { + error("gatt: bt_io_get: %s", gerr->message); + g_error_free(gerr); + g_io_channel_unref(io); + return false; + } + + g_io_channel_unref(io); + return true; +} + static struct device_state *get_device_state(struct btd_gatt_database *database, - bdaddr_t *bdaddr, - uint8_t bdaddr_type) + struct bt_att *att) { struct device_state *dev_state; + bdaddr_t bdaddr; + uint8_t bdaddr_type; + + if (!get_dst_info(att, &bdaddr, &bdaddr_type)) + return NULL; /* * Find and return a device state. If a matching state doesn't exist, * then create a new one. */ - dev_state = find_device_state(database, bdaddr, bdaddr_type); + dev_state = find_device_state(database, &bdaddr, bdaddr_type); if (dev_state) - return dev_state; + goto done; - dev_state = device_state_create(bdaddr, bdaddr_type); + dev_state = device_state_create(database, &bdaddr, bdaddr_type); queue_push_tail(database->device_states, dev_state); +done: + dev_state->disc_id = bt_att_register_disconnect(att, att_disconnected, + dev_state, NULL); + return dev_state; } static struct ccc_state *get_ccc_state(struct btd_gatt_database *database, - bdaddr_t *bdaddr, - uint8_t bdaddr_type, - uint16_t handle) + struct bt_att *att, uint16_t handle) { struct device_state *dev_state; struct ccc_state *ccc; - dev_state = get_device_state(database, bdaddr, bdaddr_type); + dev_state = get_device_state(database, att); + if (!dev_state) + return NULL; ccc = find_ccc_state(dev_state, handle); if (ccc) @@ -300,14 +385,6 @@ static struct ccc_state *get_ccc_state(struct btd_gatt_database *database, return ccc; } -static void device_state_free(void *data) -{ - struct device_state *state = data; - - queue_destroy(state->ccc_states, free); - free(state); -} - static void cancel_pending_read(void *data) { struct pending_op *op = data; @@ -690,29 +767,6 @@ static void populate_gap_service(struct btd_gatt_database *database) gatt_db_service_set_active(service, true); } -static bool get_dst_info(struct bt_att *att, bdaddr_t *dst, uint8_t *dst_type) -{ - GIOChannel *io = NULL; - GError *gerr = NULL; - - io = g_io_channel_unix_new(bt_att_get_fd(att)); - if (!io) - return false; - - bt_io_get(io, &gerr, BT_IO_OPT_DEST_BDADDR, dst, - BT_IO_OPT_DEST_TYPE, dst_type, - BT_IO_OPT_INVALID); - if (gerr) { - error("gatt: bt_io_get: %s", gerr->message); - g_error_free(gerr); - g_io_channel_unref(io); - return false; - } - - g_io_channel_unref(io); - return true; -} - static void gatt_ccc_read_cb(struct gatt_db_attribute *attrib, unsigned int id, uint16_t offset, uint8_t opcode, struct bt_att *att, @@ -724,8 +778,6 @@ static void gatt_ccc_read_cb(struct gatt_db_attribute *attrib, uint8_t ecode = 0; const uint8_t *value = NULL; size_t len = 0; - bdaddr_t bdaddr; - uint8_t bdaddr_type; handle = gatt_db_attribute_get_handle(attrib); @@ -736,13 +788,12 @@ static void gatt_ccc_read_cb(struct gatt_db_attribute *attrib, goto done; } - if (!get_dst_info(att, &bdaddr, &bdaddr_type)) { + ccc = get_ccc_state(database, att, handle); + if (!ccc) { ecode = BT_ATT_ERROR_UNLIKELY; goto done; } - ccc = get_ccc_state(database, &bdaddr, bdaddr_type, handle); - len = 2 - offset; value = len ? &ccc->value[offset] : NULL; @@ -761,8 +812,6 @@ static void gatt_ccc_write_cb(struct gatt_db_attribute *attrib, struct ccc_cb_data *ccc_cb; uint16_t handle; uint8_t ecode = 0; - bdaddr_t bdaddr; - uint8_t bdaddr_type; handle = gatt_db_attribute_get_handle(attrib); @@ -778,13 +827,12 @@ static void gatt_ccc_write_cb(struct gatt_db_attribute *attrib, goto done; } - if (!get_dst_info(att, &bdaddr, &bdaddr_type)) { + ccc = get_ccc_state(database, att, handle); + if (!ccc) { ecode = BT_ATT_ERROR_UNLIKELY; goto done; } - ccc = get_ccc_state(database, &bdaddr, bdaddr_type, handle); - ccc_cb = queue_find(database->ccc_callbacks, ccc_cb_match_handle, UINT_TO_PTR(gatt_db_attribute_get_handle(attrib))); if (!ccc_cb) { @@ -940,8 +988,11 @@ static void send_notification_to_device(void *data, void *user_data) remove: /* Remove device state if device no longer exists or is not paired */ - if (queue_remove(notify->database->device_states, device_state)) + if (queue_remove(notify->database->device_states, device_state)) { + queue_foreach(device_state->ccc_states, clear_ccc_state, + notify->database); device_state_free(device_state); + } } static void send_notification_to_devices(struct btd_gatt_database *database, -- 2.13.6 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH BlueZ 2/3] gatt: Clear subscriptions for device not paired 2017-10-23 11:17 ` [PATCH BlueZ 2/3] gatt: Clear subscriptions for device not paired Luiz Augusto von Dentz @ 2017-10-23 20:26 ` Yunhan Wang 2017-10-23 20:29 ` Luiz Augusto von Dentz 0 siblings, 1 reply; 12+ messages in thread From: Yunhan Wang @ 2017-10-23 20:26 UTC (permalink / raw) To: Luiz Augusto von Dentz; +Cc: linux-bluetooth Hi Luiz WIth latest bluez master, I am seeing segmentation fault in bluetoothd when ble disconnection happens. I think it related to this patch. The backtract is as below, #0 0x0000000000000000 in ?? () #1 0x0000000000481b4d in queue_foreach (queue=0x6fe4e0, function=function@entry=0x442d40 <clear_ccc_state>, user_data=0x6ee630) at src/shared/queue.c:220 #2 0x00000000004436c9 in att_disconnected (err=<optimized out>, user_data=0x6fe4b0) at src/gatt-database.c:310 #3 0x0000000000481b4d in queue_foreach (queue=0x705b70, function=function@entry=0x485280 <disconn_handler>, user_data=0x68) at src/shared/queue.c:220 #4 0x0000000000486632 in disconnect_cb (io=<optimized out>, user_data=0x70b140) at src/shared/att.c:590 #5 0x000000000048fbc5 in watch_callback (channel=<optimized out>, cond=<optimized out>, user_data=<optimized out>) at src/shared/io-glib.c:170 #6 0x00007ffff7b1ace5 in g_main_context_dispatch () from /lib/x86_64-linux-gnu/libglib-2.0.so.0 #7 0x00007ffff7b1b048 in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0 #8 0x00007ffff7b1b30a in g_main_loop_run () from /lib/x86_64-linux-gnu/libglib-2.0.so.0 #9 0x000000000040b51b in main (argc=1, argv=0x7fffffffe618) at src/main.c:770 thanks best wishes yunhan On Mon, Oct 23, 2017 at 4:17 AM, Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > > If the device is no longer valid or is not considered bonded anymore > clear its CCC states before removing otherwise application may continue > to notify when there are no devices listening. > --- > src/gatt-database.c | 155 ++++++++++++++++++++++++++++++++++------------------ > 1 file changed, 103 insertions(+), 52 deletions(-) > > diff --git a/src/gatt-database.c b/src/gatt-database.c > index 47304704a..6784998c3 100644 > --- a/src/gatt-database.c > +++ b/src/gatt-database.c > @@ -150,8 +150,10 @@ struct pending_op { > }; > > struct device_state { > + struct btd_gatt_database *db; > bdaddr_t bdaddr; > uint8_t bdaddr_type; > + unsigned int disc_id; > struct queue *ccc_states; > }; > > @@ -245,12 +247,14 @@ static struct ccc_state *find_ccc_state(struct device_state *dev_state, > UINT_TO_PTR(handle)); > } > > -static struct device_state *device_state_create(bdaddr_t *bdaddr, > +static struct device_state *device_state_create(struct btd_gatt_database *db, > + bdaddr_t *bdaddr, > uint8_t bdaddr_type) > { > struct device_state *dev_state; > > dev_state = new0(struct device_state, 1); > + dev_state->db = db; > dev_state->ccc_states = queue_new(); > bacpy(&dev_state->bdaddr, bdaddr); > dev_state->bdaddr_type = bdaddr_type; > @@ -258,36 +262,117 @@ static struct device_state *device_state_create(bdaddr_t *bdaddr, > return dev_state; > } > > +static void device_state_free(void *data) > +{ > + struct device_state *state = data; > + > + queue_destroy(state->ccc_states, free); > + free(state); > +} > + > +static void clear_ccc_state(void *data, void *user_data) > +{ > + struct ccc_state *ccc = data; > + struct btd_gatt_database *db = user_data; > + struct ccc_cb_data *ccc_cb; > + > + if (!ccc->value[0]) > + return; > + > + ccc_cb = queue_find(db->ccc_callbacks, ccc_cb_match_handle, > + UINT_TO_PTR(ccc->handle)); > + if (!ccc_cb) > + return; > + > + ccc_cb->callback(NULL, 0, ccc_cb->user_data); > +} > + > +static void att_disconnected(int err, void *user_data) > +{ > + struct device_state *state = user_data; > + struct btd_device *device; > + > + DBG(""); > + > + state->disc_id = 0; > + > + device = btd_adapter_get_device(state->db->adapter, &state->bdaddr, > + state->bdaddr_type); > + if (!device) > + goto remove; > + > + if (device_is_paired(device, state->bdaddr_type)) > + return; > + > +remove: > + /* Remove device state if device no longer exists or is not paired */ > + if (queue_remove(state->db->device_states, state)) { > + queue_foreach(state->ccc_states, clear_ccc_state, state->db); > + device_state_free(state); > + } > +} > + > +static bool get_dst_info(struct bt_att *att, bdaddr_t *dst, uint8_t *dst_type) > +{ > + GIOChannel *io = NULL; > + GError *gerr = NULL; > + > + io = g_io_channel_unix_new(bt_att_get_fd(att)); > + if (!io) > + return false; > + > + bt_io_get(io, &gerr, BT_IO_OPT_DEST_BDADDR, dst, > + BT_IO_OPT_DEST_TYPE, dst_type, > + BT_IO_OPT_INVALID); > + if (gerr) { > + error("gatt: bt_io_get: %s", gerr->message); > + g_error_free(gerr); > + g_io_channel_unref(io); > + return false; > + } > + > + g_io_channel_unref(io); > + return true; > +} > + > static struct device_state *get_device_state(struct btd_gatt_database *database, > - bdaddr_t *bdaddr, > - uint8_t bdaddr_type) > + struct bt_att *att) > { > struct device_state *dev_state; > + bdaddr_t bdaddr; > + uint8_t bdaddr_type; > + > + if (!get_dst_info(att, &bdaddr, &bdaddr_type)) > + return NULL; > > /* > * Find and return a device state. If a matching state doesn't exist, > * then create a new one. > */ > - dev_state = find_device_state(database, bdaddr, bdaddr_type); > + dev_state = find_device_state(database, &bdaddr, bdaddr_type); > if (dev_state) > - return dev_state; > + goto done; > > - dev_state = device_state_create(bdaddr, bdaddr_type); > + dev_state = device_state_create(database, &bdaddr, bdaddr_type); > > queue_push_tail(database->device_states, dev_state); > > +done: > + dev_state->disc_id = bt_att_register_disconnect(att, att_disconnected, > + dev_state, NULL); > + > return dev_state; > } > > static struct ccc_state *get_ccc_state(struct btd_gatt_database *database, > - bdaddr_t *bdaddr, > - uint8_t bdaddr_type, > - uint16_t handle) > + struct bt_att *att, uint16_t handle) > { > struct device_state *dev_state; > struct ccc_state *ccc; > > - dev_state = get_device_state(database, bdaddr, bdaddr_type); > + dev_state = get_device_state(database, att); > + if (!dev_state) > + return NULL; > > ccc = find_ccc_state(dev_state, handle); > if (ccc) > @@ -300,14 +385,6 @@ static struct ccc_state *get_ccc_state(struct btd_gatt_database *database, > return ccc; > } > > -static void device_state_free(void *data) > -{ > - struct device_state *state = data; > - > - queue_destroy(state->ccc_states, free); > - free(state); > -} > - > static void cancel_pending_read(void *data) > { > struct pending_op *op = data; > @@ -690,29 +767,6 @@ static void populate_gap_service(struct btd_gatt_database *database) > gatt_db_service_set_active(service, true); > } > > -static bool get_dst_info(struct bt_att *att, bdaddr_t *dst, uint8_t *dst_type) > -{ > - GIOChannel *io = NULL; > - GError *gerr = NULL; > - > - io = g_io_channel_unix_new(bt_att_get_fd(att)); > - if (!io) > - return false; > - > - bt_io_get(io, &gerr, BT_IO_OPT_DEST_BDADDR, dst, > - BT_IO_OPT_DEST_TYPE, dst_type, > - BT_IO_OPT_INVALID); > - if (gerr) { > - error("gatt: bt_io_get: %s", gerr->message); > - g_error_free(gerr); > - g_io_channel_unref(io); > - return false; > - } > - > - g_io_channel_unref(io); > - return true; > -} > - > static void gatt_ccc_read_cb(struct gatt_db_attribute *attrib, > unsigned int id, uint16_t offset, > uint8_t opcode, struct bt_att *att, > @@ -724,8 +778,6 @@ static void gatt_ccc_read_cb(struct gatt_db_attribute *attrib, > uint8_t ecode = 0; > const uint8_t *value = NULL; > size_t len = 0; > - bdaddr_t bdaddr; > - uint8_t bdaddr_type; > > handle = gatt_db_attribute_get_handle(attrib); > > @@ -736,13 +788,12 @@ static void gatt_ccc_read_cb(struct gatt_db_attribute *attrib, > goto done; > } > > - if (!get_dst_info(att, &bdaddr, &bdaddr_type)) { > + ccc = get_ccc_state(database, att, handle); > + if (!ccc) { > ecode = BT_ATT_ERROR_UNLIKELY; > goto done; > } > > - ccc = get_ccc_state(database, &bdaddr, bdaddr_type, handle); > - > len = 2 - offset; > value = len ? &ccc->value[offset] : NULL; > > @@ -761,8 +812,6 @@ static void gatt_ccc_write_cb(struct gatt_db_attribute *attrib, > struct ccc_cb_data *ccc_cb; > uint16_t handle; > uint8_t ecode = 0; > - bdaddr_t bdaddr; > - uint8_t bdaddr_type; > > handle = gatt_db_attribute_get_handle(attrib); > > @@ -778,13 +827,12 @@ static void gatt_ccc_write_cb(struct gatt_db_attribute *attrib, > goto done; > } > > - if (!get_dst_info(att, &bdaddr, &bdaddr_type)) { > + ccc = get_ccc_state(database, att, handle); > + if (!ccc) { > ecode = BT_ATT_ERROR_UNLIKELY; > goto done; > } > > - ccc = get_ccc_state(database, &bdaddr, bdaddr_type, handle); > - > ccc_cb = queue_find(database->ccc_callbacks, ccc_cb_match_handle, > UINT_TO_PTR(gatt_db_attribute_get_handle(attrib))); > if (!ccc_cb) { > @@ -940,8 +988,11 @@ static void send_notification_to_device(void *data, void *user_data) > > remove: > /* Remove device state if device no longer exists or is not paired */ > - if (queue_remove(notify->database->device_states, device_state)) > + if (queue_remove(notify->database->device_states, device_state)) { > + queue_foreach(device_state->ccc_states, clear_ccc_state, > + notify->database); > device_state_free(device_state); > + } > } > > static void send_notification_to_devices(struct btd_gatt_database *database, > -- > 2.13.6 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH BlueZ 2/3] gatt: Clear subscriptions for device not paired 2017-10-23 20:26 ` Yunhan Wang @ 2017-10-23 20:29 ` Luiz Augusto von Dentz 2017-10-23 20:59 ` Yunhan Wang 0 siblings, 1 reply; 12+ messages in thread From: Luiz Augusto von Dentz @ 2017-10-23 20:29 UTC (permalink / raw) To: Yunhan Wang; +Cc: linux-bluetooth@vger.kernel.org Hi Yunhan, On Mon, Oct 23, 2017 at 11:26 PM, Yunhan Wang <yunhanw@nestlabs.com> wrote: > Hi Luiz > > WIth latest bluez master, I am seeing segmentation fault in bluetoothd > when ble disconnection happens. I think it related to this patch. > > The backtract is as below, > #0 0x0000000000000000 in ?? () > #1 0x0000000000481b4d in queue_foreach (queue=0x6fe4e0, > function=function@entry=0x442d40 <clear_ccc_state>, > user_data=0x6ee630) at src/shared/queue.c:220 > #2 0x00000000004436c9 in att_disconnected (err=<optimized out>, > user_data=0x6fe4b0) at src/gatt-database.c:310 > #3 0x0000000000481b4d in queue_foreach (queue=0x705b70, > function=function@entry=0x485280 <disconn_handler>, user_data=0x68) at > src/shared/queue.c:220 > #4 0x0000000000486632 in disconnect_cb (io=<optimized out>, > user_data=0x70b140) at src/shared/att.c:590 > #5 0x000000000048fbc5 in watch_callback (channel=<optimized out>, > cond=<optimized out>, user_data=<optimized out>) at > src/shared/io-glib.c:170 > #6 0x00007ffff7b1ace5 in g_main_context_dispatch () from > /lib/x86_64-linux-gnu/libglib-2.0.so.0 > #7 0x00007ffff7b1b048 in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0 > #8 0x00007ffff7b1b30a in g_main_loop_run () from > /lib/x86_64-linux-gnu/libglib-2.0.so.0 > #9 0x000000000040b51b in main (argc=1, argv=0x7fffffffe618) at src/main.c:770 There is a patch for this issue: https://git.kernel.org/pub/scm/bluetooth/bluez.git/commit/?id=261cf78db4be79a0f7d44798a57730b159c9be91 > thanks > best wishes > yunhan > > On Mon, Oct 23, 2017 at 4:17 AM, Luiz Augusto von Dentz > <luiz.dentz@gmail.com> wrote: >> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> >> >> If the device is no longer valid or is not considered bonded anymore >> clear its CCC states before removing otherwise application may continue >> to notify when there are no devices listening. >> --- >> src/gatt-database.c | 155 ++++++++++++++++++++++++++++++++++------------------ >> 1 file changed, 103 insertions(+), 52 deletions(-) >> >> diff --git a/src/gatt-database.c b/src/gatt-database.c >> index 47304704a..6784998c3 100644 >> --- a/src/gatt-database.c >> +++ b/src/gatt-database.c >> @@ -150,8 +150,10 @@ struct pending_op { >> }; >> >> struct device_state { >> + struct btd_gatt_database *db; >> bdaddr_t bdaddr; >> uint8_t bdaddr_type; >> + unsigned int disc_id; >> struct queue *ccc_states; >> }; >> >> @@ -245,12 +247,14 @@ static struct ccc_state *find_ccc_state(struct device_state *dev_state, >> UINT_TO_PTR(handle)); >> } >> >> -static struct device_state *device_state_create(bdaddr_t *bdaddr, >> +static struct device_state *device_state_create(struct btd_gatt_database *db, >> + bdaddr_t *bdaddr, >> uint8_t bdaddr_type) >> { >> struct device_state *dev_state; >> >> dev_state = new0(struct device_state, 1); >> + dev_state->db = db; >> dev_state->ccc_states = queue_new(); >> bacpy(&dev_state->bdaddr, bdaddr); >> dev_state->bdaddr_type = bdaddr_type; >> @@ -258,36 +262,117 @@ static struct device_state *device_state_create(bdaddr_t *bdaddr, >> return dev_state; >> } >> >> +static void device_state_free(void *data) >> +{ >> + struct device_state *state = data; >> + >> + queue_destroy(state->ccc_states, free); >> + free(state); >> +} >> + >> +static void clear_ccc_state(void *data, void *user_data) >> +{ >> + struct ccc_state *ccc = data; >> + struct btd_gatt_database *db = user_data; >> + struct ccc_cb_data *ccc_cb; >> + >> + if (!ccc->value[0]) >> + return; >> + >> + ccc_cb = queue_find(db->ccc_callbacks, ccc_cb_match_handle, >> + UINT_TO_PTR(ccc->handle)); >> + if (!ccc_cb) >> + return; >> + >> + ccc_cb->callback(NULL, 0, ccc_cb->user_data); >> +} >> + >> +static void att_disconnected(int err, void *user_data) >> +{ >> + struct device_state *state = user_data; >> + struct btd_device *device; >> + >> + DBG(""); >> + >> + state->disc_id = 0; >> + >> + device = btd_adapter_get_device(state->db->adapter, &state->bdaddr, >> + state->bdaddr_type); >> + if (!device) >> + goto remove; >> + >> + if (device_is_paired(device, state->bdaddr_type)) >> + return; >> + >> +remove: >> + /* Remove device state if device no longer exists or is not paired */ >> + if (queue_remove(state->db->device_states, state)) { >> + queue_foreach(state->ccc_states, clear_ccc_state, state->db); >> + device_state_free(state); >> + } >> +} >> + >> +static bool get_dst_info(struct bt_att *att, bdaddr_t *dst, uint8_t *dst_type) >> +{ >> + GIOChannel *io = NULL; >> + GError *gerr = NULL; >> + >> + io = g_io_channel_unix_new(bt_att_get_fd(att)); >> + if (!io) >> + return false; >> + >> + bt_io_get(io, &gerr, BT_IO_OPT_DEST_BDADDR, dst, >> + BT_IO_OPT_DEST_TYPE, dst_type, >> + BT_IO_OPT_INVALID); >> + if (gerr) { >> + error("gatt: bt_io_get: %s", gerr->message); >> + g_error_free(gerr); >> + g_io_channel_unref(io); >> + return false; >> + } >> + >> + g_io_channel_unref(io); >> + return true; >> +} >> + >> static struct device_state *get_device_state(struct btd_gatt_database *database, >> - bdaddr_t *bdaddr, >> - uint8_t bdaddr_type) >> + struct bt_att *att) >> { >> struct device_state *dev_state; >> + bdaddr_t bdaddr; >> + uint8_t bdaddr_type; >> + >> + if (!get_dst_info(att, &bdaddr, &bdaddr_type)) >> + return NULL; >> >> /* >> * Find and return a device state. If a matching state doesn't exist, >> * then create a new one. >> */ >> - dev_state = find_device_state(database, bdaddr, bdaddr_type); >> + dev_state = find_device_state(database, &bdaddr, bdaddr_type); >> if (dev_state) >> - return dev_state; >> + goto done; >> >> - dev_state = device_state_create(bdaddr, bdaddr_type); >> + dev_state = device_state_create(database, &bdaddr, bdaddr_type); >> >> queue_push_tail(database->device_states, dev_state); >> >> +done: >> + dev_state->disc_id = bt_att_register_disconnect(att, att_disconnected, >> + dev_state, NULL); >> + >> return dev_state; >> } >> >> static struct ccc_state *get_ccc_state(struct btd_gatt_database *database, >> - bdaddr_t *bdaddr, >> - uint8_t bdaddr_type, >> - uint16_t handle) >> + struct bt_att *att, uint16_t handle) >> { >> struct device_state *dev_state; >> struct ccc_state *ccc; >> >> - dev_state = get_device_state(database, bdaddr, bdaddr_type); >> + dev_state = get_device_state(database, att); >> + if (!dev_state) >> + return NULL; >> >> ccc = find_ccc_state(dev_state, handle); >> if (ccc) >> @@ -300,14 +385,6 @@ static struct ccc_state *get_ccc_state(struct btd_gatt_database *database, >> return ccc; >> } >> >> -static void device_state_free(void *data) >> -{ >> - struct device_state *state = data; >> - >> - queue_destroy(state->ccc_states, free); >> - free(state); >> -} >> - >> static void cancel_pending_read(void *data) >> { >> struct pending_op *op = data; >> @@ -690,29 +767,6 @@ static void populate_gap_service(struct btd_gatt_database *database) >> gatt_db_service_set_active(service, true); >> } >> >> -static bool get_dst_info(struct bt_att *att, bdaddr_t *dst, uint8_t *dst_type) >> -{ >> - GIOChannel *io = NULL; >> - GError *gerr = NULL; >> - >> - io = g_io_channel_unix_new(bt_att_get_fd(att)); >> - if (!io) >> - return false; >> - >> - bt_io_get(io, &gerr, BT_IO_OPT_DEST_BDADDR, dst, >> - BT_IO_OPT_DEST_TYPE, dst_type, >> - BT_IO_OPT_INVALID); >> - if (gerr) { >> - error("gatt: bt_io_get: %s", gerr->message); >> - g_error_free(gerr); >> - g_io_channel_unref(io); >> - return false; >> - } >> - >> - g_io_channel_unref(io); >> - return true; >> -} >> - >> static void gatt_ccc_read_cb(struct gatt_db_attribute *attrib, >> unsigned int id, uint16_t offset, >> uint8_t opcode, struct bt_att *att, >> @@ -724,8 +778,6 @@ static void gatt_ccc_read_cb(struct gatt_db_attribute *attrib, >> uint8_t ecode = 0; >> const uint8_t *value = NULL; >> size_t len = 0; >> - bdaddr_t bdaddr; >> - uint8_t bdaddr_type; >> >> handle = gatt_db_attribute_get_handle(attrib); >> >> @@ -736,13 +788,12 @@ static void gatt_ccc_read_cb(struct gatt_db_attribute *attrib, >> goto done; >> } >> >> - if (!get_dst_info(att, &bdaddr, &bdaddr_type)) { >> + ccc = get_ccc_state(database, att, handle); >> + if (!ccc) { >> ecode = BT_ATT_ERROR_UNLIKELY; >> goto done; >> } >> >> - ccc = get_ccc_state(database, &bdaddr, bdaddr_type, handle); >> - >> len = 2 - offset; >> value = len ? &ccc->value[offset] : NULL; >> >> @@ -761,8 +812,6 @@ static void gatt_ccc_write_cb(struct gatt_db_attribute *attrib, >> struct ccc_cb_data *ccc_cb; >> uint16_t handle; >> uint8_t ecode = 0; >> - bdaddr_t bdaddr; >> - uint8_t bdaddr_type; >> >> handle = gatt_db_attribute_get_handle(attrib); >> >> @@ -778,13 +827,12 @@ static void gatt_ccc_write_cb(struct gatt_db_attribute *attrib, >> goto done; >> } >> >> - if (!get_dst_info(att, &bdaddr, &bdaddr_type)) { >> + ccc = get_ccc_state(database, att, handle); >> + if (!ccc) { >> ecode = BT_ATT_ERROR_UNLIKELY; >> goto done; >> } >> >> - ccc = get_ccc_state(database, &bdaddr, bdaddr_type, handle); >> - >> ccc_cb = queue_find(database->ccc_callbacks, ccc_cb_match_handle, >> UINT_TO_PTR(gatt_db_attribute_get_handle(attrib))); >> if (!ccc_cb) { >> @@ -940,8 +988,11 @@ static void send_notification_to_device(void *data, void *user_data) >> >> remove: >> /* Remove device state if device no longer exists or is not paired */ >> - if (queue_remove(notify->database->device_states, device_state)) >> + if (queue_remove(notify->database->device_states, device_state)) { >> + queue_foreach(device_state->ccc_states, clear_ccc_state, >> + notify->database); >> device_state_free(device_state); >> + } >> } >> >> static void send_notification_to_devices(struct btd_gatt_database *database, >> -- >> 2.13.6 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html -- Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH BlueZ 2/3] gatt: Clear subscriptions for device not paired 2017-10-23 20:29 ` Luiz Augusto von Dentz @ 2017-10-23 20:59 ` Yunhan Wang 2017-10-24 8:10 ` Luiz Augusto von Dentz 0 siblings, 1 reply; 12+ messages in thread From: Yunhan Wang @ 2017-10-23 20:59 UTC (permalink / raw) To: Luiz Augusto von Dentz; +Cc: linux-bluetooth@vger.kernel.org Hi, Luiz I have tried this, I think the issue is still there with your patch. thanks best wishes yunhan On Mon, Oct 23, 2017 at 1:29 PM, Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > Hi Yunhan, > > On Mon, Oct 23, 2017 at 11:26 PM, Yunhan Wang <yunhanw@nestlabs.com> wrote: >> Hi Luiz >> >> WIth latest bluez master, I am seeing segmentation fault in bluetoothd >> when ble disconnection happens. I think it related to this patch. >> >> The backtract is as below, >> #0 0x0000000000000000 in ?? () >> #1 0x0000000000481b4d in queue_foreach (queue=0x6fe4e0, >> function=function@entry=0x442d40 <clear_ccc_state>, >> user_data=0x6ee630) at src/shared/queue.c:220 >> #2 0x00000000004436c9 in att_disconnected (err=<optimized out>, >> user_data=0x6fe4b0) at src/gatt-database.c:310 >> #3 0x0000000000481b4d in queue_foreach (queue=0x705b70, >> function=function@entry=0x485280 <disconn_handler>, user_data=0x68) at >> src/shared/queue.c:220 >> #4 0x0000000000486632 in disconnect_cb (io=<optimized out>, >> user_data=0x70b140) at src/shared/att.c:590 >> #5 0x000000000048fbc5 in watch_callback (channel=<optimized out>, >> cond=<optimized out>, user_data=<optimized out>) at >> src/shared/io-glib.c:170 >> #6 0x00007ffff7b1ace5 in g_main_context_dispatch () from >> /lib/x86_64-linux-gnu/libglib-2.0.so.0 >> #7 0x00007ffff7b1b048 in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0 >> #8 0x00007ffff7b1b30a in g_main_loop_run () from >> /lib/x86_64-linux-gnu/libglib-2.0.so.0 >> #9 0x000000000040b51b in main (argc=1, argv=0x7fffffffe618) at src/main.c:770 > > There is a patch for this issue: > > https://git.kernel.org/pub/scm/bluetooth/bluez.git/commit/?id=261cf78db4be79a0f7d44798a57730b159c9be91 > >> thanks >> best wishes >> yunhan >> >> On Mon, Oct 23, 2017 at 4:17 AM, Luiz Augusto von Dentz >> <luiz.dentz@gmail.com> wrote: >>> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> >>> >>> If the device is no longer valid or is not considered bonded anymore >>> clear its CCC states before removing otherwise application may continue >>> to notify when there are no devices listening. >>> --- >>> src/gatt-database.c | 155 ++++++++++++++++++++++++++++++++++------------------ >>> 1 file changed, 103 insertions(+), 52 deletions(-) >>> >>> diff --git a/src/gatt-database.c b/src/gatt-database.c >>> index 47304704a..6784998c3 100644 >>> --- a/src/gatt-database.c >>> +++ b/src/gatt-database.c >>> @@ -150,8 +150,10 @@ struct pending_op { >>> }; >>> >>> struct device_state { >>> + struct btd_gatt_database *db; >>> bdaddr_t bdaddr; >>> uint8_t bdaddr_type; >>> + unsigned int disc_id; >>> struct queue *ccc_states; >>> }; >>> >>> @@ -245,12 +247,14 @@ static struct ccc_state *find_ccc_state(struct device_state *dev_state, >>> UINT_TO_PTR(handle)); >>> } >>> >>> -static struct device_state *device_state_create(bdaddr_t *bdaddr, >>> +static struct device_state *device_state_create(struct btd_gatt_database *db, >>> + bdaddr_t *bdaddr, >>> uint8_t bdaddr_type) >>> { >>> struct device_state *dev_state; >>> >>> dev_state = new0(struct device_state, 1); >>> + dev_state->db = db; >>> dev_state->ccc_states = queue_new(); >>> bacpy(&dev_state->bdaddr, bdaddr); >>> dev_state->bdaddr_type = bdaddr_type; >>> @@ -258,36 +262,117 @@ static struct device_state *device_state_create(bdaddr_t *bdaddr, >>> return dev_state; >>> } >>> >>> +static void device_state_free(void *data) >>> +{ >>> + struct device_state *state = data; >>> + >>> + queue_destroy(state->ccc_states, free); >>> + free(state); >>> +} >>> + >>> +static void clear_ccc_state(void *data, void *user_data) >>> +{ >>> + struct ccc_state *ccc = data; >>> + struct btd_gatt_database *db = user_data; >>> + struct ccc_cb_data *ccc_cb; >>> + >>> + if (!ccc->value[0]) >>> + return; >>> + >>> + ccc_cb = queue_find(db->ccc_callbacks, ccc_cb_match_handle, >>> + UINT_TO_PTR(ccc->handle)); >>> + if (!ccc_cb) >>> + return; >>> + >>> + ccc_cb->callback(NULL, 0, ccc_cb->user_data); >>> +} >>> + >>> +static void att_disconnected(int err, void *user_data) >>> +{ >>> + struct device_state *state = user_data; >>> + struct btd_device *device; >>> + >>> + DBG(""); >>> + >>> + state->disc_id = 0; >>> + >>> + device = btd_adapter_get_device(state->db->adapter, &state->bdaddr, >>> + state->bdaddr_type); >>> + if (!device) >>> + goto remove; >>> + >>> + if (device_is_paired(device, state->bdaddr_type)) >>> + return; >>> + >>> +remove: >>> + /* Remove device state if device no longer exists or is not paired */ >>> + if (queue_remove(state->db->device_states, state)) { >>> + queue_foreach(state->ccc_states, clear_ccc_state, state->db); >>> + device_state_free(state); >>> + } >>> +} >>> + >>> +static bool get_dst_info(struct bt_att *att, bdaddr_t *dst, uint8_t *dst_type) >>> +{ >>> + GIOChannel *io = NULL; >>> + GError *gerr = NULL; >>> + >>> + io = g_io_channel_unix_new(bt_att_get_fd(att)); >>> + if (!io) >>> + return false; >>> + >>> + bt_io_get(io, &gerr, BT_IO_OPT_DEST_BDADDR, dst, >>> + BT_IO_OPT_DEST_TYPE, dst_type, >>> + BT_IO_OPT_INVALID); >>> + if (gerr) { >>> + error("gatt: bt_io_get: %s", gerr->message); >>> + g_error_free(gerr); >>> + g_io_channel_unref(io); >>> + return false; >>> + } >>> + >>> + g_io_channel_unref(io); >>> + return true; >>> +} >>> + >>> static struct device_state *get_device_state(struct btd_gatt_database *database, >>> - bdaddr_t *bdaddr, >>> - uint8_t bdaddr_type) >>> + struct bt_att *att) >>> { >>> struct device_state *dev_state; >>> + bdaddr_t bdaddr; >>> + uint8_t bdaddr_type; >>> + >>> + if (!get_dst_info(att, &bdaddr, &bdaddr_type)) >>> + return NULL; >>> >>> /* >>> * Find and return a device state. If a matching state doesn't exist, >>> * then create a new one. >>> */ >>> - dev_state = find_device_state(database, bdaddr, bdaddr_type); >>> + dev_state = find_device_state(database, &bdaddr, bdaddr_type); >>> if (dev_state) >>> - return dev_state; >>> + goto done; >>> >>> - dev_state = device_state_create(bdaddr, bdaddr_type); >>> + dev_state = device_state_create(database, &bdaddr, bdaddr_type); >>> >>> queue_push_tail(database->device_states, dev_state); >>> >>> +done: >>> + dev_state->disc_id = bt_att_register_disconnect(att, att_disconnected, >>> + dev_state, NULL); >>> + >>> return dev_state; >>> } >>> >>> static struct ccc_state *get_ccc_state(struct btd_gatt_database *database, >>> - bdaddr_t *bdaddr, >>> - uint8_t bdaddr_type, >>> - uint16_t handle) >>> + struct bt_att *att, uint16_t handle) >>> { >>> struct device_state *dev_state; >>> struct ccc_state *ccc; >>> >>> - dev_state = get_device_state(database, bdaddr, bdaddr_type); >>> + dev_state = get_device_state(database, att); >>> + if (!dev_state) >>> + return NULL; >>> >>> ccc = find_ccc_state(dev_state, handle); >>> if (ccc) >>> @@ -300,14 +385,6 @@ static struct ccc_state *get_ccc_state(struct btd_gatt_database *database, >>> return ccc; >>> } >>> >>> -static void device_state_free(void *data) >>> -{ >>> - struct device_state *state = data; >>> - >>> - queue_destroy(state->ccc_states, free); >>> - free(state); >>> -} >>> - >>> static void cancel_pending_read(void *data) >>> { >>> struct pending_op *op = data; >>> @@ -690,29 +767,6 @@ static void populate_gap_service(struct btd_gatt_database *database) >>> gatt_db_service_set_active(service, true); >>> } >>> >>> -static bool get_dst_info(struct bt_att *att, bdaddr_t *dst, uint8_t *dst_type) >>> -{ >>> - GIOChannel *io = NULL; >>> - GError *gerr = NULL; >>> - >>> - io = g_io_channel_unix_new(bt_att_get_fd(att)); >>> - if (!io) >>> - return false; >>> - >>> - bt_io_get(io, &gerr, BT_IO_OPT_DEST_BDADDR, dst, >>> - BT_IO_OPT_DEST_TYPE, dst_type, >>> - BT_IO_OPT_INVALID); >>> - if (gerr) { >>> - error("gatt: bt_io_get: %s", gerr->message); >>> - g_error_free(gerr); >>> - g_io_channel_unref(io); >>> - return false; >>> - } >>> - >>> - g_io_channel_unref(io); >>> - return true; >>> -} >>> - >>> static void gatt_ccc_read_cb(struct gatt_db_attribute *attrib, >>> unsigned int id, uint16_t offset, >>> uint8_t opcode, struct bt_att *att, >>> @@ -724,8 +778,6 @@ static void gatt_ccc_read_cb(struct gatt_db_attribute *attrib, >>> uint8_t ecode = 0; >>> const uint8_t *value = NULL; >>> size_t len = 0; >>> - bdaddr_t bdaddr; >>> - uint8_t bdaddr_type; >>> >>> handle = gatt_db_attribute_get_handle(attrib); >>> >>> @@ -736,13 +788,12 @@ static void gatt_ccc_read_cb(struct gatt_db_attribute *attrib, >>> goto done; >>> } >>> >>> - if (!get_dst_info(att, &bdaddr, &bdaddr_type)) { >>> + ccc = get_ccc_state(database, att, handle); >>> + if (!ccc) { >>> ecode = BT_ATT_ERROR_UNLIKELY; >>> goto done; >>> } >>> >>> - ccc = get_ccc_state(database, &bdaddr, bdaddr_type, handle); >>> - >>> len = 2 - offset; >>> value = len ? &ccc->value[offset] : NULL; >>> >>> @@ -761,8 +812,6 @@ static void gatt_ccc_write_cb(struct gatt_db_attribute *attrib, >>> struct ccc_cb_data *ccc_cb; >>> uint16_t handle; >>> uint8_t ecode = 0; >>> - bdaddr_t bdaddr; >>> - uint8_t bdaddr_type; >>> >>> handle = gatt_db_attribute_get_handle(attrib); >>> >>> @@ -778,13 +827,12 @@ static void gatt_ccc_write_cb(struct gatt_db_attribute *attrib, >>> goto done; >>> } >>> >>> - if (!get_dst_info(att, &bdaddr, &bdaddr_type)) { >>> + ccc = get_ccc_state(database, att, handle); >>> + if (!ccc) { >>> ecode = BT_ATT_ERROR_UNLIKELY; >>> goto done; >>> } >>> >>> - ccc = get_ccc_state(database, &bdaddr, bdaddr_type, handle); >>> - >>> ccc_cb = queue_find(database->ccc_callbacks, ccc_cb_match_handle, >>> UINT_TO_PTR(gatt_db_attribute_get_handle(attrib))); >>> if (!ccc_cb) { >>> @@ -940,8 +988,11 @@ static void send_notification_to_device(void *data, void *user_data) >>> >>> remove: >>> /* Remove device state if device no longer exists or is not paired */ >>> - if (queue_remove(notify->database->device_states, device_state)) >>> + if (queue_remove(notify->database->device_states, device_state)) { >>> + queue_foreach(device_state->ccc_states, clear_ccc_state, >>> + notify->database); >>> device_state_free(device_state); >>> + } >>> } >>> >>> static void send_notification_to_devices(struct btd_gatt_database *database, >>> -- >>> 2.13.6 >>> >>> -- >>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in >>> the body of a message to majordomo@vger.kernel.org >>> More majordomo info at http://vger.kernel.org/majordomo-info.html > > > > -- > Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH BlueZ 2/3] gatt: Clear subscriptions for device not paired 2017-10-23 20:59 ` Yunhan Wang @ 2017-10-24 8:10 ` Luiz Augusto von Dentz 2017-10-25 7:58 ` Yunhan Wang 0 siblings, 1 reply; 12+ messages in thread From: Luiz Augusto von Dentz @ 2017-10-24 8:10 UTC (permalink / raw) To: Yunhan Wang; +Cc: linux-bluetooth@vger.kernel.org Hi Yunhan, On Mon, Oct 23, 2017 at 11:59 PM, Yunhan Wang <yunhanw@nestlabs.com> wrote: > Hi, Luiz > > I have tried this, I think the issue is still there with your patch. I don't see any problem: bluetoothd[13968]: src/device.c:att_disconnected_cb() Software caused connection abort (103) bluetoothd[13968]: src/gatt-client.c:btd_gatt_client_disconnected() Device disconnected. Cleaning up. bluetoothd[13968]: src/device.c:att_disconnected_cb() Automatic connection disabled bluetoothd[13968]: attrib/gattrib.c:g_attrib_unref() 0x8897600: g_attrib_unref=0 bluetoothd[13968]: src/gatt-database.c:att_disconnected() bluetoothd[13968]: src/gatt-database.c:ccc_write_cb() External CCC write received with value: 0x0000 Could you try running under valgrind? > thanks > best wishes > yunhan > > On Mon, Oct 23, 2017 at 1:29 PM, Luiz Augusto von Dentz > <luiz.dentz@gmail.com> wrote: >> Hi Yunhan, >> >> On Mon, Oct 23, 2017 at 11:26 PM, Yunhan Wang <yunhanw@nestlabs.com> wrote: >>> Hi Luiz >>> >>> WIth latest bluez master, I am seeing segmentation fault in bluetoothd >>> when ble disconnection happens. I think it related to this patch. >>> >>> The backtract is as below, >>> #0 0x0000000000000000 in ?? () >>> #1 0x0000000000481b4d in queue_foreach (queue=0x6fe4e0, >>> function=function@entry=0x442d40 <clear_ccc_state>, >>> user_data=0x6ee630) at src/shared/queue.c:220 >>> #2 0x00000000004436c9 in att_disconnected (err=<optimized out>, >>> user_data=0x6fe4b0) at src/gatt-database.c:310 >>> #3 0x0000000000481b4d in queue_foreach (queue=0x705b70, >>> function=function@entry=0x485280 <disconn_handler>, user_data=0x68) at >>> src/shared/queue.c:220 >>> #4 0x0000000000486632 in disconnect_cb (io=<optimized out>, >>> user_data=0x70b140) at src/shared/att.c:590 >>> #5 0x000000000048fbc5 in watch_callback (channel=<optimized out>, >>> cond=<optimized out>, user_data=<optimized out>) at >>> src/shared/io-glib.c:170 >>> #6 0x00007ffff7b1ace5 in g_main_context_dispatch () from >>> /lib/x86_64-linux-gnu/libglib-2.0.so.0 >>> #7 0x00007ffff7b1b048 in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0 >>> #8 0x00007ffff7b1b30a in g_main_loop_run () from >>> /lib/x86_64-linux-gnu/libglib-2.0.so.0 >>> #9 0x000000000040b51b in main (argc=1, argv=0x7fffffffe618) at src/main.c:770 >> >> There is a patch for this issue: >> >> https://git.kernel.org/pub/scm/bluetooth/bluez.git/commit/?id=261cf78db4be79a0f7d44798a57730b159c9be91 >> >>> thanks >>> best wishes >>> yunhan >>> >>> On Mon, Oct 23, 2017 at 4:17 AM, Luiz Augusto von Dentz >>> <luiz.dentz@gmail.com> wrote: >>>> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> >>>> >>>> If the device is no longer valid or is not considered bonded anymore >>>> clear its CCC states before removing otherwise application may continue >>>> to notify when there are no devices listening. >>>> --- >>>> src/gatt-database.c | 155 ++++++++++++++++++++++++++++++++++------------------ >>>> 1 file changed, 103 insertions(+), 52 deletions(-) >>>> >>>> diff --git a/src/gatt-database.c b/src/gatt-database.c >>>> index 47304704a..6784998c3 100644 >>>> --- a/src/gatt-database.c >>>> +++ b/src/gatt-database.c >>>> @@ -150,8 +150,10 @@ struct pending_op { >>>> }; >>>> >>>> struct device_state { >>>> + struct btd_gatt_database *db; >>>> bdaddr_t bdaddr; >>>> uint8_t bdaddr_type; >>>> + unsigned int disc_id; >>>> struct queue *ccc_states; >>>> }; >>>> >>>> @@ -245,12 +247,14 @@ static struct ccc_state *find_ccc_state(struct device_state *dev_state, >>>> UINT_TO_PTR(handle)); >>>> } >>>> >>>> -static struct device_state *device_state_create(bdaddr_t *bdaddr, >>>> +static struct device_state *device_state_create(struct btd_gatt_database *db, >>>> + bdaddr_t *bdaddr, >>>> uint8_t bdaddr_type) >>>> { >>>> struct device_state *dev_state; >>>> >>>> dev_state = new0(struct device_state, 1); >>>> + dev_state->db = db; >>>> dev_state->ccc_states = queue_new(); >>>> bacpy(&dev_state->bdaddr, bdaddr); >>>> dev_state->bdaddr_type = bdaddr_type; >>>> @@ -258,36 +262,117 @@ static struct device_state *device_state_create(bdaddr_t *bdaddr, >>>> return dev_state; >>>> } >>>> >>>> +static void device_state_free(void *data) >>>> +{ >>>> + struct device_state *state = data; >>>> + >>>> + queue_destroy(state->ccc_states, free); >>>> + free(state); >>>> +} >>>> + >>>> +static void clear_ccc_state(void *data, void *user_data) >>>> +{ >>>> + struct ccc_state *ccc = data; >>>> + struct btd_gatt_database *db = user_data; >>>> + struct ccc_cb_data *ccc_cb; >>>> + >>>> + if (!ccc->value[0]) >>>> + return; >>>> + >>>> + ccc_cb = queue_find(db->ccc_callbacks, ccc_cb_match_handle, >>>> + UINT_TO_PTR(ccc->handle)); >>>> + if (!ccc_cb) >>>> + return; >>>> + >>>> + ccc_cb->callback(NULL, 0, ccc_cb->user_data); >>>> +} >>>> + >>>> +static void att_disconnected(int err, void *user_data) >>>> +{ >>>> + struct device_state *state = user_data; >>>> + struct btd_device *device; >>>> + >>>> + DBG(""); >>>> + >>>> + state->disc_id = 0; >>>> + >>>> + device = btd_adapter_get_device(state->db->adapter, &state->bdaddr, >>>> + state->bdaddr_type); >>>> + if (!device) >>>> + goto remove; >>>> + >>>> + if (device_is_paired(device, state->bdaddr_type)) >>>> + return; >>>> + >>>> +remove: >>>> + /* Remove device state if device no longer exists or is not paired */ >>>> + if (queue_remove(state->db->device_states, state)) { >>>> + queue_foreach(state->ccc_states, clear_ccc_state, state->db); >>>> + device_state_free(state); >>>> + } >>>> +} >>>> + >>>> +static bool get_dst_info(struct bt_att *att, bdaddr_t *dst, uint8_t *dst_type) >>>> +{ >>>> + GIOChannel *io = NULL; >>>> + GError *gerr = NULL; >>>> + >>>> + io = g_io_channel_unix_new(bt_att_get_fd(att)); >>>> + if (!io) >>>> + return false; >>>> + >>>> + bt_io_get(io, &gerr, BT_IO_OPT_DEST_BDADDR, dst, >>>> + BT_IO_OPT_DEST_TYPE, dst_type, >>>> + BT_IO_OPT_INVALID); >>>> + if (gerr) { >>>> + error("gatt: bt_io_get: %s", gerr->message); >>>> + g_error_free(gerr); >>>> + g_io_channel_unref(io); >>>> + return false; >>>> + } >>>> + >>>> + g_io_channel_unref(io); >>>> + return true; >>>> +} >>>> + >>>> static struct device_state *get_device_state(struct btd_gatt_database *database, >>>> - bdaddr_t *bdaddr, >>>> - uint8_t bdaddr_type) >>>> + struct bt_att *att) >>>> { >>>> struct device_state *dev_state; >>>> + bdaddr_t bdaddr; >>>> + uint8_t bdaddr_type; >>>> + >>>> + if (!get_dst_info(att, &bdaddr, &bdaddr_type)) >>>> + return NULL; >>>> >>>> /* >>>> * Find and return a device state. If a matching state doesn't exist, >>>> * then create a new one. >>>> */ >>>> - dev_state = find_device_state(database, bdaddr, bdaddr_type); >>>> + dev_state = find_device_state(database, &bdaddr, bdaddr_type); >>>> if (dev_state) >>>> - return dev_state; >>>> + goto done; >>>> >>>> - dev_state = device_state_create(bdaddr, bdaddr_type); >>>> + dev_state = device_state_create(database, &bdaddr, bdaddr_type); >>>> >>>> queue_push_tail(database->device_states, dev_state); >>>> >>>> +done: >>>> + dev_state->disc_id = bt_att_register_disconnect(att, att_disconnected, >>>> + dev_state, NULL); >>>> + >>>> return dev_state; >>>> } >>>> >>>> static struct ccc_state *get_ccc_state(struct btd_gatt_database *database, >>>> - bdaddr_t *bdaddr, >>>> - uint8_t bdaddr_type, >>>> - uint16_t handle) >>>> + struct bt_att *att, uint16_t handle) >>>> { >>>> struct device_state *dev_state; >>>> struct ccc_state *ccc; >>>> >>>> - dev_state = get_device_state(database, bdaddr, bdaddr_type); >>>> + dev_state = get_device_state(database, att); >>>> + if (!dev_state) >>>> + return NULL; >>>> >>>> ccc = find_ccc_state(dev_state, handle); >>>> if (ccc) >>>> @@ -300,14 +385,6 @@ static struct ccc_state *get_ccc_state(struct btd_gatt_database *database, >>>> return ccc; >>>> } >>>> >>>> -static void device_state_free(void *data) >>>> -{ >>>> - struct device_state *state = data; >>>> - >>>> - queue_destroy(state->ccc_states, free); >>>> - free(state); >>>> -} >>>> - >>>> static void cancel_pending_read(void *data) >>>> { >>>> struct pending_op *op = data; >>>> @@ -690,29 +767,6 @@ static void populate_gap_service(struct btd_gatt_database *database) >>>> gatt_db_service_set_active(service, true); >>>> } >>>> >>>> -static bool get_dst_info(struct bt_att *att, bdaddr_t *dst, uint8_t *dst_type) >>>> -{ >>>> - GIOChannel *io = NULL; >>>> - GError *gerr = NULL; >>>> - >>>> - io = g_io_channel_unix_new(bt_att_get_fd(att)); >>>> - if (!io) >>>> - return false; >>>> - >>>> - bt_io_get(io, &gerr, BT_IO_OPT_DEST_BDADDR, dst, >>>> - BT_IO_OPT_DEST_TYPE, dst_type, >>>> - BT_IO_OPT_INVALID); >>>> - if (gerr) { >>>> - error("gatt: bt_io_get: %s", gerr->message); >>>> - g_error_free(gerr); >>>> - g_io_channel_unref(io); >>>> - return false; >>>> - } >>>> - >>>> - g_io_channel_unref(io); >>>> - return true; >>>> -} >>>> - >>>> static void gatt_ccc_read_cb(struct gatt_db_attribute *attrib, >>>> unsigned int id, uint16_t offset, >>>> uint8_t opcode, struct bt_att *att, >>>> @@ -724,8 +778,6 @@ static void gatt_ccc_read_cb(struct gatt_db_attribute *attrib, >>>> uint8_t ecode = 0; >>>> const uint8_t *value = NULL; >>>> size_t len = 0; >>>> - bdaddr_t bdaddr; >>>> - uint8_t bdaddr_type; >>>> >>>> handle = gatt_db_attribute_get_handle(attrib); >>>> >>>> @@ -736,13 +788,12 @@ static void gatt_ccc_read_cb(struct gatt_db_attribute *attrib, >>>> goto done; >>>> } >>>> >>>> - if (!get_dst_info(att, &bdaddr, &bdaddr_type)) { >>>> + ccc = get_ccc_state(database, att, handle); >>>> + if (!ccc) { >>>> ecode = BT_ATT_ERROR_UNLIKELY; >>>> goto done; >>>> } >>>> >>>> - ccc = get_ccc_state(database, &bdaddr, bdaddr_type, handle); >>>> - >>>> len = 2 - offset; >>>> value = len ? &ccc->value[offset] : NULL; >>>> >>>> @@ -761,8 +812,6 @@ static void gatt_ccc_write_cb(struct gatt_db_attribute *attrib, >>>> struct ccc_cb_data *ccc_cb; >>>> uint16_t handle; >>>> uint8_t ecode = 0; >>>> - bdaddr_t bdaddr; >>>> - uint8_t bdaddr_type; >>>> >>>> handle = gatt_db_attribute_get_handle(attrib); >>>> >>>> @@ -778,13 +827,12 @@ static void gatt_ccc_write_cb(struct gatt_db_attribute *attrib, >>>> goto done; >>>> } >>>> >>>> - if (!get_dst_info(att, &bdaddr, &bdaddr_type)) { >>>> + ccc = get_ccc_state(database, att, handle); >>>> + if (!ccc) { >>>> ecode = BT_ATT_ERROR_UNLIKELY; >>>> goto done; >>>> } >>>> >>>> - ccc = get_ccc_state(database, &bdaddr, bdaddr_type, handle); >>>> - >>>> ccc_cb = queue_find(database->ccc_callbacks, ccc_cb_match_handle, >>>> UINT_TO_PTR(gatt_db_attribute_get_handle(attrib))); >>>> if (!ccc_cb) { >>>> @@ -940,8 +988,11 @@ static void send_notification_to_device(void *data, void *user_data) >>>> >>>> remove: >>>> /* Remove device state if device no longer exists or is not paired */ >>>> - if (queue_remove(notify->database->device_states, device_state)) >>>> + if (queue_remove(notify->database->device_states, device_state)) { >>>> + queue_foreach(device_state->ccc_states, clear_ccc_state, >>>> + notify->database); >>>> device_state_free(device_state); >>>> + } >>>> } >>>> >>>> static void send_notification_to_devices(struct btd_gatt_database *database, >>>> -- >>>> 2.13.6 >>>> >>>> -- >>>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in >>>> the body of a message to majordomo@vger.kernel.org >>>> More majordomo info at http://vger.kernel.org/majordomo-info.html >> >> >> >> -- >> Luiz Augusto von Dentz -- Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH BlueZ 2/3] gatt: Clear subscriptions for device not paired 2017-10-24 8:10 ` Luiz Augusto von Dentz @ 2017-10-25 7:58 ` Yunhan Wang 2017-10-25 13:27 ` Luiz Augusto von Dentz 0 siblings, 1 reply; 12+ messages in thread From: Yunhan Wang @ 2017-10-25 7:58 UTC (permalink / raw) To: Luiz Augusto von Dentz; +Cc: linux-bluetooth@vger.kernel.org Hi, Luiz On Tue, Oct 24, 2017 at 1:10 AM, Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > Hi Yunhan, > > On Mon, Oct 23, 2017 at 11:59 PM, Yunhan Wang <yunhanw@nestlabs.com> wrote: >> Hi, Luiz >> >> I have tried this, I think the issue is still there with your patch. > > I don't see any problem: > > bluetoothd[13968]: src/device.c:att_disconnected_cb() Software caused > connection abort (103) > bluetoothd[13968]: src/gatt-client.c:btd_gatt_client_disconnected() > Device disconnected. Cleaning up. > bluetoothd[13968]: src/device.c:att_disconnected_cb() Automatic > connection disabled > bluetoothd[13968]: attrib/gattrib.c:g_attrib_unref() 0x8897600: g_attrib_unref=0 > bluetoothd[13968]: src/gatt-database.c:att_disconnected() > bluetoothd[13968]: src/gatt-database.c:ccc_write_cb() External CCC > write received with value: 0x0000 > > Could you try running under valgrind? > Yes, just tried under valgrind, the issue is constantly reprodcued. Btw, I am currently using btvirt -L -l2, one client, one server, then bluetoothd crash after client issue ble disconnect. I am using bluez lateset master. sudo valgrind ./src/bluetoothd --experimental --debug ==31609== Memcheck, a memory error detector ==31609== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al. ==31609== Using Valgrind-3.10.1 and LibVEX; rerun with -h for copyright info ==31609== Command: ./src/bluetoothd --experimental --debug ==31609== ==31609== Syscall param socketcall.bind(my_addr.rc_bdaddr) points to uninitialised byte(s) ==31609== at 0x588EDA7: bind (syscall-template.S:81) ==31609== by 0x43A805: logging_open (log.c:76) ==31609== by 0x43A805: __btd_log_init (log.c:314) ==31609== by 0x40ADCD: main (main.c:688) ==31609== Address 0xfff0003d6 is on thread 1's stack ==31609== in frame #1, created by __btd_log_init (log.c:306) ==31609== ==31609== Syscall param socketcall.bind(my_addr.rc_channel) points to uninitialised byte(s) ==31609== at 0x588EDA7: bind (syscall-template.S:81) ==31609== by 0x43A805: logging_open (log.c:76) ==31609== by 0x43A805: __btd_log_init (log.c:314) ==31609== by 0x40ADCD: main (main.c:688) ==31609== Address 0xfff0003d8 is on thread 1's stack ==31609== in frame #1, created by __btd_log_init (log.c:306) ==31609== ==31609== Jump to the invalid address stated on the next line ==31609== at 0x0: ??? ==31609== by 0x481B4C: queue_foreach (queue.c:220) ==31609== by 0x4436C8: att_disconnected (gatt-database.c:310) ==31609== by 0x481B4C: queue_foreach (queue.c:220) ==31609== by 0x486631: disconnect_cb (att.c:590) ==31609== by 0x48FBC4: watch_callback (io-glib.c:170) ==31609== by 0x4E7FCE4: g_main_context_dispatch (in /lib/x86_64-linux-gnu/libglib-2.0.so.0.4002.0) ==31609== by 0x4E80047: ??? (in /lib/x86_64-linux-gnu/libglib-2.0.so.0.4002.0) ==31609== by 0x4E80309: g_main_loop_run (in /lib/x86_64-linux-gnu/libglib-2.0.so.0.4002.0) ==31609== by 0x40B51A: main (main.c:770) ==31609== Address 0x0 is not stack'd, malloc'd or (recently) free'd ==31609== ==31609== ==31609== Process terminating with default action of signal 11 (SIGSEGV) ==31609== Bad permissions for mapped region at address 0x0 ==31609== at 0x0: ??? ==31609== by 0x481B4C: queue_foreach (queue.c:220) ==31609== by 0x4436C8: att_disconnected (gatt-database.c:310) ==31609== by 0x481B4C: queue_foreach (queue.c:220) ==31609== by 0x486631: disconnect_cb (att.c:590) ==31609== by 0x48FBC4: watch_callback (io-glib.c:170) ==31609== by 0x4E7FCE4: g_main_context_dispatch (in /lib/x86_64-linux-gnu/libglib-2.0.so.0.4002.0) ==31609== by 0x4E80047: ??? (in /lib/x86_64-linux-gnu/libglib-2.0.so.0.4002.0) ==31609== by 0x4E80309: g_main_loop_run (in /lib/x86_64-linux-gnu/libglib-2.0.so.0.4002.0) ==31609== by 0x40B51A: main (main.c:770) ==31609== ==31609== HEAP SUMMARY: ==31609== in use at exit: 131,062 bytes in 2,228 blocks ==31609== total heap usage: 16,067 allocs, 13,839 frees, 4,489,304 bytes allocated ==31609== ==31609== LEAK SUMMARY: ==31609== definitely lost: 0 bytes in 0 blocks ==31609== indirectly lost: 0 bytes in 0 blocks ==31609== possibly lost: 0 bytes in 0 blocks ==31609== still reachable: 131,062 bytes in 2,228 blocks ==31609== suppressed: 0 bytes in 0 blocks ==31609== Rerun with --leak-check=full to see details of leaked memory ==31609== ==31609== For counts of detected and suppressed errors, rerun with: -v ==31609== Use --track-origins=yes to see where uninitialised values come from ==31609== ERROR SUMMARY: 3 errors from 3 contexts (suppressed: 0 from 0) thanks best wishes yunhan >> thanks >> best wishes >> yunhan >> >> On Mon, Oct 23, 2017 at 1:29 PM, Luiz Augusto von Dentz >> <luiz.dentz@gmail.com> wrote: >>> Hi Yunhan, >>> >>> On Mon, Oct 23, 2017 at 11:26 PM, Yunhan Wang <yunhanw@nestlabs.com> wrote: >>>> Hi Luiz >>>> >>>> WIth latest bluez master, I am seeing segmentation fault in bluetoothd >>>> when ble disconnection happens. I think it related to this patch. >>>> >>>> The backtract is as below, >>>> #0 0x0000000000000000 in ?? () >>>> #1 0x0000000000481b4d in queue_foreach (queue=0x6fe4e0, >>>> function=function@entry=0x442d40 <clear_ccc_state>, >>>> user_data=0x6ee630) at src/shared/queue.c:220 >>>> #2 0x00000000004436c9 in att_disconnected (err=<optimized out>, >>>> user_data=0x6fe4b0) at src/gatt-database.c:310 >>>> #3 0x0000000000481b4d in queue_foreach (queue=0x705b70, >>>> function=function@entry=0x485280 <disconn_handler>, user_data=0x68) at >>>> src/shared/queue.c:220 >>>> #4 0x0000000000486632 in disconnect_cb (io=<optimized out>, >>>> user_data=0x70b140) at src/shared/att.c:590 >>>> #5 0x000000000048fbc5 in watch_callback (channel=<optimized out>, >>>> cond=<optimized out>, user_data=<optimized out>) at >>>> src/shared/io-glib.c:170 >>>> #6 0x00007ffff7b1ace5 in g_main_context_dispatch () from >>>> /lib/x86_64-linux-gnu/libglib-2.0.so.0 >>>> #7 0x00007ffff7b1b048 in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0 >>>> #8 0x00007ffff7b1b30a in g_main_loop_run () from >>>> /lib/x86_64-linux-gnu/libglib-2.0.so.0 >>>> #9 0x000000000040b51b in main (argc=1, argv=0x7fffffffe618) at src/main.c:770 >>> >>> There is a patch for this issue: >>> >>> https://git.kernel.org/pub/scm/bluetooth/bluez.git/commit/?id=261cf78db4be79a0f7d44798a57730b159c9be91 >>> >>>> thanks >>>> best wishes >>>> yunhan >>>> >>>> On Mon, Oct 23, 2017 at 4:17 AM, Luiz Augusto von Dentz >>>> <luiz.dentz@gmail.com> wrote: >>>>> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> >>>>> >>>>> If the device is no longer valid or is not considered bonded anymore >>>>> clear its CCC states before removing otherwise application may continue >>>>> to notify when there are no devices listening. >>>>> --- >>>>> src/gatt-database.c | 155 ++++++++++++++++++++++++++++++++++------------------ >>>>> 1 file changed, 103 insertions(+), 52 deletions(-) >>>>> >>>>> diff --git a/src/gatt-database.c b/src/gatt-database.c >>>>> index 47304704a..6784998c3 100644 >>>>> --- a/src/gatt-database.c >>>>> +++ b/src/gatt-database.c >>>>> @@ -150,8 +150,10 @@ struct pending_op { >>>>> }; >>>>> >>>>> struct device_state { >>>>> + struct btd_gatt_database *db; >>>>> bdaddr_t bdaddr; >>>>> uint8_t bdaddr_type; >>>>> + unsigned int disc_id; >>>>> struct queue *ccc_states; >>>>> }; >>>>> >>>>> @@ -245,12 +247,14 @@ static struct ccc_state *find_ccc_state(struct device_state *dev_state, >>>>> UINT_TO_PTR(handle)); >>>>> } >>>>> >>>>> -static struct device_state *device_state_create(bdaddr_t *bdaddr, >>>>> +static struct device_state *device_state_create(struct btd_gatt_database *db, >>>>> + bdaddr_t *bdaddr, >>>>> uint8_t bdaddr_type) >>>>> { >>>>> struct device_state *dev_state; >>>>> >>>>> dev_state = new0(struct device_state, 1); >>>>> + dev_state->db = db; >>>>> dev_state->ccc_states = queue_new(); >>>>> bacpy(&dev_state->bdaddr, bdaddr); >>>>> dev_state->bdaddr_type = bdaddr_type; >>>>> @@ -258,36 +262,117 @@ static struct device_state *device_state_create(bdaddr_t *bdaddr, >>>>> return dev_state; >>>>> } >>>>> >>>>> +static void device_state_free(void *data) >>>>> +{ >>>>> + struct device_state *state = data; >>>>> + >>>>> + queue_destroy(state->ccc_states, free); >>>>> + free(state); >>>>> +} >>>>> + >>>>> +static void clear_ccc_state(void *data, void *user_data) >>>>> +{ >>>>> + struct ccc_state *ccc = data; >>>>> + struct btd_gatt_database *db = user_data; >>>>> + struct ccc_cb_data *ccc_cb; >>>>> + >>>>> + if (!ccc->value[0]) >>>>> + return; >>>>> + >>>>> + ccc_cb = queue_find(db->ccc_callbacks, ccc_cb_match_handle, >>>>> + UINT_TO_PTR(ccc->handle)); >>>>> + if (!ccc_cb) >>>>> + return; >>>>> + >>>>> + ccc_cb->callback(NULL, 0, ccc_cb->user_data); >>>>> +} >>>>> + >>>>> +static void att_disconnected(int err, void *user_data) >>>>> +{ >>>>> + struct device_state *state = user_data; >>>>> + struct btd_device *device; >>>>> + >>>>> + DBG(""); >>>>> + >>>>> + state->disc_id = 0; >>>>> + >>>>> + device = btd_adapter_get_device(state->db->adapter, &state->bdaddr, >>>>> + state->bdaddr_type); >>>>> + if (!device) >>>>> + goto remove; >>>>> + >>>>> + if (device_is_paired(device, state->bdaddr_type)) >>>>> + return; >>>>> + >>>>> +remove: >>>>> + /* Remove device state if device no longer exists or is not paired */ >>>>> + if (queue_remove(state->db->device_states, state)) { >>>>> + queue_foreach(state->ccc_states, clear_ccc_state, state->db); >>>>> + device_state_free(state); >>>>> + } >>>>> +} >>>>> + >>>>> +static bool get_dst_info(struct bt_att *att, bdaddr_t *dst, uint8_t *dst_type) >>>>> +{ >>>>> + GIOChannel *io = NULL; >>>>> + GError *gerr = NULL; >>>>> + >>>>> + io = g_io_channel_unix_new(bt_att_get_fd(att)); >>>>> + if (!io) >>>>> + return false; >>>>> + >>>>> + bt_io_get(io, &gerr, BT_IO_OPT_DEST_BDADDR, dst, >>>>> + BT_IO_OPT_DEST_TYPE, dst_type, >>>>> + BT_IO_OPT_INVALID); >>>>> + if (gerr) { >>>>> + error("gatt: bt_io_get: %s", gerr->message); >>>>> + g_error_free(gerr); >>>>> + g_io_channel_unref(io); >>>>> + return false; >>>>> + } >>>>> + >>>>> + g_io_channel_unref(io); >>>>> + return true; >>>>> +} >>>>> + >>>>> static struct device_state *get_device_state(struct btd_gatt_database *database, >>>>> - bdaddr_t *bdaddr, >>>>> - uint8_t bdaddr_type) >>>>> + struct bt_att *att) >>>>> { >>>>> struct device_state *dev_state; >>>>> + bdaddr_t bdaddr; >>>>> + uint8_t bdaddr_type; >>>>> + >>>>> + if (!get_dst_info(att, &bdaddr, &bdaddr_type)) >>>>> + return NULL; >>>>> >>>>> /* >>>>> * Find and return a device state. If a matching state doesn't exist, >>>>> * then create a new one. >>>>> */ >>>>> - dev_state = find_device_state(database, bdaddr, bdaddr_type); >>>>> + dev_state = find_device_state(database, &bdaddr, bdaddr_type); >>>>> if (dev_state) >>>>> - return dev_state; >>>>> + goto done; >>>>> >>>>> - dev_state = device_state_create(bdaddr, bdaddr_type); >>>>> + dev_state = device_state_create(database, &bdaddr, bdaddr_type); >>>>> >>>>> queue_push_tail(database->device_states, dev_state); >>>>> >>>>> +done: >>>>> + dev_state->disc_id = bt_att_register_disconnect(att, att_disconnected, >>>>> + dev_state, NULL); >>>>> + >>>>> return dev_state; >>>>> } >>>>> >>>>> static struct ccc_state *get_ccc_state(struct btd_gatt_database *database, >>>>> - bdaddr_t *bdaddr, >>>>> - uint8_t bdaddr_type, >>>>> - uint16_t handle) >>>>> + struct bt_att *att, uint16_t handle) >>>>> { >>>>> struct device_state *dev_state; >>>>> struct ccc_state *ccc; >>>>> >>>>> - dev_state = get_device_state(database, bdaddr, bdaddr_type); >>>>> + dev_state = get_device_state(database, att); >>>>> + if (!dev_state) >>>>> + return NULL; >>>>> >>>>> ccc = find_ccc_state(dev_state, handle); >>>>> if (ccc) >>>>> @@ -300,14 +385,6 @@ static struct ccc_state *get_ccc_state(struct btd_gatt_database *database, >>>>> return ccc; >>>>> } >>>>> >>>>> -static void device_state_free(void *data) >>>>> -{ >>>>> - struct device_state *state = data; >>>>> - >>>>> - queue_destroy(state->ccc_states, free); >>>>> - free(state); >>>>> -} >>>>> - >>>>> static void cancel_pending_read(void *data) >>>>> { >>>>> struct pending_op *op = data; >>>>> @@ -690,29 +767,6 @@ static void populate_gap_service(struct btd_gatt_database *database) >>>>> gatt_db_service_set_active(service, true); >>>>> } >>>>> >>>>> -static bool get_dst_info(struct bt_att *att, bdaddr_t *dst, uint8_t *dst_type) >>>>> -{ >>>>> - GIOChannel *io = NULL; >>>>> - GError *gerr = NULL; >>>>> - >>>>> - io = g_io_channel_unix_new(bt_att_get_fd(att)); >>>>> - if (!io) >>>>> - return false; >>>>> - >>>>> - bt_io_get(io, &gerr, BT_IO_OPT_DEST_BDADDR, dst, >>>>> - BT_IO_OPT_DEST_TYPE, dst_type, >>>>> - BT_IO_OPT_INVALID); >>>>> - if (gerr) { >>>>> - error("gatt: bt_io_get: %s", gerr->message); >>>>> - g_error_free(gerr); >>>>> - g_io_channel_unref(io); >>>>> - return false; >>>>> - } >>>>> - >>>>> - g_io_channel_unref(io); >>>>> - return true; >>>>> -} >>>>> - >>>>> static void gatt_ccc_read_cb(struct gatt_db_attribute *attrib, >>>>> unsigned int id, uint16_t offset, >>>>> uint8_t opcode, struct bt_att *att, >>>>> @@ -724,8 +778,6 @@ static void gatt_ccc_read_cb(struct gatt_db_attribute *attrib, >>>>> uint8_t ecode = 0; >>>>> const uint8_t *value = NULL; >>>>> size_t len = 0; >>>>> - bdaddr_t bdaddr; >>>>> - uint8_t bdaddr_type; >>>>> >>>>> handle = gatt_db_attribute_get_handle(attrib); >>>>> >>>>> @@ -736,13 +788,12 @@ static void gatt_ccc_read_cb(struct gatt_db_attribute *attrib, >>>>> goto done; >>>>> } >>>>> >>>>> - if (!get_dst_info(att, &bdaddr, &bdaddr_type)) { >>>>> + ccc = get_ccc_state(database, att, handle); >>>>> + if (!ccc) { >>>>> ecode = BT_ATT_ERROR_UNLIKELY; >>>>> goto done; >>>>> } >>>>> >>>>> - ccc = get_ccc_state(database, &bdaddr, bdaddr_type, handle); >>>>> - >>>>> len = 2 - offset; >>>>> value = len ? &ccc->value[offset] : NULL; >>>>> >>>>> @@ -761,8 +812,6 @@ static void gatt_ccc_write_cb(struct gatt_db_attribute *attrib, >>>>> struct ccc_cb_data *ccc_cb; >>>>> uint16_t handle; >>>>> uint8_t ecode = 0; >>>>> - bdaddr_t bdaddr; >>>>> - uint8_t bdaddr_type; >>>>> >>>>> handle = gatt_db_attribute_get_handle(attrib); >>>>> >>>>> @@ -778,13 +827,12 @@ static void gatt_ccc_write_cb(struct gatt_db_attribute *attrib, >>>>> goto done; >>>>> } >>>>> >>>>> - if (!get_dst_info(att, &bdaddr, &bdaddr_type)) { >>>>> + ccc = get_ccc_state(database, att, handle); >>>>> + if (!ccc) { >>>>> ecode = BT_ATT_ERROR_UNLIKELY; >>>>> goto done; >>>>> } >>>>> >>>>> - ccc = get_ccc_state(database, &bdaddr, bdaddr_type, handle); >>>>> - >>>>> ccc_cb = queue_find(database->ccc_callbacks, ccc_cb_match_handle, >>>>> UINT_TO_PTR(gatt_db_attribute_get_handle(attrib))); >>>>> if (!ccc_cb) { >>>>> @@ -940,8 +988,11 @@ static void send_notification_to_device(void *data, void *user_data) >>>>> >>>>> remove: >>>>> /* Remove device state if device no longer exists or is not paired */ >>>>> - if (queue_remove(notify->database->device_states, device_state)) >>>>> + if (queue_remove(notify->database->device_states, device_state)) { >>>>> + queue_foreach(device_state->ccc_states, clear_ccc_state, >>>>> + notify->database); >>>>> device_state_free(device_state); >>>>> + } >>>>> } >>>>> >>>>> static void send_notification_to_devices(struct btd_gatt_database *database, >>>>> -- >>>>> 2.13.6 >>>>> >>>>> -- >>>>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in >>>>> the body of a message to majordomo@vger.kernel.org >>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>> >>> >>> >>> -- >>> Luiz Augusto von Dentz > > > > -- > Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH BlueZ 2/3] gatt: Clear subscriptions for device not paired 2017-10-25 7:58 ` Yunhan Wang @ 2017-10-25 13:27 ` Luiz Augusto von Dentz 2017-10-30 21:44 ` Yunhan Wang 0 siblings, 1 reply; 12+ messages in thread From: Luiz Augusto von Dentz @ 2017-10-25 13:27 UTC (permalink / raw) To: Yunhan Wang; +Cc: linux-bluetooth@vger.kernel.org Hi Yunhan, On Wed, Oct 25, 2017 at 10:58 AM, Yunhan Wang <yunhanw@nestlabs.com> wrote: > Hi, Luiz > > On Tue, Oct 24, 2017 at 1:10 AM, Luiz Augusto von Dentz > <luiz.dentz@gmail.com> wrote: >> Hi Yunhan, >> >> On Mon, Oct 23, 2017 at 11:59 PM, Yunhan Wang <yunhanw@nestlabs.com> wrote: >>> Hi, Luiz >>> >>> I have tried this, I think the issue is still there with your patch. >> >> I don't see any problem: >> >> bluetoothd[13968]: src/device.c:att_disconnected_cb() Software caused >> connection abort (103) >> bluetoothd[13968]: src/gatt-client.c:btd_gatt_client_disconnected() >> Device disconnected. Cleaning up. >> bluetoothd[13968]: src/device.c:att_disconnected_cb() Automatic >> connection disabled >> bluetoothd[13968]: attrib/gattrib.c:g_attrib_unref() 0x8897600: g_attrib_unref=0 >> bluetoothd[13968]: src/gatt-database.c:att_disconnected() >> bluetoothd[13968]: src/gatt-database.c:ccc_write_cb() External CCC >> write received with value: 0x0000 >> >> Could you try running under valgrind? >> > Yes, just tried under valgrind, the issue is constantly reprodcued. > Btw, I am currently using btvirt -L -l2, one client, one server, then > bluetoothd crash after client issue ble disconnect. I am using bluez > lateset master. > > sudo valgrind ./src/bluetoothd --experimental --debug > ==31609== Jump to the invalid address stated on the next line > ==31609== at 0x0: ??? > ==31609== by 0x481B4C: queue_foreach (queue.c:220) > ==31609== by 0x4436C8: att_disconnected (gatt-database.c:310) > ==31609== by 0x481B4C: queue_foreach (queue.c:220) > ==31609== by 0x486631: disconnect_cb (att.c:590) > ==31609== by 0x48FBC4: watch_callback (io-glib.c:170) > ==31609== by 0x4E7FCE4: g_main_context_dispatch (in > /lib/x86_64-linux-gnu/libglib-2.0.so.0.4002.0) > ==31609== by 0x4E80047: ??? (in > /lib/x86_64-linux-gnu/libglib-2.0.so.0.4002.0) > ==31609== by 0x4E80309: g_main_loop_run (in > /lib/x86_64-linux-gnu/libglib-2.0.so.0.4002.0) > ==31609== by 0x40B51A: main (main.c:770) > ==31609== Address 0x0 is not stack'd, malloc'd or (recently) free'd It turns out the ccc_cb->callback could be NULL in case of svc_changed, though I not sure why in our case it did not have the last call pointing to clear_ccc_state: at 0x0: ??? by 0x475C7C: clear_ccc_state (gatt-database.c:287) by 0x4D28CF: queue_foreach (queue.c:220) by 0x475FE7: att_disconnected (gatt-database.c:310) by 0x4D7255: disconn_handler (att.c:538) by 0x4D28CF: queue_foreach (queue.c:220) by 0x4D8F39: disconnect_cb (att.c:590) by 0x4E6B3A: watch_callback (io-glib.c:170) by 0x50CD246: g_main_context_dispatch (in /usr/lib64/libglib-2.0.so.0.5200.3) by 0x50CD5E7: ??? (in /usr/lib64/libglib-2.0.so.0.5200.3) by 0x50CD901: g_main_loop_run (in /usr/lib64/libglib-2.0.so.0.5200.3) by 0x40CD90: main (main.c:770) Address 0x0 is not stack'd, malloc'd or (recently) free'd Anyway now this should fixed upstream, thanks for the report. -- Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH BlueZ 2/3] gatt: Clear subscriptions for device not paired 2017-10-25 13:27 ` Luiz Augusto von Dentz @ 2017-10-30 21:44 ` Yunhan Wang 2017-11-02 17:45 ` Yunhan Wang 0 siblings, 1 reply; 12+ messages in thread From: Yunhan Wang @ 2017-10-30 21:44 UTC (permalink / raw) To: Luiz Augusto von Dentz; +Cc: linux-bluetooth@vger.kernel.org Hi, Luiz I am still consistently seeing multiple segmentation faults happening on this feature via bluetoothctl. I am using latest bluez master. Here are the full reproduced details using bluetoothctl Terminal 1(Bluetoothd): sudo gdb --args ./src/bluetoothd --experimental --debug run Terminal 2(btvirt 2 interface): sudo ./emulator/btvirt -L -l2 Terminal 3(Peripheral): select 00:AA:01:00:00:23 power on register-service 00001820-0000-1000-8000-00805f9b34fb register-characteristic 00002a06-0000-1000-8000-00805f9b34fb read,indicate register-application advertise on Terminal 4(Central): select 00:AA:01:01:00:24 power on scan on scan off connect 00:AA:01:00:00:23 select-attribute /org/bluez/hci1/dev_00_AA_01_00_00_23/service000a/char000b notify on disconnect Then one of segmentation faults happen Program received signal SIGSEGV, Segmentation fault. queue_remove (queue=0x30, data=data@entry=0x70b6c0) at src/shared/queue.c:256 256 for (entry = queue->head, prev = NULL; entry; (gdb) bt #0 queue_remove (queue=0x30, data=data@entry=0x70b6c0) at src/shared/queue.c:256 #1 0x00000000004437bb in att_disconnected (err=<optimized out>, user_data=0x70b6c0) at src/gatt-database.c:310 #2 0x0000000000481d6d in queue_foreach (queue=0x6f1d50, function=function@entry=0x4854a0 <disconn_handler>, user_data=0x68) at src/shared/queue.c:220 #3 0x0000000000486852 in disconnect_cb (io=<optimized out>, user_data=0x704ce0) at src/shared/att.c:590 #4 0x000000000048fde5 in watch_callback (channel=<optimized out>, cond=<optimized out>, user_data=<optimized out>) at src/shared/io-glib.c:170 #5 0x00007ffff7b1ace5 in g_main_context_dispatch () from /lib/x86_64-linux-gnu/libglib-2.0.so.0 #6 0x00007ffff7b1b048 in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0 #7 0x00007ffff7b1b30a in g_main_loop_run () from /lib/x86_64-linux-gnu/libglib-2.0.so.0 #8 0x000000000040b56b in main (argc=1, argv=0x7fffffffe618) at src/main.c:770 some of other faults: 1. Program received signal SIGSEGV, Segmentation fault. att_disconnected (err=<optimized out>, user_data=0x703270) at src/gatt-database.c:300 300 if (!state->db->adapter) (gdb) (gdb) bt #0 att_disconnected (err=<optimized out>, user_data=0x703270) at src/gatt-database.c:300 #1 0x0000000000481ccd in queue_foreach (queue=0x6fd7e0, function=function@entry=0x485400 <disconn_handler>, user_data=0x68) at src/shared/queue.c:220 #2 0x00000000004867b2 in disconnect_cb (io=<optimized out>, user_data=0x6ee310) at src/shared/att.c:590 #3 0x000000000048fd45 in watch_callback (channel=<optimized out>, cond=<optimized out>, user_data=<optimized out>) at src/shared/io-glib.c:170 #4 0x00007ffff7b1ace5 in g_main_context_dispatch () from /lib/x86_64-linux-gnu/libglib-2.0.so.0 #5 0x00007ffff7b1b048 in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0 #6 0x00007ffff7b1b30a in g_main_loop_run () from /lib/x86_64-linux-gnu/libglib-2.0.so.0 #7 0x000000000040b56b in main (argc=1, argv=0x7fffffffe618) at src/main.c:770 (gdb) print state $1 = (struct device_state *) 0x703270 (gdb) print state->db $2 = (struct btd_gatt_database *) 0x0 2. Program received signal SIGSEGV, Segmentation fault. btd_adapter_find_device (adapter=0x2a, dst=dst@entry=0x70a2e8, bdaddr_type=0 '\000') at src/adapter.c:821 821 list = g_slist_find_custom(adapter->devices, &addr, (gdb) quit Thanks Best wishes Yunhan On Wed, Oct 25, 2017 at 6:27 AM, Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > Hi Yunhan, > > On Wed, Oct 25, 2017 at 10:58 AM, Yunhan Wang <yunhanw@nestlabs.com> wrote: >> Hi, Luiz >> >> On Tue, Oct 24, 2017 at 1:10 AM, Luiz Augusto von Dentz >> <luiz.dentz@gmail.com> wrote: >>> Hi Yunhan, >>> >>> On Mon, Oct 23, 2017 at 11:59 PM, Yunhan Wang <yunhanw@nestlabs.com> wrote: >>>> Hi, Luiz >>>> >>>> I have tried this, I think the issue is still there with your patch. >>> >>> I don't see any problem: >>> >>> bluetoothd[13968]: src/device.c:att_disconnected_cb() Software caused >>> connection abort (103) >>> bluetoothd[13968]: src/gatt-client.c:btd_gatt_client_disconnected() >>> Device disconnected. Cleaning up. >>> bluetoothd[13968]: src/device.c:att_disconnected_cb() Automatic >>> connection disabled >>> bluetoothd[13968]: attrib/gattrib.c:g_attrib_unref() 0x8897600: g_attrib_unref=0 >>> bluetoothd[13968]: src/gatt-database.c:att_disconnected() >>> bluetoothd[13968]: src/gatt-database.c:ccc_write_cb() External CCC >>> write received with value: 0x0000 >>> >>> Could you try running under valgrind? >>> >> Yes, just tried under valgrind, the issue is constantly reprodcued. >> Btw, I am currently using btvirt -L -l2, one client, one server, then >> bluetoothd crash after client issue ble disconnect. I am using bluez >> lateset master. >> >> sudo valgrind ./src/bluetoothd --experimental --debug >> ==31609== Jump to the invalid address stated on the next line >> ==31609== at 0x0: ??? >> ==31609== by 0x481B4C: queue_foreach (queue.c:220) >> ==31609== by 0x4436C8: att_disconnected (gatt-database.c:310) >> ==31609== by 0x481B4C: queue_foreach (queue.c:220) >> ==31609== by 0x486631: disconnect_cb (att.c:590) >> ==31609== by 0x48FBC4: watch_callback (io-glib.c:170) >> ==31609== by 0x4E7FCE4: g_main_context_dispatch (in >> /lib/x86_64-linux-gnu/libglib-2.0.so.0.4002.0) >> ==31609== by 0x4E80047: ??? (in >> /lib/x86_64-linux-gnu/libglib-2.0.so.0.4002.0) >> ==31609== by 0x4E80309: g_main_loop_run (in >> /lib/x86_64-linux-gnu/libglib-2.0.so.0.4002.0) >> ==31609== by 0x40B51A: main (main.c:770) >> ==31609== Address 0x0 is not stack'd, malloc'd or (recently) free'd > > It turns out the ccc_cb->callback could be NULL in case of > svc_changed, though I not sure why in our case it did not have the > last call pointing to clear_ccc_state: > > at 0x0: ??? > by 0x475C7C: clear_ccc_state (gatt-database.c:287) > by 0x4D28CF: queue_foreach (queue.c:220) > by 0x475FE7: att_disconnected (gatt-database.c:310) > by 0x4D7255: disconn_handler (att.c:538) > by 0x4D28CF: queue_foreach (queue.c:220) > by 0x4D8F39: disconnect_cb (att.c:590) > by 0x4E6B3A: watch_callback (io-glib.c:170) > by 0x50CD246: g_main_context_dispatch (in > /usr/lib64/libglib-2.0.so.0.5200.3) > by 0x50CD5E7: ??? (in /usr/lib64/libglib-2.0.so.0.5200.3) > by 0x50CD901: g_main_loop_run (in /usr/lib64/libglib-2.0.so.0.5200.3) > by 0x40CD90: main (main.c:770) > Address 0x0 is not stack'd, malloc'd or (recently) free'd > > Anyway now this should fixed upstream, thanks for the report. > > -- > Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH BlueZ 2/3] gatt: Clear subscriptions for device not paired 2017-10-30 21:44 ` Yunhan Wang @ 2017-11-02 17:45 ` Yunhan Wang 2017-11-02 18:07 ` Luiz Augusto von Dentz 0 siblings, 1 reply; 12+ messages in thread From: Yunhan Wang @ 2017-11-02 17:45 UTC (permalink / raw) To: Luiz Augusto von Dentz; +Cc: linux-bluetooth@vger.kernel.org Hi Luiz It is working now with your latest fix. Thanks Best wishes Yunhan On Mon, Oct 30, 2017 at 2:44 PM, Yunhan Wang <yunhanw@google.com> wrote: > Hi, Luiz > > I am still consistently seeing multiple segmentation faults happening > on this feature via bluetoothctl. I am using latest bluez master. > > Here are the full reproduced details using bluetoothctl > Terminal 1(Bluetoothd): > sudo gdb --args ./src/bluetoothd --experimental --debug > run > > Terminal 2(btvirt 2 interface): > sudo ./emulator/btvirt -L -l2 > > Terminal 3(Peripheral): > select 00:AA:01:00:00:23 > power on > register-service 00001820-0000-1000-8000-00805f9b34fb > register-characteristic 00002a06-0000-1000-8000-00805f9b34fb read,indicate > register-application > advertise on > > Terminal 4(Central): > select 00:AA:01:01:00:24 > power on > scan on > scan off > connect 00:AA:01:00:00:23 > select-attribute /org/bluez/hci1/dev_00_AA_01_00_00_23/service000a/char000b > notify on > disconnect > > Then one of segmentation faults happen > > Program received signal SIGSEGV, Segmentation fault. > queue_remove (queue=0x30, data=data@entry=0x70b6c0) at src/shared/queue.c:256 > 256 for (entry = queue->head, prev = NULL; entry; > (gdb) bt > #0 queue_remove (queue=0x30, data=data@entry=0x70b6c0) > at src/shared/queue.c:256 > #1 0x00000000004437bb in att_disconnected (err=<optimized out>, > user_data=0x70b6c0) at src/gatt-database.c:310 > #2 0x0000000000481d6d in queue_foreach (queue=0x6f1d50, > function=function@entry=0x4854a0 <disconn_handler>, user_data=0x68) > at src/shared/queue.c:220 > #3 0x0000000000486852 in disconnect_cb (io=<optimized out>, > user_data=0x704ce0) at src/shared/att.c:590 > #4 0x000000000048fde5 in watch_callback (channel=<optimized out>, > cond=<optimized out>, user_data=<optimized out>) > at src/shared/io-glib.c:170 > #5 0x00007ffff7b1ace5 in g_main_context_dispatch () > from /lib/x86_64-linux-gnu/libglib-2.0.so.0 > #6 0x00007ffff7b1b048 in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0 > #7 0x00007ffff7b1b30a in g_main_loop_run () > from /lib/x86_64-linux-gnu/libglib-2.0.so.0 > #8 0x000000000040b56b in main (argc=1, argv=0x7fffffffe618) at src/main.c:770 > > some of other faults: > 1. > Program received signal SIGSEGV, Segmentation fault. > att_disconnected (err=<optimized out>, user_data=0x703270) > at src/gatt-database.c:300 > 300 if (!state->db->adapter) > (gdb) > (gdb) bt > #0 att_disconnected (err=<optimized out>, user_data=0x703270) > at src/gatt-database.c:300 > #1 0x0000000000481ccd in queue_foreach (queue=0x6fd7e0, > function=function@entry=0x485400 <disconn_handler>, user_data=0x68) > at src/shared/queue.c:220 > #2 0x00000000004867b2 in disconnect_cb (io=<optimized out>, > user_data=0x6ee310) at src/shared/att.c:590 > #3 0x000000000048fd45 in watch_callback (channel=<optimized out>, > cond=<optimized out>, user_data=<optimized out>) > at src/shared/io-glib.c:170 > #4 0x00007ffff7b1ace5 in g_main_context_dispatch () > from /lib/x86_64-linux-gnu/libglib-2.0.so.0 > #5 0x00007ffff7b1b048 in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0 > #6 0x00007ffff7b1b30a in g_main_loop_run () > from /lib/x86_64-linux-gnu/libglib-2.0.so.0 > #7 0x000000000040b56b in main (argc=1, argv=0x7fffffffe618) at src/main.c:770 > (gdb) print state > $1 = (struct device_state *) 0x703270 > (gdb) print state->db > $2 = (struct btd_gatt_database *) 0x0 > > 2. > Program received signal SIGSEGV, Segmentation fault. > btd_adapter_find_device (adapter=0x2a, dst=dst@entry=0x70a2e8, > bdaddr_type=0 '\000') at src/adapter.c:821 > 821 list = g_slist_find_custom(adapter->devices, &addr, > (gdb) quit > > > Thanks > Best wishes > Yunhan > > On Wed, Oct 25, 2017 at 6:27 AM, Luiz Augusto von Dentz > <luiz.dentz@gmail.com> wrote: >> Hi Yunhan, >> >> On Wed, Oct 25, 2017 at 10:58 AM, Yunhan Wang <yunhanw@nestlabs.com> wrote: >>> Hi, Luiz >>> >>> On Tue, Oct 24, 2017 at 1:10 AM, Luiz Augusto von Dentz >>> <luiz.dentz@gmail.com> wrote: >>>> Hi Yunhan, >>>> >>>> On Mon, Oct 23, 2017 at 11:59 PM, Yunhan Wang <yunhanw@nestlabs.com> wrote: >>>>> Hi, Luiz >>>>> >>>>> I have tried this, I think the issue is still there with your patch. >>>> >>>> I don't see any problem: >>>> >>>> bluetoothd[13968]: src/device.c:att_disconnected_cb() Software caused >>>> connection abort (103) >>>> bluetoothd[13968]: src/gatt-client.c:btd_gatt_client_disconnected() >>>> Device disconnected. Cleaning up. >>>> bluetoothd[13968]: src/device.c:att_disconnected_cb() Automatic >>>> connection disabled >>>> bluetoothd[13968]: attrib/gattrib.c:g_attrib_unref() 0x8897600: g_attrib_unref=0 >>>> bluetoothd[13968]: src/gatt-database.c:att_disconnected() >>>> bluetoothd[13968]: src/gatt-database.c:ccc_write_cb() External CCC >>>> write received with value: 0x0000 >>>> >>>> Could you try running under valgrind? >>>> >>> Yes, just tried under valgrind, the issue is constantly reprodcued. >>> Btw, I am currently using btvirt -L -l2, one client, one server, then >>> bluetoothd crash after client issue ble disconnect. I am using bluez >>> lateset master. >>> >>> sudo valgrind ./src/bluetoothd --experimental --debug >>> ==31609== Jump to the invalid address stated on the next line >>> ==31609== at 0x0: ??? >>> ==31609== by 0x481B4C: queue_foreach (queue.c:220) >>> ==31609== by 0x4436C8: att_disconnected (gatt-database.c:310) >>> ==31609== by 0x481B4C: queue_foreach (queue.c:220) >>> ==31609== by 0x486631: disconnect_cb (att.c:590) >>> ==31609== by 0x48FBC4: watch_callback (io-glib.c:170) >>> ==31609== by 0x4E7FCE4: g_main_context_dispatch (in >>> /lib/x86_64-linux-gnu/libglib-2.0.so.0.4002.0) >>> ==31609== by 0x4E80047: ??? (in >>> /lib/x86_64-linux-gnu/libglib-2.0.so.0.4002.0) >>> ==31609== by 0x4E80309: g_main_loop_run (in >>> /lib/x86_64-linux-gnu/libglib-2.0.so.0.4002.0) >>> ==31609== by 0x40B51A: main (main.c:770) >>> ==31609== Address 0x0 is not stack'd, malloc'd or (recently) free'd >> >> It turns out the ccc_cb->callback could be NULL in case of >> svc_changed, though I not sure why in our case it did not have the >> last call pointing to clear_ccc_state: >> >> at 0x0: ??? >> by 0x475C7C: clear_ccc_state (gatt-database.c:287) >> by 0x4D28CF: queue_foreach (queue.c:220) >> by 0x475FE7: att_disconnected (gatt-database.c:310) >> by 0x4D7255: disconn_handler (att.c:538) >> by 0x4D28CF: queue_foreach (queue.c:220) >> by 0x4D8F39: disconnect_cb (att.c:590) >> by 0x4E6B3A: watch_callback (io-glib.c:170) >> by 0x50CD246: g_main_context_dispatch (in >> /usr/lib64/libglib-2.0.so.0.5200.3) >> by 0x50CD5E7: ??? (in /usr/lib64/libglib-2.0.so.0.5200.3) >> by 0x50CD901: g_main_loop_run (in /usr/lib64/libglib-2.0.so.0.5200.3) >> by 0x40CD90: main (main.c:770) >> Address 0x0 is not stack'd, malloc'd or (recently) free'd >> >> Anyway now this should fixed upstream, thanks for the report. >> >> -- >> Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH BlueZ 2/3] gatt: Clear subscriptions for device not paired 2017-11-02 17:45 ` Yunhan Wang @ 2017-11-02 18:07 ` Luiz Augusto von Dentz 0 siblings, 0 replies; 12+ messages in thread From: Luiz Augusto von Dentz @ 2017-11-02 18:07 UTC (permalink / raw) To: Yunhan Wang; +Cc: linux-bluetooth@vger.kernel.org Hi Yunhan, On Thu, Nov 2, 2017 at 7:45 PM, Yunhan Wang <yunhanw@google.com> wrote: > Hi Luiz > > It is working now with your latest fix. Great, hopefully this will settle things for server notifications. > Thanks > Best wishes > Yunhan > > On Mon, Oct 30, 2017 at 2:44 PM, Yunhan Wang <yunhanw@google.com> wrote: >> Hi, Luiz >> >> I am still consistently seeing multiple segmentation faults happening >> on this feature via bluetoothctl. I am using latest bluez master. >> >> Here are the full reproduced details using bluetoothctl >> Terminal 1(Bluetoothd): >> sudo gdb --args ./src/bluetoothd --experimental --debug >> run >> >> Terminal 2(btvirt 2 interface): >> sudo ./emulator/btvirt -L -l2 >> >> Terminal 3(Peripheral): >> select 00:AA:01:00:00:23 >> power on >> register-service 00001820-0000-1000-8000-00805f9b34fb >> register-characteristic 00002a06-0000-1000-8000-00805f9b34fb read,indicate >> register-application >> advertise on >> >> Terminal 4(Central): >> select 00:AA:01:01:00:24 >> power on >> scan on >> scan off >> connect 00:AA:01:00:00:23 >> select-attribute /org/bluez/hci1/dev_00_AA_01_00_00_23/service000a/char000b >> notify on >> disconnect >> >> Then one of segmentation faults happen >> >> Program received signal SIGSEGV, Segmentation fault. >> queue_remove (queue=0x30, data=data@entry=0x70b6c0) at src/shared/queue.c:256 >> 256 for (entry = queue->head, prev = NULL; entry; >> (gdb) bt >> #0 queue_remove (queue=0x30, data=data@entry=0x70b6c0) >> at src/shared/queue.c:256 >> #1 0x00000000004437bb in att_disconnected (err=<optimized out>, >> user_data=0x70b6c0) at src/gatt-database.c:310 >> #2 0x0000000000481d6d in queue_foreach (queue=0x6f1d50, >> function=function@entry=0x4854a0 <disconn_handler>, user_data=0x68) >> at src/shared/queue.c:220 >> #3 0x0000000000486852 in disconnect_cb (io=<optimized out>, >> user_data=0x704ce0) at src/shared/att.c:590 >> #4 0x000000000048fde5 in watch_callback (channel=<optimized out>, >> cond=<optimized out>, user_data=<optimized out>) >> at src/shared/io-glib.c:170 >> #5 0x00007ffff7b1ace5 in g_main_context_dispatch () >> from /lib/x86_64-linux-gnu/libglib-2.0.so.0 >> #6 0x00007ffff7b1b048 in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0 >> #7 0x00007ffff7b1b30a in g_main_loop_run () >> from /lib/x86_64-linux-gnu/libglib-2.0.so.0 >> #8 0x000000000040b56b in main (argc=1, argv=0x7fffffffe618) at src/main.c:770 >> >> some of other faults: >> 1. >> Program received signal SIGSEGV, Segmentation fault. >> att_disconnected (err=<optimized out>, user_data=0x703270) >> at src/gatt-database.c:300 >> 300 if (!state->db->adapter) >> (gdb) >> (gdb) bt >> #0 att_disconnected (err=<optimized out>, user_data=0x703270) >> at src/gatt-database.c:300 >> #1 0x0000000000481ccd in queue_foreach (queue=0x6fd7e0, >> function=function@entry=0x485400 <disconn_handler>, user_data=0x68) >> at src/shared/queue.c:220 >> #2 0x00000000004867b2 in disconnect_cb (io=<optimized out>, >> user_data=0x6ee310) at src/shared/att.c:590 >> #3 0x000000000048fd45 in watch_callback (channel=<optimized out>, >> cond=<optimized out>, user_data=<optimized out>) >> at src/shared/io-glib.c:170 >> #4 0x00007ffff7b1ace5 in g_main_context_dispatch () >> from /lib/x86_64-linux-gnu/libglib-2.0.so.0 >> #5 0x00007ffff7b1b048 in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0 >> #6 0x00007ffff7b1b30a in g_main_loop_run () >> from /lib/x86_64-linux-gnu/libglib-2.0.so.0 >> #7 0x000000000040b56b in main (argc=1, argv=0x7fffffffe618) at src/main.c:770 >> (gdb) print state >> $1 = (struct device_state *) 0x703270 >> (gdb) print state->db >> $2 = (struct btd_gatt_database *) 0x0 >> >> 2. >> Program received signal SIGSEGV, Segmentation fault. >> btd_adapter_find_device (adapter=0x2a, dst=dst@entry=0x70a2e8, >> bdaddr_type=0 '\000') at src/adapter.c:821 >> 821 list = g_slist_find_custom(adapter->devices, &addr, >> (gdb) quit >> >> >> Thanks >> Best wishes >> Yunhan >> >> On Wed, Oct 25, 2017 at 6:27 AM, Luiz Augusto von Dentz >> <luiz.dentz@gmail.com> wrote: >>> Hi Yunhan, >>> >>> On Wed, Oct 25, 2017 at 10:58 AM, Yunhan Wang <yunhanw@nestlabs.com> wrote: >>>> Hi, Luiz >>>> >>>> On Tue, Oct 24, 2017 at 1:10 AM, Luiz Augusto von Dentz >>>> <luiz.dentz@gmail.com> wrote: >>>>> Hi Yunhan, >>>>> >>>>> On Mon, Oct 23, 2017 at 11:59 PM, Yunhan Wang <yunhanw@nestlabs.com> wrote: >>>>>> Hi, Luiz >>>>>> >>>>>> I have tried this, I think the issue is still there with your patch. >>>>> >>>>> I don't see any problem: >>>>> >>>>> bluetoothd[13968]: src/device.c:att_disconnected_cb() Software caused >>>>> connection abort (103) >>>>> bluetoothd[13968]: src/gatt-client.c:btd_gatt_client_disconnected() >>>>> Device disconnected. Cleaning up. >>>>> bluetoothd[13968]: src/device.c:att_disconnected_cb() Automatic >>>>> connection disabled >>>>> bluetoothd[13968]: attrib/gattrib.c:g_attrib_unref() 0x8897600: g_attrib_unref=0 >>>>> bluetoothd[13968]: src/gatt-database.c:att_disconnected() >>>>> bluetoothd[13968]: src/gatt-database.c:ccc_write_cb() External CCC >>>>> write received with value: 0x0000 >>>>> >>>>> Could you try running under valgrind? >>>>> >>>> Yes, just tried under valgrind, the issue is constantly reprodcued. >>>> Btw, I am currently using btvirt -L -l2, one client, one server, then >>>> bluetoothd crash after client issue ble disconnect. I am using bluez >>>> lateset master. >>>> >>>> sudo valgrind ./src/bluetoothd --experimental --debug >>>> ==31609== Jump to the invalid address stated on the next line >>>> ==31609== at 0x0: ??? >>>> ==31609== by 0x481B4C: queue_foreach (queue.c:220) >>>> ==31609== by 0x4436C8: att_disconnected (gatt-database.c:310) >>>> ==31609== by 0x481B4C: queue_foreach (queue.c:220) >>>> ==31609== by 0x486631: disconnect_cb (att.c:590) >>>> ==31609== by 0x48FBC4: watch_callback (io-glib.c:170) >>>> ==31609== by 0x4E7FCE4: g_main_context_dispatch (in >>>> /lib/x86_64-linux-gnu/libglib-2.0.so.0.4002.0) >>>> ==31609== by 0x4E80047: ??? (in >>>> /lib/x86_64-linux-gnu/libglib-2.0.so.0.4002.0) >>>> ==31609== by 0x4E80309: g_main_loop_run (in >>>> /lib/x86_64-linux-gnu/libglib-2.0.so.0.4002.0) >>>> ==31609== by 0x40B51A: main (main.c:770) >>>> ==31609== Address 0x0 is not stack'd, malloc'd or (recently) free'd >>> >>> It turns out the ccc_cb->callback could be NULL in case of >>> svc_changed, though I not sure why in our case it did not have the >>> last call pointing to clear_ccc_state: >>> >>> at 0x0: ??? >>> by 0x475C7C: clear_ccc_state (gatt-database.c:287) >>> by 0x4D28CF: queue_foreach (queue.c:220) >>> by 0x475FE7: att_disconnected (gatt-database.c:310) >>> by 0x4D7255: disconn_handler (att.c:538) >>> by 0x4D28CF: queue_foreach (queue.c:220) >>> by 0x4D8F39: disconnect_cb (att.c:590) >>> by 0x4E6B3A: watch_callback (io-glib.c:170) >>> by 0x50CD246: g_main_context_dispatch (in >>> /usr/lib64/libglib-2.0.so.0.5200.3) >>> by 0x50CD5E7: ??? (in /usr/lib64/libglib-2.0.so.0.5200.3) >>> by 0x50CD901: g_main_loop_run (in /usr/lib64/libglib-2.0.so.0.5200.3) >>> by 0x40CD90: main (main.c:770) >>> Address 0x0 is not stack'd, malloc'd or (recently) free'd >>> >>> Anyway now this should fixed upstream, thanks for the report. >>> >>> -- >>> Luiz Augusto von Dentz -- Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH BlueZ 3/3] test/example-gatt-server: Don't change measuments if not notifying 2017-10-23 11:17 [PATCH BlueZ 1/3] shared/att: Fix crash when calling disconnect handlers Luiz Augusto von Dentz 2017-10-23 11:17 ` [PATCH BlueZ 2/3] gatt: Clear subscriptions for device not paired Luiz Augusto von Dentz @ 2017-10-23 11:17 ` Luiz Augusto von Dentz 1 sibling, 0 replies; 12+ messages in thread From: Luiz Augusto von Dentz @ 2017-10-23 11:17 UTC (permalink / raw) To: linux-bluetooth From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> This makes it clearer when notifications are actually in effect. --- test/example-gatt-server | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/example-gatt-server b/test/example-gatt-server index 24aaff973..689e86ff7 100755 --- a/test/example-gatt-server +++ b/test/example-gatt-server @@ -401,6 +401,8 @@ class BatteryLevelCharacteristic(Characteristic): { 'Value': [dbus.Byte(self.battery_lvl)] }, []) def drain_battery(self): + if not self.notifying: + return True if self.battery_lvl > 0: self.battery_lvl -= 2 if self.battery_lvl < 0: -- 2.13.6 ^ permalink raw reply related [flat|nested] 12+ messages in thread
end of thread, other threads:[~2017-11-02 18:07 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-10-23 11:17 [PATCH BlueZ 1/3] shared/att: Fix crash when calling disconnect handlers Luiz Augusto von Dentz 2017-10-23 11:17 ` [PATCH BlueZ 2/3] gatt: Clear subscriptions for device not paired Luiz Augusto von Dentz 2017-10-23 20:26 ` Yunhan Wang 2017-10-23 20:29 ` Luiz Augusto von Dentz 2017-10-23 20:59 ` Yunhan Wang 2017-10-24 8:10 ` Luiz Augusto von Dentz 2017-10-25 7:58 ` Yunhan Wang 2017-10-25 13:27 ` Luiz Augusto von Dentz 2017-10-30 21:44 ` Yunhan Wang 2017-11-02 17:45 ` Yunhan Wang 2017-11-02 18:07 ` Luiz Augusto von Dentz 2017-10-23 11:17 ` [PATCH BlueZ 3/3] test/example-gatt-server: Don't change measuments if not notifying Luiz Augusto von Dentz
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).