2020-08-21 07:45:47

by Sonny Sasaka

Subject: [PATCH BlueZ] device: Fix race condition between device connection and disconnection

When Connect() is called and waiting for return, dev_disconnected may be
called due to MGMT_EV_DEVICE_DISCONNECTED event from kernel. In that
case reply to client that the connection failed otherwise the dbus
method will timeout because bluetoothd never replies.

Tested with simulation of a lot of Connect() to bluetooth devices and
check that error is returned from bluetoothd rather than dbus timeout
when this race condition happens.

Reviewed-by: Miao-chen Chou <[email protected]>

---
src/device.c | 12 ++++++++++++
1 file changed, 12 insertions(+)

diff --git a/src/device.c b/src/device.c
index 7b7808405..55e7b4042 100644
--- a/src/device.c
+++ b/src/device.c
@@ -3006,6 +3006,7 @@ void device_add_connection(struct btd_device *dev, uint8_t bdaddr_type)
void device_remove_connection(struct btd_device *device, uint8_t bdaddr_type)
{
struct bearer_state *state = get_state(device, bdaddr_type);
+ DBusMessage *reply;

if (!state->connected)
return;
@@ -3020,6 +3021,17 @@ void device_remove_connection(struct btd_device *device, uint8_t bdaddr_type)
device->disconn_timer = 0;
}

+ // This could be executed while the client is waiting for Connect() but
+ // att_connect_cb has not been invoked.
+ // In that case reply the client that the connection failed.
+ if (device->connect) {
+ DBG("connection removed while Connect() is waiting reply");
+ reply = btd_error_failed(device->connect, "Disconnected early");
+ g_dbus_send_message(dbus_conn, reply);
+ dbus_message_unref(device->connect);
+ device->connect = NULL;
+ }
+
while (device->disconnects) {
DBusMessage *msg = device->disconnects->data;

--
2.26.2

2020-08-21 17:37:37

by Luiz Augusto von Dentz

Subject: Re: [PATCH BlueZ] device: Fix race condition between device connection and disconnection

Hi Sonny,

On Fri, Aug 21, 2020 at 12:47 AM Sonny Sasaka <[email protected]> wrote:
>
> When Connect() is called and waiting for return, dev_disconnected may be
> called due to MGMT_EV_DEVICE_DISCONNECTED event from kernel. In that
> case reply to client that the connection failed otherwise the dbus
> method will timeout because bluetoothd never replies.
>
> Tested with simulation of a lot of Connect() to bluetooth devices and
> check that error is returned from bluetoothd rather than dbus timeout
> when this race condition happens.
>
> Reviewed-by: Miao-chen Chou <[email protected]>
>
> ---
> src/device.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/src/device.c b/src/device.c
> index 7b7808405..55e7b4042 100644
> --- a/src/device.c
> +++ b/src/device.c
> @@ -3006,6 +3006,7 @@ void device_add_connection(struct btd_device *dev, uint8_t bdaddr_type)
> void device_remove_connection(struct btd_device *device, uint8_t bdaddr_type)
> {
> struct bearer_state *state = get_state(device, bdaddr_type);
> + DBusMessage *reply;
>
> if (!state->connected)
> return;
> @@ -3020,6 +3021,17 @@ void device_remove_connection(struct btd_device *device, uint8_t bdaddr_type)
> device->disconn_timer = 0;
> }
>
> + // This could be executed while the client is waiting for Connect() but
> + // att_connect_cb has not been invoked.
> + // In that case reply the client that the connection failed.

Please use C style comment /* */

> + if (device->connect) {
> + DBG("connection removed while Connect() is waiting reply");
> + reply = btd_error_failed(device->connect, "Disconnected early");
> + g_dbus_send_message(dbus_conn, reply);
> + dbus_message_unref(device->connect);
> + device->connect = NULL;
> + }
> +
> while (device->disconnects) {
> DBusMessage *msg = device->disconnects->data;
>
> --
> 2.26.2
>

Otherwise it looks good.

--
Luiz Augusto von Dentz

2020-08-21 18:02:12

by Sonny Sasaka

Subject: Re: [PATCH BlueZ] device: Fix race condition between device connection and disconnection

Hi Luiz,

Thanks for the feedback. I have sent a v2 fixing the comment style.

On Fri, Aug 21, 2020 at 10:36 AM Luiz Augusto von Dentz
<[email protected]> wrote:
>
> Hi Sonny,
>
> On Fri, Aug 21, 2020 at 12:47 AM Sonny Sasaka <[email protected]> wrote:
> >
> > When Connect() is called and waiting for return, dev_disconnected may be
> > called due to MGMT_EV_DEVICE_DISCONNECTED event from kernel. In that
> > case reply to client that the connection failed otherwise the dbus
> > method will timeout because bluetoothd never replies.
> >
> > Tested with simulation of a lot of Connect() to bluetooth devices and
> > check that error is returned from bluetoothd rather than dbus timeout
> > when this race condition happens.
> >
> > Reviewed-by: Miao-chen Chou <[email protected]>
> >
> > ---
> > src/device.c | 12 ++++++++++++
> > 1 file changed, 12 insertions(+)
> >
> > diff --git a/src/device.c b/src/device.c
> > index 7b7808405..55e7b4042 100644
> > --- a/src/device.c
> > +++ b/src/device.c
> > @@ -3006,6 +3006,7 @@ void device_add_connection(struct btd_device *dev, uint8_t bdaddr_type)
> > void device_remove_connection(struct btd_device *device, uint8_t bdaddr_type)
> > {
> > struct bearer_state *state = get_state(device, bdaddr_type);
> > + DBusMessage *reply;
> >
> > if (!state->connected)
> > return;
> > @@ -3020,6 +3021,17 @@ void device_remove_connection(struct btd_device *device, uint8_t bdaddr_type)
> > device->disconn_timer = 0;
> > }
> >
> > + // This could be executed while the client is waiting for Connect() but
> > + // att_connect_cb has not been invoked.
> > + // In that case reply the client that the connection failed.
>
> Please use C style comment /* */
>
> > + if (device->connect) {
> > + DBG("connection removed while Connect() is waiting reply");
> > + reply = btd_error_failed(device->connect, "Disconnected early");
> > + g_dbus_send_message(dbus_conn, reply);
> > + dbus_message_unref(device->connect);
> > + device->connect = NULL;
> > + }
> > +
> > while (device->disconnects) {
> > DBusMessage *msg = device->disconnects->data;
> >
> > --
> > 2.26.2
> >
>
> Otherwise it looks good.
>
> --
> Luiz Augusto von Dentz