All of lore.kernel.org
 help / color / mirror / Atom feed
* [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

* [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

* 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

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

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

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

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

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

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

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

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

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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.