Bluetooth: fix GATT service reregistering

* Fixed issue with reregistering of GATT services. * Added unit tests covering the GATT reregistering scenario. Signed-off-by: Stine Åkredalen <[email protected]>
zephyrproject-rtos · Nov 28, 2024 · 5d27478 · 5d27478
1 parent c22233a
commit 5d27478
Show file tree

Hide file tree

Showing 3 changed files with 194 additions and 1 deletion.
diff --git a/include/zephyr/bluetooth/gatt.h b/include/zephyr/bluetooth/gatt.h
@@ -296,8 +296,14 @@ struct bt_gatt_attr {
 	 *  satisfied before calling read() or write().
 	 *
 	 *  @sa bt_gatt_discover_func_t about this field.
+	 *
+	 *  @note The `_auto_assigned_handle` flag in this bitfield indicates if the attribute
+	 *  handle was automatically assigned by the stack (1) or manually set by the
+	 *  application (0).
+	 *  This flag must not be modified by the application.
 	 */
-	uint16_t perm;
+	uint16_t perm: 15;
+	uint16_t _auto_assigned_handle: 1;
 };
 
 /** @brief GATT Service structure */

diff --git a/subsys/bluetooth/host/gatt.c b/subsys/bluetooth/host/gatt.c
@@ -1307,9 +1307,11 @@ static int gatt_register(struct bt_gatt_service *svc)
 populate:
 	/* Populate the handles and append them to the list */
 	for (; attrs && count; attrs++, count--) {
+		attrs->_auto_assigned_handle = 0;
 		if (!attrs->handle) {
 			/* Allocate handle if not set already */
 			attrs->handle = ++handle;
+			attrs->_auto_assigned_handle = 1;
 		} else if (attrs->handle > handle) {
 			/* Use existing handle if valid */
 			handle = attrs->handle;
@@ -1727,6 +1729,12 @@ static int gatt_unregister(struct bt_gatt_service *svc)
 		if (is_host_managed_ccc(attr)) {
 			gatt_unregister_ccc(attr->user_data);
 		}
+
+		/* The stack should not clear any handles set by the user. */
+		if (attr->_auto_assigned_handle) {
+			attr->handle = 0;
+			attr->_auto_assigned_handle = 0;
+		}
 	}
 
 	return 0;

diff --git a/tests/bluetooth/gatt/src/main.c b/tests/bluetooth/gatt/src/main.c
@@ -142,6 +142,185 @@ ZTEST(test_gatt, test_gatt_unregister)
 		     "Test service unregister failed");
 }
 
