| Message ID | 20201030160833.BlueZ.v1.1.Ia45f3edc48142d9db0dc4b315c84ab60a149697f@changeid (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [BlueZ,v1] adapter: Fix a crash caused by lingering discovery client pointer | expand |
Hi Miao-chen, On Fri, Oct 30, 2020 at 4:13 PM Miao-chen Chou <mcchou@chromium.org> wrote: > > This cleans up the lingering pointer, adapter->client, during powering > off the adapter. The crash occurs when a D-Bus client set Powered > property to false and immediately calls StopDiscovery() when there is > ongoing discovery. As a part of powering off the adapter, > adapter->discovery_list gets cleared, and given that adapter->client > refers to one of the clients in adapter->discovery_list, adapter->client > should be cleared along with it. > > Reviewed-by: Alain Michaud <alainm@chromium.org> > Reviewed-by: Sonny Sasaka <sonnysasaka@chromium.org> > --- > > src/adapter.c | 17 ++++++++++++++++- > 1 file changed, 16 insertions(+), 1 deletion(-) > > diff --git a/src/adapter.c b/src/adapter.c > index c0053000a..74bfb0448 100644 > --- a/src/adapter.c > +++ b/src/adapter.c > @@ -1507,8 +1507,10 @@ static void discovery_free(void *user_data) > client->discovery_filter = NULL; > } > > - if (client->msg) > + if (client->msg) { > dbus_message_unref(client->msg); > + client->msg = NULL; > + } This shouldn't make any difference as the whole list is freed and so is the client. > > g_free(client->owner); > g_free(client); > @@ -5301,6 +5303,19 @@ static void free_service_auth(gpointer data, gpointer user_data) > > static void remove_discovery_list(struct btd_adapter *adapter) > { > + DBusMessage *msg; > + > + if (adapter->client) { > + msg = adapter->client->msg; > + if (msg) { > + g_dbus_send_message(dbus_conn, btd_error_busy(msg)); > + dbus_message_unref(msg); > + adapter->client->msg = NULL; > + } > + > + adapter->client = NULL; Shouldn't you call discovery_free as well here? Or perhaps we could move the lines above inside discovery_free so it detects if the adapter->client is pointing to a client that is being freed. > + } > + > g_slist_free_full(adapter->set_filter_list, discovery_free); > adapter->set_filter_list = NULL; > > -- > 2.26.2 >
Hi Luiz, On Fri, Oct 30, 2020 at 4:48 PM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > > Hi Miao-chen, > > On Fri, Oct 30, 2020 at 4:13 PM Miao-chen Chou <mcchou@chromium.org> wrote: > > > > This cleans up the lingering pointer, adapter->client, during powering > > off the adapter. The crash occurs when a D-Bus client set Powered > > property to false and immediately calls StopDiscovery() when there is > > ongoing discovery. As a part of powering off the adapter, > > adapter->discovery_list gets cleared, and given that adapter->client > > refers to one of the clients in adapter->discovery_list, adapter->client > > should be cleared along with it. > > > > Reviewed-by: Alain Michaud <alainm@chromium.org> > > Reviewed-by: Sonny Sasaka <sonnysasaka@chromium.org> > > --- > > > > src/adapter.c | 17 ++++++++++++++++- > > 1 file changed, 16 insertions(+), 1 deletion(-) > > > > diff --git a/src/adapter.c b/src/adapter.c > > index c0053000a..74bfb0448 100644 > > --- a/src/adapter.c > > +++ b/src/adapter.c > > @@ -1507,8 +1507,10 @@ static void discovery_free(void *user_data) > > client->discovery_filter = NULL; > > } > > > > - if (client->msg) > > + if (client->msg) { > > dbus_message_unref(client->msg); > > + client->msg = NULL; > > + } > > This shouldn't make any difference as the whole list is freed and so > is the client. Please see my below reply. > > > > > g_free(client->owner); > > g_free(client); > > @@ -5301,6 +5303,19 @@ static void free_service_auth(gpointer data, gpointer user_data) > > > > static void remove_discovery_list(struct btd_adapter *adapter) > > { > > + DBusMessage *msg; > > + > > + if (adapter->client) { > > + msg = adapter->client->msg; > > + if (msg) { > > + g_dbus_send_message(dbus_conn, btd_error_busy(msg)); > > + dbus_message_unref(msg); > > + adapter->client->msg = NULL; > > + } > > + > > + adapter->client = NULL; > > Shouldn't you call discovery_free as well here? Or perhaps we could > move the lines above inside discovery_free so it detects if the > adapter->client is pointing to a client that is being freed. > discovery_free are done in the following lines, so there is no need to call discovery_free() for a referencing pointer, adapter->client. It's a good point to move it so discovery_free() instead. > > + } > > + > > g_slist_free_full(adapter->set_filter_list, discovery_free); > > adapter->set_filter_list = NULL; > > > > -- > > 2.26.2 > > > > > -- > Luiz Augusto von Dentz Regards, Miao
diff --git a/src/adapter.c b/src/adapter.c index c0053000a..74bfb0448 100644 --- a/src/adapter.c +++ b/src/adapter.c @@ -1507,8 +1507,10 @@ static void discovery_free(void *user_data) client->discovery_filter = NULL; } - if (client->msg) + if (client->msg) { dbus_message_unref(client->msg); + client->msg = NULL; + } g_free(client->owner); g_free(client); @@ -5301,6 +5303,19 @@ static void free_service_auth(gpointer data, gpointer user_data) static void remove_discovery_list(struct btd_adapter *adapter) { + DBusMessage *msg; + + if (adapter->client) { + msg = adapter->client->msg; + if (msg) { + g_dbus_send_message(dbus_conn, btd_error_busy(msg)); + dbus_message_unref(msg); + adapter->client->msg = NULL; + } + + adapter->client = NULL; + } + g_slist_free_full(adapter->set_filter_list, discovery_free); adapter->set_filter_list = NULL;