[PATCH BlueZ 1/2] shared/mgmt: Fix notify leak in mgmt_unregister()

[PATCH BlueZ 1/2] shared/mgmt: Fix notify leak in mgmt_unregister()

[Iaroslav Voitovych]

mgmt_unregister() called from a notification callback takes the entry
off notify_list and marks it removed instead of freeing it.
process_notify() frees the removed entries still on notify_list once
the walk is done, so this one is never freed and its destroy callback
never runs.

If the callback unregisters the next entry rather than its own, this
is also a use-after-free: queue_foreach() has already saved that
entry's queue node, and queue_remove_if() frees it. That is the crash
fixed for mgmt_unregister_index() by commit 872729a91632
("shared/mgmt: Fix crash when removing index"), still present in
mgmt_unregister().

Leave the entry on the list while notifying, as mgmt_unregister_index()
and mgmt_unregister_all() do, and let process_notify() free it.
Unregistering an entry that is already marked removed now returns
false, since it is no longer registered; for an entry marked by
mgmt_unregister_index() or mgmt_unregister_all(), mgmt_unregister()
used to return true, take it off the list and leak it. No caller
checks the return value.

mgmt-tester, mesh-tester and userchan-tester unregister from event
callbacks, and under ASan each reports these entries as leaked, for
example userchan-tester:

  Direct leak of 80 byte(s) in 2 object(s) allocated from:
    #0 in malloc
    #1 in util_malloc src/shared/util.c:46
    #2 in mgmt_register src/shared/mgmt.c:974
    #3 in test_open_success tools/userchan-tester.c:275

Fixes: 872729a91632 ("shared/mgmt: Fix crash when removing index")
---
 src/shared/mgmt.c | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/src/shared/mgmt.c b/src/shared/mgmt.c
index f56bcee14..f5aeecf3f 100644
--- a/src/shared/mgmt.c
+++ b/src/shared/mgmt.c
@@ -999,18 +999,28 @@ bool mgmt_unregister(struct mgmt *mgmt, unsigned int id)
 	if (!mgmt || !id)
 		return false;
 
-	notify = queue_remove_if(mgmt->notify_list, match_notify_id,
+	/*
+	 * While notifying, leave the entry on the list for process_notify()
+	 * to free once it is done with it.
+	 */
+	if (mgmt->in_notify) {
+		notify = queue_find(mgmt->notify_list, match_notify_id,
 							UINT_TO_PTR(id));
-	if (!notify)
-		return false;
+		if (!notify || notify->removed)
+			return false;
+
+		notify->removed = true;
+		mgmt->need_notify_cleanup = true;
 
-	if (!mgmt->in_notify) {
-		destroy_notify(notify);
 		return true;
 	}
 
-	notify->removed = true;
-	mgmt->need_notify_cleanup = true;
+	notify = queue_remove_if(mgmt->notify_list, match_notify_id,
+							UINT_TO_PTR(id));
+	if (!notify)
+		return false;
+
+	destroy_notify(notify);
 
 	return true;
 }

base-commit: d84171e6cd68a5ab95d0f3a384279c36bd108a13
-- 
2.43.0

[PATCH BlueZ 2/2] unit/test-mgmt: Test unregistering from callbacks

[Iaroslav Voitovych]

Add /mgmt/unregister/3 to /5, where a notification callback
unregisters its own entry, the next entry, or another entry followed
by mgmt_unregister_all(). The unregistered entry must not be notified
again, its destroy callback must not run inside the notification and
must have run once by the next idle callback, and unregistering it
again must fail.

Add /mgmt/unregister/6, which unregisters outside a notification: the
entry is destroyed at once and unregistering it again fails.

Without the previous patch, /4 faults under ASan (heap-use-after-free
in queue_foreach()), /3 never destroys the entry, and /5 gets true for
an entry already marked removed.
---
 unit/test-mgmt.c | 225 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 225 insertions(+)

diff --git a/unit/test-mgmt.c b/unit/test-mgmt.c
index 691af79a4..7de612b58 100644
--- a/unit/test-mgmt.c
+++ b/unit/test-mgmt.c
@@ -416,6 +416,223 @@ static void test_destroy(gconstpointer data)
 	execute_context(context);
 }
 