+/* Test that a service A can be re-registered after registering it once, unregistering it, and then
+ * registering another service B.
+ * No pre-allocated handles. Repeat the process multiple times.
+ */
+ZTEST(test_gatt, test_gatt_reregister)
+{
+	struct bt_gatt_attr local_test_attrs[] = {
+		/* Vendor Primary Service Declaration */
+		BT_GATT_PRIMARY_SERVICE(&test_uuid),
+
+		BT_GATT_CHARACTERISTIC(&test_chrc_uuid.uuid, BT_GATT_CHRC_READ | BT_GATT_CHRC_WRITE,
+				       BT_GATT_PERM_READ_AUTHEN | BT_GATT_PERM_WRITE_AUTHEN,
+				       read_test, write_test, test_value),
+	};
+
+	struct bt_gatt_attr local_test1_attrs[] = {
+		/* Vendor Primary Service Declaration */
+		BT_GATT_PRIMARY_SERVICE(&test1_uuid),
+
+		BT_GATT_CHARACTERISTIC(&test1_nfy_uuid.uuid, BT_GATT_CHRC_NOTIFY, BT_GATT_PERM_NONE,
+				       NULL, NULL, &nfy_enabled),
+		BT_GATT_CCC(test1_ccc_cfg_changed, BT_GATT_PERM_READ | BT_GATT_PERM_WRITE),
+	};
+	struct bt_gatt_service local_test_svc = BT_GATT_SERVICE(local_test_attrs);
+	struct bt_gatt_service local_test1_svc = BT_GATT_SERVICE(local_test1_attrs);
+
+	/* Checks that the procedure is successful for a few iterations. However, a single iteration
+	 * was sufficient to trigger the reported bug.
+	 */
+	for (int i = 0; i < 10; i++) {
+
+		zassert_false(bt_gatt_service_register(&local_test_svc),
+			      "Test service A registration failed");
+
+		zassert_false(bt_gatt_service_unregister(&local_test_svc),
+			      "Test service A unregister failed");
+
+		/* Check that the handles are the same as before registering the service */
+		for (int j = 0; j < local_test_svc.attr_count; j++) {
+			zassert_equal(local_test_svc.attrs[j].handle, 0x0000,
+				      "Test service A handle not reset");
+		}
+
+		zassert_false(bt_gatt_service_register(&local_test1_svc),
+			      "Test service B registration failed");
+
+		zassert_false(bt_gatt_service_register(&local_test_svc),
+			      "Test service A re-registering failed...");
+
+		/* Clean up */
+		zassert_false(bt_gatt_service_unregister(&local_test_svc),
+			      "Test service A unregister failed");
+		zassert_false(bt_gatt_service_unregister(&local_test1_svc),
+			      "Test service B unregister failed");
+	}
+}
+
+/* Test that a service A can be re-registered after registering it once, unregistering it, and then
+ * registering another service B.
+ * Service A and B both have pre-allocated handles for their attributes.
+ * Check that pre-allocated handles are the same after unregistering as they were before
+ * registering the service.
+ */
+ZTEST(test_gatt, test_gatt_reregister_pre_allocated_handles)
+{
+	struct bt_gatt_attr local_test_attrs[] = {
+		/* Vendor Primary Service Declaration */
+		BT_GATT_PRIMARY_SERVICE(&test_uuid),
+
+		BT_GATT_CHARACTERISTIC(&test_chrc_uuid.uuid, BT_GATT_CHRC_READ | BT_GATT_CHRC_WRITE,
+				       BT_GATT_PERM_READ_AUTHEN | BT_GATT_PERM_WRITE_AUTHEN,
+				       read_test, write_test, test_value),
+	};
+
+	struct bt_gatt_attr local_test1_attrs[] = {
+		/* Vendor Primary Service Declaration */
+		BT_GATT_PRIMARY_SERVICE(&test1_uuid),
+
+		BT_GATT_CHARACTERISTIC(&test1_nfy_uuid.uuid, BT_GATT_CHRC_NOTIFY, BT_GATT_PERM_NONE,
+				       NULL, NULL, &nfy_enabled),
+		BT_GATT_CCC(test1_ccc_cfg_changed, BT_GATT_PERM_READ | BT_GATT_PERM_WRITE),
+	};
+
+	struct bt_gatt_service prealloc_test_svc = BT_GATT_SERVICE(local_test_attrs);
+	struct bt_gatt_service prealloc_test1_svc = BT_GATT_SERVICE(local_test1_attrs);
+
+	/* Pre-allocate handles for both services */
+	for (int i = 0; i < prealloc_test_svc.attr_count; i++) {
+		prealloc_test_svc.attrs[i].handle = 0x0100 + i;
+	}
+	for (int i = 0; i < prealloc_test1_svc.attr_count; i++) {
+		prealloc_test1_svc.attrs[i].handle = 0x0200 + i;
+	}
+
+	zassert_false(bt_gatt_service_register(&prealloc_test_svc),
+		      "Test service A registration failed");
+
+	zassert_false(bt_gatt_service_unregister(&prealloc_test_svc),
+		      "Test service A unregister failed");
+
+	/* Check that the handles are the same as before registering the service */
+	for (int i = 0; i < prealloc_test_svc.attr_count; i++) {
+		zassert_equal(prealloc_test_svc.attrs[i].handle, 0x0100 + i,
+			      "Test service A handle not reset");
+	}
+
+	zassert_false(bt_gatt_service_register(&prealloc_test1_svc),
+		      "Test service B registration failed");
+
+	zassert_false(bt_gatt_service_register(&prealloc_test_svc),
+		      "Test service A re-registering failed...");
+
+	/* Clean up */
+	zassert_false(bt_gatt_service_unregister(&prealloc_test_svc),
+		      "Test service A unregister failed");
+	zassert_false(bt_gatt_service_unregister(&prealloc_test1_svc),
+		      "Test service B unregister failed");
+}
+
+/* Test that a service A can be re-registered after registering it once, unregistering it, and then
+ * registering another service B.
+ * Service A has pre-allocated handles for its attributes, while Service B has handles assigned by
+ * the stack when registered.
+ * Check that pre-allocated handles are the same after unregistering as they were before
+ * registering the service.
+ */
+ZTEST(test_gatt, test_gatt_reregister_pre_allocated_handle_single)
+{
+	struct bt_gatt_attr local_test_attrs[] = {
+		/* Vendor Primary Service Declaration */
+		BT_GATT_PRIMARY_SERVICE(&test_uuid),
+
+		BT_GATT_CHARACTERISTIC(&test_chrc_uuid.uuid, BT_GATT_CHRC_READ | BT_GATT_CHRC_WRITE,
+				       BT_GATT_PERM_READ_AUTHEN | BT_GATT_PERM_WRITE_AUTHEN,
+				       read_test, write_test, test_value),
+	};
+
+	struct bt_gatt_attr local_test1_attrs[] = {
+		/* Vendor Primary Service Declaration */
+		BT_GATT_PRIMARY_SERVICE(&test1_uuid),
+
+		BT_GATT_CHARACTERISTIC(&test1_nfy_uuid.uuid, BT_GATT_CHRC_NOTIFY, BT_GATT_PERM_NONE,
+				       NULL, NULL, &nfy_enabled),
+		BT_GATT_CCC(test1_ccc_cfg_changed, BT_GATT_PERM_READ | BT_GATT_PERM_WRITE),
+	};
+
+	struct bt_gatt_service prealloc_test_svc = BT_GATT_SERVICE(local_test_attrs);
+	struct bt_gatt_service auto_test_svc = BT_GATT_SERVICE(local_test1_attrs);
+
+	/* Pre-allocate handles for one service only */
+	for (int j = 0; j < prealloc_test_svc.attr_count; j++) {
+		prealloc_test_svc.attrs[j].handle = 0x0100 + j;
+	}
+
+	zassert_false(bt_gatt_service_register(&prealloc_test_svc),
+		      "Test service A registration failed");
+
+	zassert_false(bt_gatt_service_unregister(&prealloc_test_svc),
+		      "Test service A unregister failed");
+
+	/* Check that the handles are the same as before registering the service */
+	for (int i = 0; i < prealloc_test_svc.attr_count; i++) {
+		zassert_equal(prealloc_test_svc.attrs[i].handle, 0x0100 + i,
+			      "Test service A handle not reset");
+	}
+
+	zassert_false(bt_gatt_service_register(&auto_test_svc),
+		      "Test service B registration failed");
+
+	zassert_false(bt_gatt_service_register(&prealloc_test_svc),
+		      "Test service A re-registering failed...");
+
+	/* Clean up */
+	zassert_false(bt_gatt_service_unregister(&prealloc_test_svc),
+		      "Test service A unregister failed");
+	zassert_false(bt_gatt_service_unregister(&auto_test_svc),
+		      "Test service B unregister failed");
+}
+
 static uint8_t count_attr(const struct bt_gatt_attr *attr, uint16_t handle,
 			  void *user_data)
 {