| Message ID | 20200821061643.16278-1-sonnysasaka@chromium.org (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Luiz Von Dentz |
| Headers | show |
| Series | [BlueZ,1/2] gatt: StartNotify is not allowed when device is disconnecting | expand |
Dear BlueZ maintainers, Friendly ping to review this patch. Thanks! On Thu, Aug 20, 2020 at 11:17 PM Sonny Sasaka <sonnysasaka@chromium.org> wrote: > > From: Joseph Hwang <josephsih@chromium.org> > > This patch fixed a bluetoothd crash in register_notify_cb(). The > crash is incurred by an exception that under some situation, a > characteristic may be freed when register_notify_cb() is invoked. > > When a device is disconnecting, the device interface would hold valid > for a while until the disconnection procedure between the client and > the server is completed. If another process happens to request to start > notification of a characteristic on the disconnecting device, it may > incur a problem. In this case, the client would still send the > StartNotify request since the characteristic object is still valid. > However, the characteristic may be freed soon and become invalid > when the corresponding callback function is invoked later. This > leads to the bluetoothd crash due to the segmentation fault. > > To handle the exception, if another process requests to start > notification when the device is disconnecting, it should reject the > request. > > Tested on Chrome OS that this patch fixes bluetoothd crash in > register_notify_cb(). > > --- > src/gatt-client.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/src/gatt-client.c b/src/gatt-client.c > index 20c3fbec2..c706307c7 100644 > --- a/src/gatt-client.c > +++ b/src/gatt-client.c > @@ -1545,6 +1545,12 @@ static DBusMessage *characteristic_start_notify(DBusConnection *conn, > const char *sender = dbus_message_get_sender(msg); > struct async_dbus_op *op; > struct notify_client *client; > + struct btd_device *device = chrc->service->client->device; > + > + if (device_is_disconnecting(device)) { > + error("Device is disconnecting. StartNotify is not allowed."); > + return btd_error_not_connected(msg); > + } > > if (chrc->notify_io) > return btd_error_not_permitted(msg, "Notify acquired"); > -- > 2.26.2 >
Hi Sonny, On Wed, Sep 16, 2020 at 3:40 PM Sonny Sasaka <sonnysasaka@chromium.org> wrote: > > Dear BlueZ maintainers, > > Friendly ping to review this patch. Thanks! > > > On Thu, Aug 20, 2020 at 11:17 PM Sonny Sasaka <sonnysasaka@chromium.org> wrote: > > > > From: Joseph Hwang <josephsih@chromium.org> > > > > This patch fixed a bluetoothd crash in register_notify_cb(). The > > crash is incurred by an exception that under some situation, a > > characteristic may be freed when register_notify_cb() is invoked. > > > > When a device is disconnecting, the device interface would hold valid > > for a while until the disconnection procedure between the client and > > the server is completed. If another process happens to request to start > > notification of a characteristic on the disconnecting device, it may > > incur a problem. In this case, the client would still send the > > StartNotify request since the characteristic object is still valid. > > However, the characteristic may be freed soon and become invalid > > when the corresponding callback function is invoked later. This > > leads to the bluetoothd crash due to the segmentation fault. > > > > To handle the exception, if another process requests to start > > notification when the device is disconnecting, it should reject the > > request. > > > > Tested on Chrome OS that this patch fixes bluetoothd crash in > > register_notify_cb(). > > > > --- > > src/gatt-client.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/src/gatt-client.c b/src/gatt-client.c > > index 20c3fbec2..c706307c7 100644 > > --- a/src/gatt-client.c > > +++ b/src/gatt-client.c > > @@ -1545,6 +1545,12 @@ static DBusMessage *characteristic_start_notify(DBusConnection *conn, > > const char *sender = dbus_message_get_sender(msg); > > struct async_dbus_op *op; > > struct notify_client *client; > > + struct btd_device *device = chrc->service->client->device; > > + > > + if (device_is_disconnecting(device)) { > > + error("Device is disconnecting. StartNotify is not allowed."); > > + return btd_error_not_connected(msg); > > + } > > > > if (chrc->notify_io) > > return btd_error_not_permitted(msg, "Notify acquired"); > > -- > > 2.26.2 > > Applied, thanks.
diff --git a/src/gatt-client.c b/src/gatt-client.c index 20c3fbec2..c706307c7 100644 --- a/src/gatt-client.c +++ b/src/gatt-client.c @@ -1545,6 +1545,12 @@ static DBusMessage *characteristic_start_notify(DBusConnection *conn, const char *sender = dbus_message_get_sender(msg); struct async_dbus_op *op; struct notify_client *client; + struct btd_device *device = chrc->service->client->device; + + if (device_is_disconnecting(device)) { + error("Device is disconnecting. StartNotify is not allowed."); + return btd_error_not_connected(msg); + } if (chrc->notify_io) return btd_error_not_permitted(msg, "Notify acquired");
From: Joseph Hwang <josephsih@chromium.org> This patch fixed a bluetoothd crash in register_notify_cb(). The crash is incurred by an exception that under some situation, a characteristic may be freed when register_notify_cb() is invoked. When a device is disconnecting, the device interface would hold valid for a while until the disconnection procedure between the client and the server is completed. If another process happens to request to start notification of a characteristic on the disconnecting device, it may incur a problem. In this case, the client would still send the StartNotify request since the characteristic object is still valid. However, the characteristic may be freed soon and become invalid when the corresponding callback function is invoked later. This leads to the bluetoothd crash due to the segmentation fault. To handle the exception, if another process requests to start notification when the device is disconnecting, it should reject the request. Tested on Chrome OS that this patch fixes bluetoothd crash in register_notify_cb(). --- src/gatt-client.c | 6 ++++++ 1 file changed, 6 insertions(+)