Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 14 Aug 2020 15:58:18 -0700
Message-Id: <20200814155807.Bluez.1.Ia90c97ad9ec0b51b7aaae1216864e8379c1470d5@changeid>
Mime-Version: 1.0
Subject: [Bluez PATCH] advertising: Fix dbus response for over-advertising
From:   Daniel Winkler <danielwinkler@google.com>
To:     simon.mikuda@streamunlimited.com, linux-bluetooth@vger.kernel.org
Cc:     chromeos-bluetooth-upstreaming@chromium.org,
        Daniel Winkler <danielwinkler@google.com>,
        Sonny Sasaka <sonnysasaka@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-bluetooth-owner@vger.kernel.org
Precedence: bulk

client_free would always send a dbus method_return to fix the case where
a request to Unregister occurred before the MGMT call returned. However,
in the code path where too many advertisements are registered, this
method_return prevents the failure from being sent properly. This patch
makes sure the reference to the register_advertisement DBusMessage is
not stored in the client structure until the end of
register_advertisement. This ensures that we only respond once, either
in register_advertisement or in client_free, not both.

It also changes the dbus response in the fast unregister_advertisement
case from a method_return to a btd_error_failed, since the registration
was never allowed to complete, and thus was not successful.

The patch was tested in the following ways:

To verify it did not break the segfault fix in
caff2b48ca54bbc57b5da3f63317767489aa5b48, I repro'd the failure by
quickly unregistering after registering, and verified that the segfault
is still fixed with this change.

Ran through our automated tests that register too many advertisements
and verify that the registration fails with the intended "Maximum
Advertisements Reached" error response.

Reviewed-by: Sonny Sasaka <sonnysasaka@chromium.org>

---

 src/advertising.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/src/advertising.c b/src/advertising.c
index 076d591b6..e5f25948d 100644
--- a/src/advertising.c
+++ b/src/advertising.c
@@ -119,9 +119,13 @@ static void client_free(void *data)
 	}
 
 	if (client->reg) {
-		g_dbus_send_message(btd_get_dbus_connection(),
-				dbus_message_new_method_return(client->reg));
+		DBusMessage *reply;
+
+		reply = btd_error_failed(client->reg,
+					"Failed to complete registration");
+		g_dbus_send_message(btd_get_dbus_connection(), reply);
 		dbus_message_unref(client->reg);
+		client->reg = NULL;
 	}
 
 	if (client->add_adv_id)
@@ -1152,8 +1156,6 @@ static struct btd_adv_client *client_create(struct btd_adv_manager *manager,
 	g_dbus_client_set_proxy_handlers(client->client, client_proxy_added,
 							NULL, NULL, client);
 
-	client->reg = dbus_message_ref(msg);
-
 	client->data = bt_ad_new();
 	if (!client->data)
 		goto fail;
@@ -1216,6 +1218,8 @@ static DBusMessage *register_advertisement(DBusConnection *conn,
 
 	DBG("Registered advertisement at path %s", match.path);
 
+	client->reg = dbus_message_ref(msg);
+
 	queue_push_tail(manager->clients, client);
 
 	return NULL;
-- 
2.28.0.220.ged08abb693-goog