[PATCH BlueZ] gatt: Fix failure of repeated AcquireNotify calls

[PATCH BlueZ] gatt: Fix failure of repeated AcquireNotify calls

[Rob Spanton]

This patch fixes a problem that can be encountered if a DBUS client
performs the following steps:

 1) Calls AcquireNotify on a characteristic
 2) Closes the fd produced by AcquireNotify
 3) Immediately calls AcquireNotify again on the same characteristic
 4) Disconnects DBUS client (does not have to be immediately)
 5) Reconnects DBUS client and call AcquireNotify

If these steps are followed, then the third call to AcquireNotify
will often be responded to with an error message stating "Notify
acquired".  Furthermore, the second call to AcquireNotify will not be
provided with an fd.

It turns out that the following was happening:  Closing the fd causes
bluez to disable notifications on that characteristic by writing to
the CCC.  If the second call to AcquireNotify is made before that CCC
write has completed, then a new write to the CCC to re-enable
notifications is enqueued.  Once that first write has completed, the
second write is then taken from the queue and started in
`disable_ccc_callback()`.  Unfortunately `disable_ccc_callback()` was
not actually using the data from the queue, but was instead just
re-using the data that it had been passed (`notify_data` instead of
`next_data`).

This meant that the write to the CCC to enable notifications would
happen, but the callback that needed to be made to the code that was
waiting for the enqueued operation to complete would never happen.  In
this AcquireNotify case, the register_notify_io_cb() function would
not be called, resulting in no socket creation and no response to the
second AcquireNotify call.  Instead it would leave some state
hanging around on bluez's representation of the characteristic, and so
subsequent calls to AcquireNotify by any DBUS client would fail with
the aforementioned error.

The fix is simple here -- make `disable_ccc_callback()` pass the
correct data through.
---
 src/shared/gatt-client.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c
index 858209c24..2c104097e 100644
--- a/src/shared/gatt-client.c
+++ b/src/shared/gatt-client.c
@@ -1947,7 +1947,7 @@ static void disable_ccc_callback(uint8_t opcode, const void *pdu,
 	 */
 	while (1) {
 		next_data = queue_pop_head(notify_data->chrc->reg_notify_queue);
-		if (!next_data || notify_data_write_ccc(notify_data, true,
+		if (!next_data || notify_data_write_ccc(next_data, true,
 							enable_ccc_callback))
 			return;
 	}
-- 
2.21.0

Re: [PATCH BlueZ] gatt: Fix failure of repeated AcquireNotify calls

[Luiz Augusto von Dentz]

Hi Rob,

On Tue, Jul 9, 2019 at 8:09 PM Rob Spanton <rob@adventurousmachines.com> wrote:
>
> This patch fixes a problem that can be encountered if a DBUS client
> performs the following steps:
>
>  1) Calls AcquireNotify on a characteristic
>  2) Closes the fd produced by AcquireNotify
>  3) Immediately calls AcquireNotify again on the same characteristic
>  4) Disconnects DBUS client (does not have to be immediately)
>  5) Reconnects DBUS client and call AcquireNotify
>
> If these steps are followed, then the third call to AcquireNotify
> will often be responded to with an error message stating "Notify
> acquired".  Furthermore, the second call to AcquireNotify will not be
> provided with an fd.
>
> It turns out that the following was happening:  Closing the fd causes
> bluez to disable notifications on that characteristic by writing to
> the CCC.  If the second call to AcquireNotify is made before that CCC
> write has completed, then a new write to the CCC to re-enable
> notifications is enqueued.  Once that first write has completed, the
> second write is then taken from the queue and started in
> `disable_ccc_callback()`.  Unfortunately `disable_ccc_callback()` was
> not actually using the data from the queue, but was instead just
> re-using the data that it had been passed (`notify_data` instead of
> `next_data`).
>
> This meant that the write to the CCC to enable notifications would
> happen, but the callback that needed to be made to the code that was
> waiting for the enqueued operation to complete would never happen.  In
> this AcquireNotify case, the register_notify_io_cb() function would
> not be called, resulting in no socket creation and no response to the
> second AcquireNotify call.  Instead it would leave some state
> hanging around on bluez's representation of the characteristic, and so
> subsequent calls to AcquireNotify by any DBUS client would fail with
> the aforementioned error.
>
> The fix is simple here -- make `disable_ccc_callback()` pass the
> correct data through.
> ---
>  src/shared/gatt-client.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c
> index 858209c24..2c104097e 100644
> --- a/src/shared/gatt-client.c
> +++ b/src/shared/gatt-client.c
> @@ -1947,7 +1947,7 @@ static void disable_ccc_callback(uint8_t opcode, const void *pdu,
>          */
>         while (1) {
>                 next_data = queue_pop_head(notify_data->chrc->reg_notify_queue);
> -               if (!next_data || notify_data_write_ccc(notify_data, true,
> +               if (!next_data || notify_data_write_ccc(next_data, true,
>                                                         enable_ccc_callback))
>                         return;
>         }
> --
> 2.21.0
>

Applied, thanks.

-- 
Luiz Augusto von Dentz