+struct unregister_test;
+
+struct unregister_entry {
+	struct unregister_test *test;
+	unsigned int n;
+};
+
+/*
+ * An entry unregistered from a notification callback must not be notified
+ * again, must not be destroyed while the notification is dispatched, and
+ * must have been destroyed, once, by the next idle callback.
+ */
+struct unregister_test {
+	struct context *context;
+	unsigned int entries;
+	unsigned int events;
+	struct unregister_entry entry[3];
+	unsigned int id[3];
+	bool removed[3];
+	unsigned int called[3];
+	unsigned int expected[3];
+	unsigned int destroyed[3];
+};
+
+static void unregister_destroy(void *user_data)
+{
+	struct unregister_entry *entry = user_data;
+
+	entry->test->destroyed[entry->n]++;
+	g_assert_cmpuint(entry->test->destroyed[entry->n], ==, 1);
+}
+
+static void unregister_add(struct unregister_test *test,
+				const struct command_test_data *data,
+				mgmt_notify_func_t callback)
+{
+	unsigned int n = test->entries++;
+
+	test->entry[n].test = test;
+	test->entry[n].n = n;
+	test->id[n] = mgmt_register(test->context->mgmt_client, data->opcode,
+					data->index, callback, &test->entry[n],
+					unregister_destroy);
+	g_assert_cmpuint(test->id[n], !=, 0);
+}
+
+static void unregister_check_alive(struct unregister_test *test)
+{
+	unsigned int i;
+
+	for (i = 0; i < test->entries; i++)
+		g_assert_cmpuint(test->destroyed[i], ==, 0);
+}
+
+static gboolean unregister_check_idle(gpointer user_data)
+{
+	struct unregister_test *test = user_data;
+	unsigned int i;
+
+	/* Entries unregistered while notifying are freed by now */
+	for (i = 0; i < test->entries; i++)
+		g_assert_cmpuint(test->destroyed[i], ==, test->removed[i]);
+
+	context_quit(test->context);
+
+	return FALSE;
+}
+
+static void unregister_from_cb(struct unregister_test *test, unsigned int n)
+{
+	struct mgmt *mgmt = test->context->mgmt_client;
+
+	g_assert_cmpint(mgmt_unregister(mgmt, test->id[n]), ==, true);
+	g_assert_cmpint(mgmt_unregister(mgmt, test->id[n]), ==, false);
+	test->removed[n] = true;
+
+	unregister_check_alive(test);
+}
+
+static void count_cb(uint16_t index, uint16_t length, const void *param,
+							void *user_data)
+{
+	struct unregister_entry *entry = user_data;
+
+	entry->test->called[entry->n]++;
+}
+
+static void unregister_self_cb(uint16_t index, uint16_t length,
+					const void *param, void *user_data)
+{
+	struct unregister_entry *entry = user_data;
+
+	if (!entry->test->called[entry->n]++)
+		unregister_from_cb(entry->test, entry->n);
+}
+
+static void unregister_next_cb(uint16_t index, uint16_t length,
+					const void *param, void *user_data)
+{
+	struct unregister_entry *entry = user_data;
+
+	if (!entry->test->called[entry->n]++)
+		unregister_from_cb(entry->test, entry->n + 1);
+}
+
+static void unregister_all_from_cb(uint16_t index, uint16_t length,
+					const void *param, void *user_data)
+{
+	struct unregister_entry *entry = user_data;
+	struct unregister_test *test = entry->test;
+	struct mgmt *mgmt = test->context->mgmt_client;
+	unsigned int i;
+
+	test->called[entry->n]++;
+
+	unregister_from_cb(test, 1);
+	g_assert_cmpint(mgmt_unregister_all(mgmt), ==, true);
+
+	for (i = 0; i < test->entries; i++)
+		test->removed[i] = true;
+
+	g_assert_cmpint(mgmt_unregister(mgmt, test->id[2]), ==, false);
+
+	unregister_check_alive(test);
+
+	/* No registered entry is left to schedule the check */
+	g_idle_add(unregister_check_idle, test);
+}
+
+static void unregister_quit_cb(uint16_t index, uint16_t length,
+					const void *param, void *user_data)
+{
+	struct unregister_entry *entry = user_data;
+	struct unregister_test *test = entry->test;
+
+	if (++test->called[entry->n] < test->events) {
+		unregister_check_alive(test);
+		return;
+	}
+
+	g_idle_add(unregister_check_idle, test);
+}
+
+static void unregister_run(struct unregister_test *test,
+				const struct command_test_data *data)
+{
+	unsigned int i;
+
+	for (i = 0; i < test->events; i++)
+		g_assert_cmpint(write(test->context->fd, data->cmd_data,
+					data->cmd_size), ==, data->cmd_size);
+
+	execute_context(test->context);
+
+	for (i = 0; i < test->entries; i++) {
+		g_assert_cmpuint(test->called[i], ==, test->expected[i]);
+		g_assert_cmpuint(test->destroyed[i], ==, 1);
+	}
+}
+
+static void test_unregister_self(gconstpointer data)
+{
+	/* Calls per entry over two events; the first unregisters itself */
+	struct unregister_test test = { .context = create_context(),
+					.events = 2, .expected = { 1, 2, 2 } };
+
+	unregister_add(&test, data, unregister_self_cb);
+	unregister_add(&test, data, count_cb);
+	unregister_add(&test, data, unregister_quit_cb);
+
+	unregister_run(&test, data);
+}
+
+static void test_unregister_next(gconstpointer data)
+{
+	/* Calls per entry over two events; the first unregisters the second */
+	struct unregister_test test = { .context = create_context(),
+					.events = 2, .expected = { 2, 0, 2 } };
+
+	unregister_add(&test, data, unregister_next_cb);
+	unregister_add(&test, data, count_cb);
+	unregister_add(&test, data, unregister_quit_cb);
+
+	unregister_run(&test, data);
+}
+
+static void test_unregister_id_all(gconstpointer data)
+{
+	/* Calls per entry; the first unregisters the second, then all */
+	struct unregister_test test = { .context = create_context(),
+					.events = 1, .expected = { 1, 0, 0 } };
+
+	unregister_add(&test, data, unregister_all_from_cb);
+	unregister_add(&test, data, count_cb);
+	unregister_add(&test, data, count_cb);
+
+	unregister_run(&test, data);
+}
+
+static void test_unregister_direct(gconstpointer data)
+{
+	/* Calls per entry; the first is unregistered before the event */
+	struct unregister_test test = { .context = create_context(),
+					.events = 1, .expected = { 0, 1 } };
+	struct mgmt *mgmt = test.context->mgmt_client;
+
+	unregister_add(&test, data, count_cb);
+	unregister_add(&test, data, unregister_quit_cb);
+
+	g_assert_cmpint(mgmt_unregister(mgmt, test.id[0]), ==, true);
+	g_assert_cmpuint(test.destroyed[0], ==, 1);
+	g_assert_cmpint(mgmt_unregister(mgmt, test.id[0]), ==, false);
+	test.removed[0] = true;
+
+	unregister_run(&test, data);
+}
+
 int main(int argc, char *argv[])
 {
 	g_test_init(&argc, &argv, NULL);
@@ -435,6 +652,14 @@ int main(int argc, char *argv[])
 							test_unregister_all);
 	g_test_add_data_func("/mgmt/unregister/2", &event_test_1,
 							test_unregister_index);
