[PATCH BlueZ 1/4] shared/gatt-client: Fix not resetting request id

[PATCH BlueZ 1/4] shared/gatt-client: Fix not resetting request id

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

The request id shall be reset to 0 after bt_att_cancel has been called.
---
 src/shared/gatt-client.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c
index 06ac763..5fd0208 100644
--- a/src/shared/gatt-client.c
+++ b/src/shared/gatt-client.c
@@ -1581,6 +1581,7 @@ static void complete_unregister_notify(void *data)
 	 */
 	if (notify_data->att_id) {
 		bt_att_cancel(notify_data->client->att, notify_data->att_id);
+		notify_data->att_id = 0;
 		goto done;
 	}
 
-- 
2.5.0

[PATCH BlueZ 2/4] core/gatt-client: Fix not being able to cancel notifications

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

While the notification is still in process notifying flag wont be set but
it shall still be possible to cancel it otherwise the client has to wait
until the remote respond which can take a lot of time (up to 30 seconds).
---
 src/gatt-client.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/src/gatt-client.c b/src/gatt-client.c
index 39f6646..b4ca3b6 100644
--- a/src/gatt-client.c
+++ b/src/gatt-client.c
@@ -1205,9 +1205,6 @@ static DBusMessage *characteristic_stop_notify(DBusConnection *conn,
 	const char *sender = dbus_message_get_sender(msg);
 	struct notify_client *client;
 
-	if (!chrc->notifying)
-		return btd_error_failed(msg, "Not notifying");
-
 	client = queue_remove_if(chrc->notify_clients, match_notify_sender,
 							(void *) sender);
 	if (!client)
-- 
2.5.0

[PATCH BlueZ 3/4] shared/gatt-client: Fix bogus asserts

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

The asserts involving notify_count were bogus because it should be
possible to disable a notification even before the remote respond to
the write command.
---
 src/shared/gatt-client.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c
index 5fd0208..91acad5 100644
--- a/src/shared/gatt-client.c
+++ b/src/shared/gatt-client.c
@@ -1116,9 +1116,6 @@ static void complete_notify_request(void *data)
 {
 	struct notify_data *notify_data = data;
 
-	/* Increment the per-characteristic ref count of notify handlers */
-	__sync_fetch_and_add(&notify_data->chrc->notify_count, 1);
-
 	notify_data->att_id = 0;
 	notify_data->callback(0, notify_data->user_data);
 }
@@ -1174,7 +1171,6 @@ static void enable_ccc_callback(uint8_t opcode, const void *pdu,
 	struct notify_data *notify_data = user_data;
 	uint16_t att_ecode;
 
-	assert(!notify_data->chrc->notify_count);
 	assert(notify_data->chrc->ccc_write_id);
 
 	notify_data->chrc->ccc_write_id = 0;
@@ -1264,6 +1260,9 @@ static unsigned int register_notify(struct bt_gatt_client *client,
 
 	notify_data->id = client->next_reg_id++;
 
+	/* Increment the per-characteristic ref count of notify handlers */
+	__sync_fetch_and_add(&notify_data->chrc->notify_count, 1);
+
 	/*
 	 * If a write to the CCC descriptor is in progress, then queue this
 	 * request.
@@ -1274,9 +1273,9 @@ static unsigned int register_notify(struct bt_gatt_client *client,
 	}
 
 	/*
-	 * If the ref count is not zero, then notifications are already enabled.
+	 * If the ref count > 1, then notifications are already enabled.
 	 */
-	if (chrc->notify_count > 0 || !chrc->ccc_handle) {
+	if (chrc->notify_count > 1 || !chrc->ccc_handle) {
 		complete_notify_request(notify_data);
 		return notify_data->id;
 	}
@@ -1555,7 +1554,6 @@ static void disable_ccc_callback(uint8_t opcode, const void *pdu,
 	struct notify_data *notify_data = user_data;
 	struct notify_data *next_data;
 
-	assert(!notify_data->chrc->notify_count);
 	assert(notify_data->chrc->ccc_write_id);
 
 	notify_data->chrc->ccc_write_id = 0;
@@ -2960,9 +2958,6 @@ bool bt_gatt_client_unregister_notify(struct bt_gatt_client *client,
 	if (!notify_data)
 		return false;
 
-	assert(notify_data->chrc->notify_count > 0);
-	assert(!notify_data->chrc->ccc_write_id);
-
 	complete_unregister_notify(notify_data);
 	return true;
 }
-- 
2.5.0

[PATCH BlueZ 4/4] shared/gatt-client: Fix crash unregistering notification

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

This fixes the following crash which happen when a notification is queued
waiting for CCC to be written but it cancelled before it completes:

Invalid read of size 4
  at 0x4E0C38: notify_data_unref (gatt-client.c:201)
  by 0x4D9BBE: queue_remove_all (queue.c:351)
  by 0x4D9C14: queue_destroy (queue.c:73)
  by 0x4E0DBB: notify_chrc_free (gatt-client.c:277)
  by 0x4D9BBE: queue_remove_all (queue.c:351)
  by 0x4D9C14: queue_destroy (queue.c:73)
  by 0x4E3860: bt_gatt_client_free (gatt-client.c:1664)
  by 0x4E38D0: bt_gatt_client_unref (gatt-client.c:1749)
  by 0x45D324: gas_free (gas.c:65)
  by 0x45D324: gap_driver_remove (gas.c:254)
  by 0x4A72A0: service_remove (service.c:176)
  by 0x4B74E9: device_remove (device.c:3865)
  by 0x492971: adapter_remove (adapter.c:5236)
Address 0x8cb8830 is 16 bytes inside a block of size 64 free'd
  at 0x4C29E00: free (vg_replace_malloc.c:530)
  by 0x4E0C69: notify_data_unref (gatt-client.c:207)
  by 0x4E58C9: complete_unregister_notify (gatt-client.c:1593)
  by 0x4E58C9: bt_gatt_client_unregister_notify (gatt-client.c:2961)
  by 0x4A8D77: characteristic_stop_notify (gatt-client.c:1214)
  by 0x4D4E82: process_message.isra.3 (object.c:259)
  by 0x4D56DC: generic_message (object.c:1071)
  by 0x53DD1A2: ??? (in /usr/lib64/libdbus-1.so.3.14.6)
  by 0x53CE733: dbus_connection_dispatch (in /usr/lib64/libdbus-1.so.3.14.6)
  by 0x4CFADF: message_dispatch (mainloop.c:72)
  by 0x50C8E39: g_main_context_dispatch (in /usr/lib64/libglib-2.0.so.0.4600.2)
  by 0x50C91CF: ??? (in /usr/lib64/libglib-2.0.so.0.4600.2)
  by 0x50C94F1: g_main_loop_run (in /usr/lib64/libglib-2.0.so.0.4600.2)
---
 src/shared/gatt-client.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c
index 91acad5..130d602 100644
--- a/src/shared/gatt-client.c
+++ b/src/shared/gatt-client.c
@@ -2958,6 +2958,9 @@ bool bt_gatt_client_unregister_notify(struct bt_gatt_client *client,
 	if (!notify_data)
 		return false;
 
+	/* Remove data if it has been queued */
+	queue_remove(notify_data->chrc->reg_notify_queue, notify_data);
+
 	complete_unregister_notify(notify_data);
 	return true;
 }
-- 
2.5.0

Re: [PATCH BlueZ 1/4] shared/gatt-client: Fix not resetting request id

[Luiz Augusto von Dentz]

Hi,

On Mon, Feb 1, 2016 at 4:13 PM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>
> The request id shall be reset to 0 after bt_att_cancel has been called.
> ---
>  src/shared/gatt-client.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c
> index 06ac763..5fd0208 100644
> --- a/src/shared/gatt-client.c
> +++ b/src/shared/gatt-client.c
> @@ -1581,6 +1581,7 @@ static void complete_unregister_notify(void *data)
>          */
>         if (notify_data->att_id) {
>                 bt_att_cancel(notify_data->client->att, notify_data->att_id);
> +               notify_data->att_id = 0;
>                 goto done;
>         }
>
> --
> 2.5.0
>

Pushed.

-- 
Luiz Augusto von Dentz