[PATCH BlueZ 0/4] GATT server API fixes

[PATCH BlueZ 0/4] GATT server API fixes

[Arman Uguray]

This patch set includes some minor bug fixes for the GATT server D-Bus API and
adds the missing implementation for the GattManager1.UnregisterService method.

Arman Uguray (4):
  core/adapter: Deduplicate local service UUIDs
  core/gatt: Fix crash in gatt-database destructor
  core/gatt: Remove spammy log messages
  core/gatt: Add UnregisterService and track sender

 src/adapter.c       | 30 +++++++++++++++++--------
 src/gatt-database.c | 63 +++++++++++++++++++++++++++++++++++++++++------------
 2 files changed, 70 insertions(+), 23 deletions(-)

-- 
2.2.0.rc0.207.ga3a616c

[PATCH BlueZ 1/4] core/adapter: Deduplicate local service UUIDs

[Arman Uguray]

An adapter's local GATT database can contain multiple services with the
same UUID and some of these services may also be present in the SDP
tables. This patch deduplicates these so that the response returned for
the "UUIDs" property only contains a single instance of each service
UUID.
---
 src/adapter.c | 30 +++++++++++++++++++++---------
 1 file changed, 21 insertions(+), 9 deletions(-)

diff --git a/src/adapter.c b/src/adapter.c
index c12f557..93de573 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -2192,10 +2192,9 @@ static gboolean property_get_discovering(const GDBusPropertyTable *property,
 
 static void add_gatt_uuid(struct gatt_db_attribute *attrib, void *user_data)
 {
-	DBusMessageIter *iter = user_data;
+	GHashTable *uuids = user_data;
 	bt_uuid_t uuid, u128;
 	char uuidstr[MAX_LEN_UUID_STR + 1];
-	const char *ptr = uuidstr;
 
 	if (!gatt_db_service_get_active(attrib))
 		return;
@@ -2206,7 +2205,15 @@ static void add_gatt_uuid(struct gatt_db_attribute *attrib, void *user_data)
 	bt_uuid_to_uuid128(&uuid, &u128);
 	bt_uuid_to_string(&u128, uuidstr, sizeof(uuidstr));
 
-	dbus_message_iter_append_basic(iter, DBUS_TYPE_STRING, &ptr);
+	g_hash_table_add(uuids, strdup(uuidstr));
+}
+
+static void iter_append_uuid(gpointer key, gpointer value, gpointer user_data)
+{
+	DBusMessageIter *iter = user_data;
+	const char *uuid = key;
+
+	dbus_message_iter_append_basic(iter, DBUS_TYPE_STRING, &uuid);
 }
 
 static gboolean property_get_uuids(const GDBusPropertyTable *property,
@@ -2216,9 +2223,11 @@ static gboolean property_get_uuids(const GDBusPropertyTable *property,
 	DBusMessageIter entry;
 	sdp_list_t *l;
 	struct gatt_db *db;
+	GHashTable *uuids;
 
-	dbus_message_iter_open_container(iter, DBUS_TYPE_ARRAY,
-					DBUS_TYPE_STRING_AS_STRING, &entry);
+	uuids = g_hash_table_new_full(g_str_hash, g_str_equal, free, NULL);
+	if (!uuids)
+		return FALSE;
 
 	/* SDP records */
 	for (l = adapter->services; l != NULL; l = l->next) {
@@ -2229,18 +2238,21 @@ static gboolean property_get_uuids(const GDBusPropertyTable *property,
 		if (uuid == NULL)
 			continue;
 
-		dbus_message_iter_append_basic(&entry, DBUS_TYPE_STRING,
-								&uuid);
-		free(uuid);
+		g_hash_table_add(uuids, uuid);
 	}
 
 	/* GATT services */
 	db = btd_gatt_database_get_db(adapter->database);
 	if (db)
-		gatt_db_foreach_service(db, NULL, add_gatt_uuid, &entry);
+		gatt_db_foreach_service(db, NULL, add_gatt_uuid, uuids);
 
+	dbus_message_iter_open_container(iter, DBUS_TYPE_ARRAY,
+					DBUS_TYPE_STRING_AS_STRING, &entry);
+	g_hash_table_foreach(uuids, iter_append_uuid, &entry);
 	dbus_message_iter_close_container(iter, &entry);
 
+	g_hash_table_destroy(uuids);
+
 	return TRUE;
 }
 
-- 
2.2.0.rc0.207.ga3a616c

[PATCH BlueZ 2/4] core/gatt: Fix crash in gatt-database destructor

[Arman Uguray]

This patch fixes an invalid access that occurs during daemon shutdown
if at least one external GATT service has been registered:

==4764== Invalid read of size 8
==4764==    at 0x4C8812: queue_foreach (queue.c:241)
==4764==    by 0x47A29C: send_notification_to_devices (gatt-database.c:904)
==4764==    by 0x47BAB8: send_service_changed (gatt-database.c:932)
==4764==    by 0x47BB3D: gatt_db_service_removed (gatt-database.c:972)
==4764==    by 0x4D5CA1: handle_notify (gatt-db.c:264)
==4764==    by 0x4C888F: queue_foreach (queue.c:251)
==4764==    by 0x4D675B: notify_service_changed (gatt-db.c:281)
==4764==    by 0x4D680C: gatt_db_service_destroy (gatt-db.c:292)
==4764==    by 0x4D6889: gatt_db_remove_service (gatt-db.c:424)
==4764==    by 0x47B237: service_free (gatt-database.c:347)
==4764==    by 0x4C8C4F: queue_remove_all (queue.c:387)
==4764==    by 0x4C8CB4: queue_destroy (queue.c:76)
==4764==  Address 0x5e9d0f8 is 8 bytes inside a block of size 32 free'd
==4764==    at 0x4C2ACE9: free (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==4764==    by 0x4C8488: queue_unref (queue.c:53)
==4764==    by 0x4C8CC4: queue_destroy (queue.c:78)
==4764==    by 0x47C2E5: gatt_database_free (gatt-database.c:394)
==4764==    by 0x47D21D: btd_gatt_database_destroy (gatt-database.c:2203)
==4764==    by 0x48809F: adapter_remove (adapter.c:4595)
==4764==    by 0x495D42: adapter_cleanup (adapter.c:7486)
==4764==    by 0x40BBDD: main (main.c:666)
==4764==
==4764== Invalid read of size 8
==4764==    at 0x4C8812: queue_foreach (queue.c:241)
==4764==    by 0x47BB56: gatt_db_service_removed (gatt-database.c:974)
==4764==    by 0x4D5CA1: handle_notify (gatt-db.c:264)
==4764==    by 0x4C888F: queue_foreach (queue.c:251)
==4764==    by 0x4D675B: notify_service_changed (gatt-db.c:281)
==4764==    by 0x4D680C: gatt_db_service_destroy (gatt-db.c:292)
==4764==    by 0x4D6889: gatt_db_remove_service (gatt-db.c:424)
==4764==    by 0x47B237: service_free (gatt-database.c:347)
==4764==    by 0x4C8C4F: queue_remove_all (queue.c:387)
==4764==    by 0x4C8CB4: queue_destroy (queue.c:76)
==4764==    by 0x47C2FB: gatt_database_free (gatt-database.c:395)
==4764==    by 0x47D21D: btd_gatt_database_destroy (gatt-database.c:2203)
==4764==  Address 0x5e9d0f8 is 8 bytes inside a block of size 32 free'd
==4764==    at 0x4C2ACE9: free (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==4764==    by 0x4C8488: queue_unref (queue.c:53)
==4764==    by 0x4C8CC4: queue_destroy (queue.c:78)
==4764==    by 0x47C2E5: gatt_database_free (gatt-database.c:394)
==4764==    by 0x47D21D: btd_gatt_database_destroy (gatt-database.c:2203)
==4764==    by 0x48809F: adapter_remove (adapter.c:4595)
==4764==    by 0x495D42: adapter_cleanup (adapter.c:7486)
==4764==    by 0x40BBDD: main (main.c:666)
==4764==
---
 src/gatt-database.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/gatt-database.c b/src/gatt-database.c
index a68bb4f..21c9e95 100644
--- a/src/gatt-database.c
+++ b/src/gatt-database.c
@@ -391,14 +391,16 @@ static void gatt_database_free(void *data)
 		adapter_service_remove(database->adapter, database->gap_handle);
 
 	/* TODO: Persistently store CCC states before freeing them */
+	gatt_db_unregister(database->db, database->db_id);
+
 	queue_destroy(database->device_states, device_state_free);
 	queue_destroy(database->services, service_free);
 	queue_destroy(database->ccc_callbacks, ccc_cb_free);
 	database->device_states = NULL;
 	database->ccc_callbacks = NULL;
 
-	gatt_db_unregister(database->db, database->db_id);
 	gatt_db_unref(database->db);
+
 	btd_adapter_unref(database->adapter);
 	free(database);
 }
-- 
2.2.0.rc0.207.ga3a616c

[PATCH BlueZ 3/4] core/gatt: Remove spammy log messages

[Arman Uguray]

This patch removes some spammy debug log messages regarding CEP/CCC
descriptor processing.
---
 src/gatt-database.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/src/gatt-database.c b/src/gatt-database.c
index 21c9e95..c7b2223 100644
--- a/src/gatt-database.c
+++ b/src/gatt-database.c
@@ -1708,10 +1708,8 @@ static bool database_add_ccc(struct external_service *service,
 						struct external_chrc *chrc)
 {
 	if (!(chrc->props & BT_GATT_CHRC_PROP_NOTIFY) &&
-				!(chrc->props & BT_GATT_CHRC_PROP_INDICATE)) {
-		DBG("No need to create CCC entry for characteristic");
+				!(chrc->props & BT_GATT_CHRC_PROP_INDICATE))
 		return true;
-	}
 
 	chrc->ccc = service_add_ccc(service->attrib, service->database,
 						ccc_write_cb, chrc, NULL);
@@ -1726,6 +1724,8 @@ static bool database_add_ccc(struct external_service *service,
 		return false;
 	}
 
+	DBG("Created CCC entry for characteristic");
+
 	return true;
 }
 
@@ -1745,10 +1745,8 @@ static bool database_add_cep(struct external_service *service,
 	bt_uuid_t uuid;
 	uint8_t value[2];
 
-	if (!chrc->ext_props) {
-		DBG("No need to create CEP entry for characteristic");
+	if (!chrc->ext_props)
 		return true;
-	}
 
 	bt_uuid16_create(&uuid, GATT_CHARAC_EXT_PROPER_UUID);
 	cep = gatt_db_service_add_descriptor(service->attrib, &uuid,
@@ -1768,6 +1766,8 @@ static bool database_add_cep(struct external_service *service,
 		return false;
 	}
 
+	DBG("Created CEP entry for characteristic");
+
 	return true;
 }
 
-- 
2.2.0.rc0.207.ga3a616c

[PATCH BlueZ 4/4] core/gatt: Add UnregisterService and track sender

[Arman Uguray]

This patch implements the GattManager1.UnregisterService method. The
API implementation is slightly changed so that external services are
tracked on a per path + sender basis and not just based on the object
path.
---
 src/gatt-database.c | 47 ++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 40 insertions(+), 7 deletions(-)

diff --git a/src/gatt-database.c b/src/gatt-database.c
index c7b2223..d4dd661 100644
--- a/src/gatt-database.c
+++ b/src/gatt-database.c
@@ -978,12 +978,18 @@ static void gatt_db_service_removed(struct gatt_db_attribute *attrib,
 								ccc_cb_free);
 }
 
-static bool match_service_path(const void *a, const void *b)
+struct svc_match_data {
+	const char *path;
+	const char *sender;
+};
+
+static bool match_service(const void *a, const void *b)
 {
 	const struct external_service *service = a;
-	const char *path = b;
+	const struct svc_match_data *data = b;
 
-	return g_strcmp0(service->path, path) == 0;
+	return g_strcmp0(service->path, data->path) == 0 &&
+				g_strcmp0(service->owner, data->sender) == 0;
 }
 
 static gboolean service_free_idle_cb(void *data)
@@ -2064,9 +2070,11 @@ static DBusMessage *manager_register_service(DBusConnection *conn,
 					DBusMessage *msg, void *user_data)
 {
 	struct btd_gatt_database *database = user_data;
+	const char *sender = dbus_message_get_sender(msg);
 	DBusMessageIter args;
 	const char *path;
 	struct external_service *service;
+	struct svc_match_data match_data;
 
 	if (!dbus_message_iter_init(msg, &args))
 		return btd_error_invalid_args(msg);
@@ -2076,7 +2084,10 @@ static DBusMessage *manager_register_service(DBusConnection *conn,
 
 	dbus_message_iter_get_basic(&args, &path);
 
-	if (queue_find(database->services, match_service_path, path))
+	match_data.path = path;
+	match_data.sender = sender;
+
+	if (queue_find(database->services, match_service, &match_data))
 		return btd_error_already_exists(msg);
 
 	dbus_message_iter_next(&args);
@@ -2098,10 +2109,32 @@ static DBusMessage *manager_register_service(DBusConnection *conn,
 static DBusMessage *manager_unregister_service(DBusConnection *conn,
 					DBusMessage *msg, void *user_data)
 {
-	DBG("UnregisterService");
+	struct btd_gatt_database *database = user_data;
+	const char *sender = dbus_message_get_sender(msg);
+	const char *path;
+	DBusMessageIter args;
+	struct external_service *service;
+	struct svc_match_data match_data;
 
-	/* TODO */
-	return NULL;
+	if (!dbus_message_iter_init(msg, &args))
+		return btd_error_invalid_args(msg);
+
+	if (dbus_message_iter_get_arg_type(&args) != DBUS_TYPE_OBJECT_PATH)
+		return btd_error_invalid_args(msg);
+
+	dbus_message_iter_get_basic(&args, &path);
+
+	match_data.path = path;
+	match_data.sender = sender;
+
+	service = queue_remove_if(database->services, match_service,
+								&match_data);
+	if (!service)
+		return btd_error_does_not_exist(msg);
+
+	service_free(service);
+
+	return dbus_message_new_method_return(msg);
 }
 
 static const GDBusMethodTable manager_methods[] = {
-- 
2.2.0.rc0.207.ga3a616c

Re: [PATCH BlueZ 0/4] GATT server API fixes

[Johan Hedberg]

Hi Arman,

On Wed, Mar 04, 2015, Arman Uguray wrote:
> This patch set includes some minor bug fixes for the GATT server D-Bus API and
> adds the missing implementation for the GattManager1.UnregisterService method.
> 
> Arman Uguray (4):
>   core/adapter: Deduplicate local service UUIDs
>   core/gatt: Fix crash in gatt-database destructor
>   core/gatt: Remove spammy log messages
>   core/gatt: Add UnregisterService and track sender
> 
>  src/adapter.c       | 30 +++++++++++++++++--------
>  src/gatt-database.c | 63 +++++++++++++++++++++++++++++++++++++++++------------
>  2 files changed, 70 insertions(+), 23 deletions(-)

All patches in this set have been applied. Thanks.

Johan