+	g_test_add_data_func("/mgmt/unregister/3", &event_test_1,
+							test_unregister_self);
+	g_test_add_data_func("/mgmt/unregister/4", &event_test_1,
+							test_unregister_next);
+	g_test_add_data_func("/mgmt/unregister/5", &event_test_1,
+							test_unregister_id_all);
+	g_test_add_data_func("/mgmt/unregister/6", &event_test_1,
+							test_unregister_direct);
 
 	g_test_add_data_func("/mgmt/destroy/1", &event_test_1, test_destroy);
 
-- 
2.43.0

RE: [BlueZ,1/2] shared/mgmt: Fix notify leak in mgmt_unregister()

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 1022 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/series/1181228/

---Test result---

Test Summary:
CheckPatch                    PASS      1.10 seconds
GitLint                       PASS      0.60 seconds
BuildEll                      PASS      20.38 seconds
BluezMake                     PASS      393.18 seconds
MakeCheck                     PASS      14.47 seconds
MakeDistcheck                 PASS      143.40 seconds
CheckValgrind                 PASS      235.53 seconds
CheckSmatch                   PASS      295.17 seconds
bluezmakeextell               PASS      95.34 seconds
TestFunctional                PASS      1142.46 seconds
IncrementalBuild              PASS      400.96 seconds
ScanBuild                     PASS      1150.52 seconds



https://github.com/bluez/bluez/pull/2634

---
Regards,
Linux Bluetooth

Re: [PATCH BlueZ 1/2] shared/mgmt: Fix notify leak in mgmt_unregister()

[patchwork-bot+bluetooth]

Hello:

This series was applied to bluetooth/bluez.git (master)
by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>:

On Thu,  8 Oct 2026 05:41:01 +0200 you wrote:
> mgmt_unregister() called from a notification callback takes the entry
> off notify_list and marks it removed instead of freeing it.
> process_notify() frees the removed entries still on notify_list once
> the walk is done, so this one is never freed and its destroy callback
> never runs.
> 
> If the callback unregisters the next entry rather than its own, this
> is also a use-after-free: queue_foreach() has already saved that
> entry's queue node, and queue_remove_if() frees it. That is the crash
> fixed for mgmt_unregister_index() by commit 872729a91632
> ("shared/mgmt: Fix crash when removing index"), still present in
> mgmt_unregister().
> 
> [...]

Here is the summary with links:
  - [BlueZ,1/2] shared/mgmt: Fix notify leak in mgmt_unregister()
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=ae23df052290
  - [BlueZ,2/2] unit/test-mgmt: Test unregistering from callbacks
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=6f5eeb402d3b

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html