[PATCH BlueZ 1/3] adapter: Do not remove client watch directly if discovery fails

[PATCH BlueZ 1/3] adapter: Do not remove client watch directly if discovery fails

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

Client watch is used for both discovery and it filters so in case the
client has set the later the watch shall be perserved.
---
 src/adapter.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/adapter.c b/src/adapter.c
index 76acfea70..bf51b120b 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -1651,7 +1651,6 @@ fail:
 	if (client->msg) {
 		reply = btd_error_busy(client->msg);
 		g_dbus_send_message(dbus_conn, reply);
-		g_dbus_remove_watch(dbus_conn, client->watch);
 		discovery_remove(client, false);
 		return;
 	}
-- 
2.25.3

[PATCH BlueZ 2/3] adapter: Consolitate code for discovery reply

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

This consolidate code that were used to reply discovery commands in a
single function so it easier to reuse and maintain.
---
 src/adapter.c | 45 +++++++++++++++++++++++----------------------
 1 file changed, 23 insertions(+), 22 deletions(-)

diff --git a/src/adapter.c b/src/adapter.c
index bf51b120b..c23c84175 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -1584,13 +1584,30 @@ static void discovery_remove(struct watch_client *client, bool exit)
 
 static void trigger_start_discovery(struct btd_adapter *adapter, guint delay);
 
+static void discovery_reply(struct watch_client *client, uint8_t status)
+{
+	DBusMessage *reply;
+
+	if (!client->msg)
+		return;
+
+	if (!status) {
+		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;
+}
+
 static void start_discovery_complete(uint8_t status, uint16_t length,
 					const void *param, void *user_data)
 {
 	struct btd_adapter *adapter = user_data;
 	struct watch_client *client;
 	const struct mgmt_cp_start_discovery *rp = param;
-	DBusMessage *reply;
 
 	DBG("status 0x%02x", status);
 
@@ -1630,12 +1647,7 @@ static void start_discovery_complete(uint8_t status, uint16_t length,
 		else
 			adapter->filtered_discovery = false;
 
-		if (client->msg) {
-			g_dbus_send_reply(dbus_conn, client->msg,
-						DBUS_TYPE_INVALID);
-			dbus_message_unref(client->msg);
-			client->msg = NULL;
-		}
+		discovery_reply(client, status);
 
 		if (adapter->discovering)
 			return;
@@ -1649,8 +1661,7 @@ static void start_discovery_complete(uint8_t status, uint16_t length,
 fail:
 	/* Reply with an error if the first discovery has failed */
 	if (client->msg) {
-		reply = btd_error_busy(client->msg);
-		g_dbus_send_message(dbus_conn, reply);
+		discovery_reply(client, status);
 		discovery_remove(client, false);
 		return;
 	}
@@ -1917,23 +1928,13 @@ static void stop_discovery_complete(uint8_t status, uint16_t length,
 {
 	struct watch_client *client = user_data;
 	struct btd_adapter *adapter = client->adapter;
-	DBusMessage *reply;
 
 	DBG("status 0x%02x", status);
 
-	if (status != MGMT_STATUS_SUCCESS) {
-		if (client->msg) {
-			reply = btd_error_busy(client->msg);
-			g_dbus_send_message(dbus_conn, reply);
-		}
-		goto done;
-	}
+	discovery_reply(client, status);
 
-	if (client->msg) {
-		g_dbus_send_reply(dbus_conn, client->msg, DBUS_TYPE_INVALID);
-		dbus_message_unref(client->msg);
-		client->msg = NULL;
-	}
+	if (status != MGMT_STATUS_SUCCESS)
+		goto done;
 
 	adapter->discovery_type = 0x00;
 	adapter->discovery_enable = 0x00;
-- 
2.25.3

[PATCH BlueZ 3/3] adapter: Fix possible crash when stopping discovery

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

If the client disconnect/crash while MGMT_OP_STOP_DISCOVERY was pending
it would possibly cause a crash as the client pointer is passed to
mgmt_send and accessed in the callback after being freed.

To fix this the adapter itself is now passed to the callback so the
client is not accessed directly, instead the code now checks if
discovery_list has not been cleared in the meantime and only then
proceed to access the client pointer which is how
MGMT_OP_START_DISCOVERY is handled.
---
 src/adapter.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/src/adapter.c b/src/adapter.c
index c23c84175..64815ecd2 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -1926,11 +1926,19 @@ static bool set_discovery_discoverable(struct btd_adapter *adapter, bool enable)
 static void stop_discovery_complete(uint8_t status, uint16_t length,
 					const void *param, void *user_data)
 {
-	struct watch_client *client = user_data;
-	struct btd_adapter *adapter = client->adapter;
+	struct btd_adapter *adapter = user_data;
+	struct watch_client *client;
 
 	DBG("status 0x%02x", status);
 
+	/* Is there are no clients the discovery must have been stopped while
+	 * discovery command was pending.
+	 */
+	if (!adapter->discovery_list)
+		return;
+
+	client = adapter->discovery_list->data;
+
 	discovery_reply(client, status);
 
 	if (status != MGMT_STATUS_SUCCESS)
-- 
2.25.3

Re: [PATCH BlueZ 1/3] adapter: Do not remove client watch directly if discovery fails

[Luiz Augusto von Dentz]

Hi,

On Mon, Jun 8, 2020 at 3:01 PM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>
> Client watch is used for both discovery and it filters so in case the
> client has set the later the watch shall be perserved.
> ---
>  src/adapter.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/src/adapter.c b/src/adapter.c
> index 76acfea70..bf51b120b 100644
> --- a/src/adapter.c
> +++ b/src/adapter.c
> @@ -1651,7 +1651,6 @@ fail:
>         if (client->msg) {
>                 reply = btd_error_busy(client->msg);
>                 g_dbus_send_message(dbus_conn, reply);
> -               g_dbus_remove_watch(dbus_conn, client->watch);
>                 discovery_remove(client, false);
>                 return;
>         }
> --
> 2.25.3
>

Pushed.

-- 
Luiz Augusto von Dentz