Hi Luiz,
For 0001-adapter-Do-not-remove-client-watch-directly-if-disco.patch,
it looks good to me.
For 0002-adapter-Consolitate-code-for-discovery-reply.patch, please
see me following comments.
+static void discovery_reply(struct watch_client *client, uint8_t status)
+{
+ DBusMessage *reply;
+
+ if (!client->msg)
+ return;
+
+ if (!status) {
It'd better to change this to "if (status == MGMT_STATUS_SUCCESS) {".
+ g_dbus_send_reply(dbus_conn, client->msg, DBUS_TYPE_INVALID);
+ } else {
+ reply = btd_error_busy(client->msg);
+ g_dbus_send_message(dbus_conn, reply);
+ }
+
+ dbus_message_unref(client->msg);
+ client->msg = NULL;
+}
I also notice that we treated the status other than
MGMT_STATUS_SUCCESS to be busy, but this can be addressed as a
separate patch.
For 0003-adapter-Fix-possible-crash-when-stopping-discovery.patch, I
had few comments here.
(1) I didn't see the corresponding changes to pass the pointer of the
adapter as the user data when sending MGMT_OP_STOP_DISCOVERY command.
Should it be part of the patch?
(2) This does resolve the crashing due to use-after-free of a
watch_client. However, the following logic doesn't seem to be correct
to me. If you recall the call path that we discussed, which is
"client1 start_discovery() -> client1 start_discovery_complete() ->
client1 stop_discovery() -> client2 start_discovery() -> client1
detach from D-Bus which triggers discovery_disconnect() -> client1
stop_discovery_complete() -> crash)",
when client2 starts the discovery, client2 is added to
adapter->discovery_list, so once stop_discovery_complete() is called
with client1, client2 is the only client in adapter->discovery_list.
And this statement remains true even with this patch. That being said,
the following "client = adapter->discovery_list->data" would return
client2, which is not supposed to be replied by
stop_discovery_complete() issued by client1. Agree?
+ if (!adapter->discovery_list)
+ return;
+
+ client = adapter->discovery_list->data;
Thanks,
Miao
On Tue, Jun 9, 2020 at 2:25 PM Von Dentz, Luiz <luiz.von.dentz@xxxxxxxxx> wrote:
>
> Hi,
>
>
> On Mon, Jun 8, 2020 at 6:11 PM Von Dentz, Luiz <luiz.von.dentz@xxxxxxxxx> wrote:
> >
> > Hi Miao,
> >
> > On Mon, Jun 8, 2020 at 6:03 PM Miao-chen Chou <mcchou@xxxxxxxxxxxx> wrote:
> >>
> >> This properly handles the unref of client->msg in
> >> stop_discovery_complete() and the reset of it. This also handles the unref
> >> of client->msg, the reset of client->watch and the reset of client->msg in
> >> start_discovery_complete().
> >>
> >> The following test was performed:
> >> (1) Intentionally changed the MGMT status other than MGMT_STATUS_SUCCESS
> >> in stop_discovery_complete() and start_discovery_complete() and built
> >> bluetoothd.
> >> (2) In bluetoothctl console, issued scan on/scan off to invoke
> >> StartDiscovery and verified that new discovery requests can be processed.
> >>
> >> Reviewed-by: Alain Michaud <alainm@xxxxxxxxxxxx>
> >> Reviewed-by: Sonny Sasaka <sonnysasaka@xxxxxxxxxxxx>
> >> ---
> >>
> >> src/adapter.c | 5 +++++
> >> 1 file changed, 5 insertions(+)
> >>
> >> diff --git a/src/adapter.c b/src/adapter.c
> >> index 76acfea70..0857a3115 100644
> >> --- a/src/adapter.c
> >> +++ b/src/adapter.c
> >> @@ -1652,6 +1652,9 @@ fail:
> >> reply = btd_error_busy(client->msg);
> >> g_dbus_send_message(dbus_conn, reply);
> >> g_dbus_remove_watch(dbus_conn, client->watch);
> >
> >
> > We shouldn't be removing the watch directly since the client may have registered filters so we let discovery_remove do it by calling discovery_free if necessary.
> >
> >>
> >> + client->watch = 0;
> >> + dbus_message_unref(client->msg);
> >> + client->msg = NULL;
> >> discovery_remove(client, false);
> >> return;
> >> }
> >> @@ -1926,6 +1929,8 @@ static void stop_discovery_complete(uint8_t status, uint16_t length,
> >> if (client->msg) {
> >> reply = btd_error_busy(client->msg);
> >> g_dbus_send_message(dbus_conn, reply);
> >> + dbus_message_unref(client->msg);
> >> + client->msg = NULL;
> >> }
> >> goto done;
> >> }
> >> --
> >> 2.26.2
> >
> >
> > Ive sent similar fixes upstream, let me attach them here just in case.
>
> Any comments on these changes, I would like to push them as soon as possible.
