Hi Archie,
On Mon, Sep 14, 2020 at 8:04 PM Archie Pusaka <[email protected]> wrote:
>
> From: Archie Pusaka <[email protected]>
>
> RemoveDevice on adapter interface used to remove a device, even when
> the device is connected. However, since the introduction of the new
> 30 seconds timeout when setting a device as temporary, RemoveDevice
> doesn't immediately remove a connected device, but only disconnects
> it and waits for the timer to expire before effectively removes it.
>
> This patch removes the device as soon as it gets disconnected,
> provided the disconnection is triggered by a call to RemoveDevice.
> The regular timeout still applies for other cases.
>
> Tested manually by calling RemoveDevice on a connected device,
> and with ChromeOS autotest setup.
>
> Reviewed-by: Daniel Winkler <[email protected]>
> ---
>
> Changes in v3:
> * Rebasing again
>
> Changes in v2:
> * Rebasing to HEAD
>
> src/adapter.c | 2 --
> src/adapter.h | 2 ++
> src/device.c | 11 +++++++++++
> 3 files changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/src/adapter.c b/src/adapter.c
> index df628a7fd..4e27bd74b 100644
> --- a/src/adapter.c
> +++ b/src/adapter.c
> @@ -80,8 +80,6 @@
> #include "adv_monitor.h"
> #include "eir.h"
>
> -#define ADAPTER_INTERFACE "org.bluez.Adapter1"
> -
> #define MODE_OFF 0x00
> #define MODE_CONNECTABLE 0x01
> #define MODE_DISCOVERABLE 0x02
> diff --git a/src/adapter.h b/src/adapter.h
> index c70a7b0da..2f1e4b737 100644
> --- a/src/adapter.h
> +++ b/src/adapter.h
> @@ -29,6 +29,8 @@
> #include <lib/bluetooth.h>
> #include <lib/sdp.h>
>
> +#define ADAPTER_INTERFACE "org.bluez.Adapter1"
> +
> #define MAX_NAME_LENGTH 248
>
> /* Invalid SSP passkey value used to indicate negative replies */
> diff --git a/src/device.c b/src/device.c
> index 8f73ce4d3..3e7784034 100644
> --- a/src/device.c
> +++ b/src/device.c
> @@ -3007,6 +3007,7 @@ void device_remove_connection(struct btd_device *device, uint8_t bdaddr_type)
> {
> struct bearer_state *state = get_state(device, bdaddr_type);
> DBusMessage *reply;
> + bool remove_device = false;
>
> if (!state->connected)
> return;
> @@ -3036,6 +3037,10 @@ void device_remove_connection(struct btd_device *device, uint8_t bdaddr_type)
> while (device->disconnects) {
> DBusMessage *msg = device->disconnects->data;
>
> + if (dbus_message_is_method_call(msg, ADAPTER_INTERFACE,
> + "RemoveDevice"))
> + remove_device = true;
> +
> g_dbus_send_reply(dbus_conn, msg, DBUS_TYPE_INVALID);
> device->disconnects = g_slist_remove(device->disconnects, msg);
> dbus_message_unref(msg);
> @@ -3061,6 +3066,9 @@ void device_remove_connection(struct btd_device *device, uint8_t bdaddr_type)
>
> g_dbus_emit_property_changed(dbus_conn, device->path,
> DEVICE_INTERFACE, "Connected");
> +
> + if (remove_device)
> + btd_adapter_remove_device(device->adapter, device);
> }
>
> guint device_add_disconnect_watch(struct btd_device *device,
> @@ -4482,6 +4490,9 @@ void device_remove(struct btd_device *device, gboolean remove_stored)
> disconnect_all(device);
> }
>
> + if (device->temporary_timer > 0)
> + g_source_remove(device->temporary_timer);
> +
> if (device->store_id > 0) {
> g_source_remove(device->store_id);
> device->store_id = 0;
> --
> 2.28.0.618.gf4bc123cb7-goog
>
Applied, thanks.
--
Luiz Augusto von Dentz
Hi Archie,
On Tue, Sep 15, 2020 at 9:51 AM Luiz Augusto von Dentz
<[email protected]> wrote:
>
> Hi Archie,
>
> On Mon, Sep 14, 2020 at 8:04 PM Archie Pusaka <[email protected]> wrote:
> >
> > From: Archie Pusaka <[email protected]>
> >
> > RemoveDevice on adapter interface used to remove a device, even when
> > the device is connected. However, since the introduction of the new
> > 30 seconds timeout when setting a device as temporary, RemoveDevice
> > doesn't immediately remove a connected device, but only disconnects
> > it and waits for the timer to expire before effectively removes it.
> >
> > This patch removes the device as soon as it gets disconnected,
> > provided the disconnection is triggered by a call to RemoveDevice.
> > The regular timeout still applies for other cases.
> >
> > Tested manually by calling RemoveDevice on a connected device,
> > and with ChromeOS autotest setup.
> >
> > Reviewed-by: Daniel Winkler <[email protected]>
> > ---
> >
> > Changes in v3:
> > * Rebasing again
> >
> > Changes in v2:
> > * Rebasing to HEAD
> >
> > src/adapter.c | 2 --
> > src/adapter.h | 2 ++
> > src/device.c | 11 +++++++++++
> > 3 files changed, 13 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/adapter.c b/src/adapter.c
> > index df628a7fd..4e27bd74b 100644
> > --- a/src/adapter.c
> > +++ b/src/adapter.c
> > @@ -80,8 +80,6 @@
> > #include "adv_monitor.h"
> > #include "eir.h"
> >
> > -#define ADAPTER_INTERFACE "org.bluez.Adapter1"
> > -
> > #define MODE_OFF 0x00
> > #define MODE_CONNECTABLE 0x01
> > #define MODE_DISCOVERABLE 0x02
> > diff --git a/src/adapter.h b/src/adapter.h
> > index c70a7b0da..2f1e4b737 100644
> > --- a/src/adapter.h
> > +++ b/src/adapter.h
> > @@ -29,6 +29,8 @@
> > #include <lib/bluetooth.h>
> > #include <lib/sdp.h>
> >
> > +#define ADAPTER_INTERFACE "org.bluez.Adapter1"
> > +
> > #define MAX_NAME_LENGTH 248
> >
> > /* Invalid SSP passkey value used to indicate negative replies */
> > diff --git a/src/device.c b/src/device.c
> > index 8f73ce4d3..3e7784034 100644
> > --- a/src/device.c
> > +++ b/src/device.c
> > @@ -3007,6 +3007,7 @@ void device_remove_connection(struct btd_device *device, uint8_t bdaddr_type)
> > {
> > struct bearer_state *state = get_state(device, bdaddr_type);
> > DBusMessage *reply;
> > + bool remove_device = false;
> >
> > if (!state->connected)
> > return;
> > @@ -3036,6 +3037,10 @@ void device_remove_connection(struct btd_device *device, uint8_t bdaddr_type)
> > while (device->disconnects) {
> > DBusMessage *msg = device->disconnects->data;
> >
> > + if (dbus_message_is_method_call(msg, ADAPTER_INTERFACE,
> > + "RemoveDevice"))
> > + remove_device = true;
> > +
> > g_dbus_send_reply(dbus_conn, msg, DBUS_TYPE_INVALID);
> > device->disconnects = g_slist_remove(device->disconnects, msg);
> > dbus_message_unref(msg);
> > @@ -3061,6 +3066,9 @@ void device_remove_connection(struct btd_device *device, uint8_t bdaddr_type)
> >
> > g_dbus_emit_property_changed(dbus_conn, device->path,
> > DEVICE_INTERFACE, "Connected");
> > +
> > + if (remove_device)
> > + btd_adapter_remove_device(device->adapter, device);
It looks like there are instances where device_remove_connection is
called that can lead to the following trace:
==4030336== Invalid read of size 8
==4030336== at 0x40B8A1: device_is_authenticating (device.c:6975)
==4030336== by 0x3ABA2F: adapter_remove_connection (adapter.c:7166)
==4030336== by 0x3C2A60: dev_disconnected (adapter.c:8123)
==4030336== by 0x45C6B4: request_complete (mgmt.c:298)
==4030336== by 0x45FF74: can_read_data (mgmt.c:390)
==4030336== by 0x49B28F: watch_callback (io-glib.c:157)
==4030336== by 0x495312F: g_main_context_dispatch (in
/usr/lib64/libglib-2.0.so.0.7000.2)
==4030336== by 0x49A8207: ??? (in /usr/lib64/libglib-2.0.so.0.7000.2)
==4030336== by 0x4952852: g_main_loop_run (in
/usr/lib64/libglib-2.0.so.0.7000.2)
==4030336== by 0x49C814: mainloop_run (mainloop-glib.c:66)
==4030336== by 0x49CD0B: mainloop_run_with_signal (mainloop-notify.c:188)
==4030336== by 0x29B18B: main (main.c:1239)
==4030336== Address 0x771bfe0 is 448 bytes inside a block of size 656 free'd
==4030336== at 0x48440E4: free (vg_replace_malloc.c:872)
==4030336== by 0x4954DAC: g_free (in /usr/lib64/libglib-2.0.so.0.7000.2)
==4030336== by 0x44D166: remove_interface (object.c:660)
==4030336== by 0x44DEDA: g_dbus_unregister_interface (object.c:1394)
==4030336== by 0x3ABA27: adapter_remove_connection (adapter.c:7164)
==4030336== by 0x3C2A60: dev_disconnected (adapter.c:8123)
==4030336== by 0x45C6B4: request_complete (mgmt.c:298)
==4030336== by 0x45FF74: can_read_data (mgmt.c:390)
==4030336== by 0x49B28F: watch_callback (io-glib.c:157)
==4030336== by 0x495312F: g_main_context_dispatch (in
/usr/lib64/libglib-2.0.so.0.7000.2)
==4030336== by 0x49A8207: ??? (in /usr/lib64/libglib-2.0.so.0.7000.2)
==4030336== by 0x4952852: g_main_loop_run (in
/usr/lib64/libglib-2.0.so.0.7000.2)
So it appeared to be unsafe to call btd_adapter_remove_device, btw
this happened when Ive attempted to pair 2 emulator instances
(btvirt).
> > }
> >
> > guint device_add_disconnect_watch(struct btd_device *device,
> > @@ -4482,6 +4490,9 @@ void device_remove(struct btd_device *device, gboolean remove_stored)
> > disconnect_all(device);
> > }
> >
> > + if (device->temporary_timer > 0)
> > + g_source_remove(device->temporary_timer);
> > +
> > if (device->store_id > 0) {
> > g_source_remove(device->store_id);
> > device->store_id = 0;
> > --
> > 2.28.0.618.gf4bc123cb7-goog
> >
>
> Applied, thanks.
>
> --
> Luiz Augusto von Dentz
--
Luiz Augusto von Dentz
Hi Luiz,
On Wed, 9 Feb 2022 at 10:39, Luiz Augusto von Dentz
<[email protected]> wrote:
>
> Hi Archie,
>
> On Tue, Sep 15, 2020 at 9:51 AM Luiz Augusto von Dentz
> <[email protected]> wrote:
> >
> > Hi Archie,
> >
> > On Mon, Sep 14, 2020 at 8:04 PM Archie Pusaka <[email protected]> wrote:
> > >
> > > From: Archie Pusaka <[email protected]>
> > >
> > > RemoveDevice on adapter interface used to remove a device, even when
> > > the device is connected. However, since the introduction of the new
> > > 30 seconds timeout when setting a device as temporary, RemoveDevice
> > > doesn't immediately remove a connected device, but only disconnects
> > > it and waits for the timer to expire before effectively removes it.
> > >
> > > This patch removes the device as soon as it gets disconnected,
> > > provided the disconnection is triggered by a call to RemoveDevice.
> > > The regular timeout still applies for other cases.
> > >
> > > Tested manually by calling RemoveDevice on a connected device,
> > > and with ChromeOS autotest setup.
> > >
> > > Reviewed-by: Daniel Winkler <[email protected]>
> > > ---
> > >
> > > Changes in v3:
> > > * Rebasing again
> > >
> > > Changes in v2:
> > > * Rebasing to HEAD
> > >
> > > src/adapter.c | 2 --
> > > src/adapter.h | 2 ++
> > > src/device.c | 11 +++++++++++
> > > 3 files changed, 13 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/src/adapter.c b/src/adapter.c
> > > index df628a7fd..4e27bd74b 100644
> > > --- a/src/adapter.c
> > > +++ b/src/adapter.c
> > > @@ -80,8 +80,6 @@
> > > #include "adv_monitor.h"
> > > #include "eir.h"
> > >
> > > -#define ADAPTER_INTERFACE "org.bluez.Adapter1"
> > > -
> > > #define MODE_OFF 0x00
> > > #define MODE_CONNECTABLE 0x01
> > > #define MODE_DISCOVERABLE 0x02
> > > diff --git a/src/adapter.h b/src/adapter.h
> > > index c70a7b0da..2f1e4b737 100644
> > > --- a/src/adapter.h
> > > +++ b/src/adapter.h
> > > @@ -29,6 +29,8 @@
> > > #include <lib/bluetooth.h>
> > > #include <lib/sdp.h>
> > >
> > > +#define ADAPTER_INTERFACE "org.bluez.Adapter1"
> > > +
> > > #define MAX_NAME_LENGTH 248
> > >
> > > /* Invalid SSP passkey value used to indicate negative replies */
> > > diff --git a/src/device.c b/src/device.c
> > > index 8f73ce4d3..3e7784034 100644
> > > --- a/src/device.c
> > > +++ b/src/device.c
> > > @@ -3007,6 +3007,7 @@ void device_remove_connection(struct btd_device *device, uint8_t bdaddr_type)
> > > {
> > > struct bearer_state *state = get_state(device, bdaddr_type);
> > > DBusMessage *reply;
> > > + bool remove_device = false;
> > >
> > > if (!state->connected)
> > > return;
> > > @@ -3036,6 +3037,10 @@ void device_remove_connection(struct btd_device *device, uint8_t bdaddr_type)
> > > while (device->disconnects) {
> > > DBusMessage *msg = device->disconnects->data;
> > >
> > > + if (dbus_message_is_method_call(msg, ADAPTER_INTERFACE,
> > > + "RemoveDevice"))
> > > + remove_device = true;
> > > +
> > > g_dbus_send_reply(dbus_conn, msg, DBUS_TYPE_INVALID);
> > > device->disconnects = g_slist_remove(device->disconnects, msg);
> > > dbus_message_unref(msg);
> > > @@ -3061,6 +3066,9 @@ void device_remove_connection(struct btd_device *device, uint8_t bdaddr_type)
> > >
> > > g_dbus_emit_property_changed(dbus_conn, device->path,
> > > DEVICE_INTERFACE, "Connected");
> > > +
> > > + if (remove_device)
> > > + btd_adapter_remove_device(device->adapter, device);
>
> It looks like there are instances where device_remove_connection is
> called that can lead to the following trace:
>
> ==4030336== Invalid read of size 8
> ==4030336== at 0x40B8A1: device_is_authenticating (device.c:6975)
> ==4030336== by 0x3ABA2F: adapter_remove_connection (adapter.c:7166)
> ==4030336== by 0x3C2A60: dev_disconnected (adapter.c:8123)
> ==4030336== by 0x45C6B4: request_complete (mgmt.c:298)
> ==4030336== by 0x45FF74: can_read_data (mgmt.c:390)
> ==4030336== by 0x49B28F: watch_callback (io-glib.c:157)
> ==4030336== by 0x495312F: g_main_context_dispatch (in
> /usr/lib64/libglib-2.0.so.0.7000.2)
> ==4030336== by 0x49A8207: ??? (in /usr/lib64/libglib-2.0.so.0.7000.2)
> ==4030336== by 0x4952852: g_main_loop_run (in
> /usr/lib64/libglib-2.0.so.0.7000.2)
> ==4030336== by 0x49C814: mainloop_run (mainloop-glib.c:66)
> ==4030336== by 0x49CD0B: mainloop_run_with_signal (mainloop-notify.c:188)
> ==4030336== by 0x29B18B: main (main.c:1239)
> ==4030336== Address 0x771bfe0 is 448 bytes inside a block of size 656 free'd
> ==4030336== at 0x48440E4: free (vg_replace_malloc.c:872)
> ==4030336== by 0x4954DAC: g_free (in /usr/lib64/libglib-2.0.so.0.7000.2)
> ==4030336== by 0x44D166: remove_interface (object.c:660)
> ==4030336== by 0x44DEDA: g_dbus_unregister_interface (object.c:1394)
> ==4030336== by 0x3ABA27: adapter_remove_connection (adapter.c:7164)
> ==4030336== by 0x3C2A60: dev_disconnected (adapter.c:8123)
> ==4030336== by 0x45C6B4: request_complete (mgmt.c:298)
> ==4030336== by 0x45FF74: can_read_data (mgmt.c:390)
> ==4030336== by 0x49B28F: watch_callback (io-glib.c:157)
> ==4030336== by 0x495312F: g_main_context_dispatch (in
> /usr/lib64/libglib-2.0.so.0.7000.2)
> ==4030336== by 0x49A8207: ??? (in /usr/lib64/libglib-2.0.so.0.7000.2)
> ==4030336== by 0x4952852: g_main_loop_run (in
> /usr/lib64/libglib-2.0.so.0.7000.2)
>
> So it appeared to be unsafe to call btd_adapter_remove_device, btw
> this happened when Ive attempted to pair 2 emulator instances
> (btvirt).
Does this happen after calling Adapter1.RemoveDevice? I suppose if
that's true then adapter_remove_connection shouldn't have been called
since the device should have been removed at this point.
Perhaps I misunderstood your message?
>
> > > }
> > >
> > > guint device_add_disconnect_watch(struct btd_device *device,
> > > @@ -4482,6 +4490,9 @@ void device_remove(struct btd_device *device, gboolean remove_stored)
> > > disconnect_all(device);
> > > }
> > >
> > > + if (device->temporary_timer > 0)
> > > + g_source_remove(device->temporary_timer);
> > > +
> > > if (device->store_id > 0) {
> > > g_source_remove(device->store_id);
> > > device->store_id = 0;
> > > --
> > > 2.28.0.618.gf4bc123cb7-goog
> > >
> >
> > Applied, thanks.
> >
> > --
> > Luiz Augusto von Dentz
>
>
>
> --
> Luiz Augusto von Dentz
Thanks,
Archie
Hi Archie,
On Tue, Feb 8, 2022 at 10:37 PM Archie Pusaka <[email protected]> wrote:
>
> Hi Luiz,
>
> On Wed, 9 Feb 2022 at 10:39, Luiz Augusto von Dentz
> <[email protected]> wrote:
> >
> > Hi Archie,
> >
> > On Tue, Sep 15, 2020 at 9:51 AM Luiz Augusto von Dentz
> > <[email protected]> wrote:
> > >
> > > Hi Archie,
> > >
> > > On Mon, Sep 14, 2020 at 8:04 PM Archie Pusaka <[email protected]> wrote:
> > > >
> > > > From: Archie Pusaka <[email protected]>
> > > >
> > > > RemoveDevice on adapter interface used to remove a device, even when
> > > > the device is connected. However, since the introduction of the new
> > > > 30 seconds timeout when setting a device as temporary, RemoveDevice
> > > > doesn't immediately remove a connected device, but only disconnects
> > > > it and waits for the timer to expire before effectively removes it.
> > > >
> > > > This patch removes the device as soon as it gets disconnected,
> > > > provided the disconnection is triggered by a call to RemoveDevice.
> > > > The regular timeout still applies for other cases.
> > > >
> > > > Tested manually by calling RemoveDevice on a connected device,
> > > > and with ChromeOS autotest setup.
> > > >
> > > > Reviewed-by: Daniel Winkler <[email protected]>
> > > > ---
> > > >
> > > > Changes in v3:
> > > > * Rebasing again
> > > >
> > > > Changes in v2:
> > > > * Rebasing to HEAD
> > > >
> > > > src/adapter.c | 2 --
> > > > src/adapter.h | 2 ++
> > > > src/device.c | 11 +++++++++++
> > > > 3 files changed, 13 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/src/adapter.c b/src/adapter.c
> > > > index df628a7fd..4e27bd74b 100644
> > > > --- a/src/adapter.c
> > > > +++ b/src/adapter.c
> > > > @@ -80,8 +80,6 @@
> > > > #include "adv_monitor.h"
> > > > #include "eir.h"
> > > >
> > > > -#define ADAPTER_INTERFACE "org.bluez.Adapter1"
> > > > -
> > > > #define MODE_OFF 0x00
> > > > #define MODE_CONNECTABLE 0x01
> > > > #define MODE_DISCOVERABLE 0x02
> > > > diff --git a/src/adapter.h b/src/adapter.h
> > > > index c70a7b0da..2f1e4b737 100644
> > > > --- a/src/adapter.h
> > > > +++ b/src/adapter.h
> > > > @@ -29,6 +29,8 @@
> > > > #include <lib/bluetooth.h>
> > > > #include <lib/sdp.h>
> > > >
> > > > +#define ADAPTER_INTERFACE "org.bluez.Adapter1"
> > > > +
> > > > #define MAX_NAME_LENGTH 248
> > > >
> > > > /* Invalid SSP passkey value used to indicate negative replies */
> > > > diff --git a/src/device.c b/src/device.c
> > > > index 8f73ce4d3..3e7784034 100644
> > > > --- a/src/device.c
> > > > +++ b/src/device.c
> > > > @@ -3007,6 +3007,7 @@ void device_remove_connection(struct btd_device *device, uint8_t bdaddr_type)
> > > > {
> > > > struct bearer_state *state = get_state(device, bdaddr_type);
> > > > DBusMessage *reply;
> > > > + bool remove_device = false;
> > > >
> > > > if (!state->connected)
> > > > return;
> > > > @@ -3036,6 +3037,10 @@ void device_remove_connection(struct btd_device *device, uint8_t bdaddr_type)
> > > > while (device->disconnects) {
> > > > DBusMessage *msg = device->disconnects->data;
> > > >
> > > > + if (dbus_message_is_method_call(msg, ADAPTER_INTERFACE,
> > > > + "RemoveDevice"))
> > > > + remove_device = true;
> > > > +
> > > > g_dbus_send_reply(dbus_conn, msg, DBUS_TYPE_INVALID);
> > > > device->disconnects = g_slist_remove(device->disconnects, msg);
> > > > dbus_message_unref(msg);
> > > > @@ -3061,6 +3066,9 @@ void device_remove_connection(struct btd_device *device, uint8_t bdaddr_type)
> > > >
> > > > g_dbus_emit_property_changed(dbus_conn, device->path,
> > > > DEVICE_INTERFACE, "Connected");
> > > > +
> > > > + if (remove_device)
> > > > + btd_adapter_remove_device(device->adapter, device);
> >
> > It looks like there are instances where device_remove_connection is
> > called that can lead to the following trace:
> >
> > ==4030336== Invalid read of size 8
> > ==4030336== at 0x40B8A1: device_is_authenticating (device.c:6975)
> > ==4030336== by 0x3ABA2F: adapter_remove_connection (adapter.c:7166)
> > ==4030336== by 0x3C2A60: dev_disconnected (adapter.c:8123)
> > ==4030336== by 0x45C6B4: request_complete (mgmt.c:298)
> > ==4030336== by 0x45FF74: can_read_data (mgmt.c:390)
> > ==4030336== by 0x49B28F: watch_callback (io-glib.c:157)
> > ==4030336== by 0x495312F: g_main_context_dispatch (in
> > /usr/lib64/libglib-2.0.so.0.7000.2)
> > ==4030336== by 0x49A8207: ??? (in /usr/lib64/libglib-2.0.so.0.7000.2)
> > ==4030336== by 0x4952852: g_main_loop_run (in
> > /usr/lib64/libglib-2.0.so.0.7000.2)
> > ==4030336== by 0x49C814: mainloop_run (mainloop-glib.c:66)
> > ==4030336== by 0x49CD0B: mainloop_run_with_signal (mainloop-notify.c:188)
> > ==4030336== by 0x29B18B: main (main.c:1239)
> > ==4030336== Address 0x771bfe0 is 448 bytes inside a block of size 656 free'd
> > ==4030336== at 0x48440E4: free (vg_replace_malloc.c:872)
> > ==4030336== by 0x4954DAC: g_free (in /usr/lib64/libglib-2.0.so.0.7000.2)
> > ==4030336== by 0x44D166: remove_interface (object.c:660)
> > ==4030336== by 0x44DEDA: g_dbus_unregister_interface (object.c:1394)
> > ==4030336== by 0x3ABA27: adapter_remove_connection (adapter.c:7164)
> > ==4030336== by 0x3C2A60: dev_disconnected (adapter.c:8123)
> > ==4030336== by 0x45C6B4: request_complete (mgmt.c:298)
> > ==4030336== by 0x45FF74: can_read_data (mgmt.c:390)
> > ==4030336== by 0x49B28F: watch_callback (io-glib.c:157)
> > ==4030336== by 0x495312F: g_main_context_dispatch (in
> > /usr/lib64/libglib-2.0.so.0.7000.2)
> > ==4030336== by 0x49A8207: ??? (in /usr/lib64/libglib-2.0.so.0.7000.2)
> > ==4030336== by 0x4952852: g_main_loop_run (in
> > /usr/lib64/libglib-2.0.so.0.7000.2)
> >
> > So it appeared to be unsafe to call btd_adapter_remove_device, btw
> > this happened when Ive attempted to pair 2 emulator instances
> > (btvirt).
>
> Does this happen after calling Adapter1.RemoveDevice? I suppose if
> that's true then adapter_remove_connection shouldn't have been called
> since the device should have been removed at this point.
> Perhaps I misunderstood your message?
Looks like there are more people with the same problem:
https://github.com/bluez/bluez/issues/290
> > > > }
> > > >
> > > > guint device_add_disconnect_watch(struct btd_device *device,
> > > > @@ -4482,6 +4490,9 @@ void device_remove(struct btd_device *device, gboolean remove_stored)
> > > > disconnect_all(device);
> > > > }
> > > >
> > > > + if (device->temporary_timer > 0)
> > > > + g_source_remove(device->temporary_timer);
> > > > +
> > > > if (device->store_id > 0) {
> > > > g_source_remove(device->store_id);
> > > > device->store_id = 0;
> > > > --
> > > > 2.28.0.618.gf4bc123cb7-goog
> > > >
> > >
> > > Applied, thanks.
> > >
> > > --
> > > Luiz Augusto von Dentz
> >
> >
> >
> > --
> > Luiz Augusto von Dentz
>
> Thanks,
> Archie
--
Luiz Augusto von Dentz
Hi Luiz,
On Fri, 11 Feb 2022 at 05:31, Luiz Augusto von Dentz
<[email protected]> wrote:
>
> Hi Archie,
>
> On Tue, Feb 8, 2022 at 10:37 PM Archie Pusaka <[email protected]> wrote:
> >
> > Hi Luiz,
> >
> > On Wed, 9 Feb 2022 at 10:39, Luiz Augusto von Dentz
> > <[email protected]> wrote:
> > >
> > > Hi Archie,
> > >
> > > On Tue, Sep 15, 2020 at 9:51 AM Luiz Augusto von Dentz
> > > <[email protected]> wrote:
> > > >
> > > > Hi Archie,
> > > >
> > > > On Mon, Sep 14, 2020 at 8:04 PM Archie Pusaka <[email protected]> wrote:
> > > > >
> > > > > From: Archie Pusaka <[email protected]>
> > > > >
> > > > > RemoveDevice on adapter interface used to remove a device, even when
> > > > > the device is connected. However, since the introduction of the new
> > > > > 30 seconds timeout when setting a device as temporary, RemoveDevice
> > > > > doesn't immediately remove a connected device, but only disconnects
> > > > > it and waits for the timer to expire before effectively removes it.
> > > > >
> > > > > This patch removes the device as soon as it gets disconnected,
> > > > > provided the disconnection is triggered by a call to RemoveDevice.
> > > > > The regular timeout still applies for other cases.
> > > > >
> > > > > Tested manually by calling RemoveDevice on a connected device,
> > > > > and with ChromeOS autotest setup.
> > > > >
> > > > > Reviewed-by: Daniel Winkler <[email protected]>
> > > > > ---
> > > > >
> > > > > Changes in v3:
> > > > > * Rebasing again
> > > > >
> > > > > Changes in v2:
> > > > > * Rebasing to HEAD
> > > > >
> > > > > src/adapter.c | 2 --
> > > > > src/adapter.h | 2 ++
> > > > > src/device.c | 11 +++++++++++
> > > > > 3 files changed, 13 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/src/adapter.c b/src/adapter.c
> > > > > index df628a7fd..4e27bd74b 100644
> > > > > --- a/src/adapter.c
> > > > > +++ b/src/adapter.c
> > > > > @@ -80,8 +80,6 @@
> > > > > #include "adv_monitor.h"
> > > > > #include "eir.h"
> > > > >
> > > > > -#define ADAPTER_INTERFACE "org.bluez.Adapter1"
> > > > > -
> > > > > #define MODE_OFF 0x00
> > > > > #define MODE_CONNECTABLE 0x01
> > > > > #define MODE_DISCOVERABLE 0x02
> > > > > diff --git a/src/adapter.h b/src/adapter.h
> > > > > index c70a7b0da..2f1e4b737 100644
> > > > > --- a/src/adapter.h
> > > > > +++ b/src/adapter.h
> > > > > @@ -29,6 +29,8 @@
> > > > > #include <lib/bluetooth.h>
> > > > > #include <lib/sdp.h>
> > > > >
> > > > > +#define ADAPTER_INTERFACE "org.bluez.Adapter1"
> > > > > +
> > > > > #define MAX_NAME_LENGTH 248
> > > > >
> > > > > /* Invalid SSP passkey value used to indicate negative replies */
> > > > > diff --git a/src/device.c b/src/device.c
> > > > > index 8f73ce4d3..3e7784034 100644
> > > > > --- a/src/device.c
> > > > > +++ b/src/device.c
> > > > > @@ -3007,6 +3007,7 @@ void device_remove_connection(struct btd_device *device, uint8_t bdaddr_type)
> > > > > {
> > > > > struct bearer_state *state = get_state(device, bdaddr_type);
> > > > > DBusMessage *reply;
> > > > > + bool remove_device = false;
> > > > >
> > > > > if (!state->connected)
> > > > > return;
> > > > > @@ -3036,6 +3037,10 @@ void device_remove_connection(struct btd_device *device, uint8_t bdaddr_type)
> > > > > while (device->disconnects) {
> > > > > DBusMessage *msg = device->disconnects->data;
> > > > >
> > > > > + if (dbus_message_is_method_call(msg, ADAPTER_INTERFACE,
> > > > > + "RemoveDevice"))
> > > > > + remove_device = true;
> > > > > +
> > > > > g_dbus_send_reply(dbus_conn, msg, DBUS_TYPE_INVALID);
> > > > > device->disconnects = g_slist_remove(device->disconnects, msg);
> > > > > dbus_message_unref(msg);
> > > > > @@ -3061,6 +3066,9 @@ void device_remove_connection(struct btd_device *device, uint8_t bdaddr_type)
> > > > >
> > > > > g_dbus_emit_property_changed(dbus_conn, device->path,
> > > > > DEVICE_INTERFACE, "Connected");
> > > > > +
> > > > > + if (remove_device)
> > > > > + btd_adapter_remove_device(device->adapter, device);
> > >
> > > It looks like there are instances where device_remove_connection is
> > > called that can lead to the following trace:
> > >
> > > ==4030336== Invalid read of size 8
> > > ==4030336== at 0x40B8A1: device_is_authenticating (device.c:6975)
> > > ==4030336== by 0x3ABA2F: adapter_remove_connection (adapter.c:7166)
> > > ==4030336== by 0x3C2A60: dev_disconnected (adapter.c:8123)
> > > ==4030336== by 0x45C6B4: request_complete (mgmt.c:298)
> > > ==4030336== by 0x45FF74: can_read_data (mgmt.c:390)
> > > ==4030336== by 0x49B28F: watch_callback (io-glib.c:157)
> > > ==4030336== by 0x495312F: g_main_context_dispatch (in
> > > /usr/lib64/libglib-2.0.so.0.7000.2)
> > > ==4030336== by 0x49A8207: ??? (in /usr/lib64/libglib-2.0.so.0.7000.2)
> > > ==4030336== by 0x4952852: g_main_loop_run (in
> > > /usr/lib64/libglib-2.0.so.0.7000.2)
> > > ==4030336== by 0x49C814: mainloop_run (mainloop-glib.c:66)
> > > ==4030336== by 0x49CD0B: mainloop_run_with_signal (mainloop-notify.c:188)
> > > ==4030336== by 0x29B18B: main (main.c:1239)
> > > ==4030336== Address 0x771bfe0 is 448 bytes inside a block of size 656 free'd
> > > ==4030336== at 0x48440E4: free (vg_replace_malloc.c:872)
> > > ==4030336== by 0x4954DAC: g_free (in /usr/lib64/libglib-2.0.so.0.7000.2)
> > > ==4030336== by 0x44D166: remove_interface (object.c:660)
> > > ==4030336== by 0x44DEDA: g_dbus_unregister_interface (object.c:1394)
> > > ==4030336== by 0x3ABA27: adapter_remove_connection (adapter.c:7164)
> > > ==4030336== by 0x3C2A60: dev_disconnected (adapter.c:8123)
> > > ==4030336== by 0x45C6B4: request_complete (mgmt.c:298)
> > > ==4030336== by 0x45FF74: can_read_data (mgmt.c:390)
> > > ==4030336== by 0x49B28F: watch_callback (io-glib.c:157)
> > > ==4030336== by 0x495312F: g_main_context_dispatch (in
> > > /usr/lib64/libglib-2.0.so.0.7000.2)
> > > ==4030336== by 0x49A8207: ??? (in /usr/lib64/libglib-2.0.so.0.7000.2)
> > > ==4030336== by 0x4952852: g_main_loop_run (in
> > > /usr/lib64/libglib-2.0.so.0.7000.2)
> > >
> > > So it appeared to be unsafe to call btd_adapter_remove_device, btw
> > > this happened when Ive attempted to pair 2 emulator instances
> > > (btvirt).
> >
> > Does this happen after calling Adapter1.RemoveDevice? I suppose if
> > that's true then adapter_remove_connection shouldn't have been called
> > since the device should have been removed at this point.
> > Perhaps I misunderstood your message?
>
> Looks like there are more people with the same problem:
>
> https://github.com/bluez/bluez/issues/290
Thanks for letting me know.
I see that you have submitted a fix and Tedd has verified it.
Strangely enough I cannot seem to repro it, nor do I understand how
that even happened.
>
> > > > > }
> > > > >
> > > > > guint device_add_disconnect_watch(struct btd_device *device,
> > > > > @@ -4482,6 +4490,9 @@ void device_remove(struct btd_device *device, gboolean remove_stored)
> > > > > disconnect_all(device);
> > > > > }
> > > > >
> > > > > + if (device->temporary_timer > 0)
> > > > > + g_source_remove(device->temporary_timer);
> > > > > +
> > > > > if (device->store_id > 0) {
> > > > > g_source_remove(device->store_id);
> > > > > device->store_id = 0;
> > > > > --
> > > > > 2.28.0.618.gf4bc123cb7-goog
> > > > >
> > > >
> > > > Applied, thanks.
> > > >
> > > > --
> > > > Luiz Augusto von Dentz
> > >
> > >
> > >
> > > --
> > > Luiz Augusto von Dentz
> >
> > Thanks,
> > Archie
>
>
>
> --
> Luiz Augusto von Dentz
Regards,
Archie