Hi Rob,
On Tue, Jul 9, 2019 at 8:09 PM Rob Spanton <rob@xxxxxxxxxxxxxxxxxxxxxxx> 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
