[PATCH BlueZ v1] shared/timeout: Fix use-after-free when a timeout removes itself

[PATCH BlueZ v1] shared/timeout: Fix use-after-free when a timeout removes itself

[Luiz Augusto von Dentz]

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

If the callback of a timeout_add() timer removes it, e.g. the tester
test timeout restarting it for teardown, its data is freed before
timeout_callback() reads the id, and the id may already be reused by a
timeout added in the callback, which then gets removed instead.

Keep the data while the callback runs and free it afterwards if it was
removed, and add a unit test for it.

Fixes: efe0efc925db ("shared/timeout: Add timeout_add_seconds abstraction")
Assisted-by: OpenCode:claude-opus-5.5 valgrind
---
 src/shared/timeout-mainloop.c | 23 +++++++++++++++++++--
 unit/test-mainloop.c          | 38 +++++++++++++++++++++++++++++++++++
 2 files changed, 59 insertions(+), 2 deletions(-)

diff --git a/src/shared/timeout-mainloop.c b/src/shared/timeout-mainloop.c
index e6f9c5e826b3..3dfcdee8143d 100644
--- a/src/shared/timeout-mainloop.c
+++ b/src/shared/timeout-mainloop.c
@@ -20,14 +20,28 @@ struct timeout_data {
 	timeout_destroy_func_t destroy;
 	unsigned int timeout;
 	void *user_data;
+	bool running;
+	bool removed;
 };
 
 static void timeout_callback(int id, void *user_data)
 {
 	struct timeout_data *data = user_data;
+	bool ret;
 
-	if (data->func(data->user_data) &&
-			!mainloop_modify_timeout(data->id, data->timeout))
+	data->running = true;
+	ret = data->func(data->user_data);
+	data->running = false;
+
+	/* The callback removed the timeout, so data was only kept around to
+	 * be checked here and its id may already be reused.
+	 */
+	if (data->removed) {
+		free(data);
+		return;
+	}
+
+	if (ret && !mainloop_modify_timeout(data->id, data->timeout))
 		return;
 
 	mainloop_remove_timeout(data->id);
@@ -40,6 +54,11 @@ static void timeout_destroy(void *user_data)
 	if (data->destroy)
 		data->destroy(data->user_data);
 
+	if (data->running) {
+		data->removed = true;
+		return;
+	}
+
 	free(data);
 }
 
diff --git a/unit/test-mainloop.c b/unit/test-mainloop.c
index 70ee03ee4283..b876e0285907 100644
--- a/unit/test-mainloop.c
+++ b/unit/test-mainloop.c
@@ -12,6 +12,7 @@
 #include <stdlib.h>
 
 #include "src/shared/mainloop.h"
+#include "src/shared/timeout.h"
 
 
 #define test_failed_msg(msg) \
@@ -153,6 +154,42 @@ static void test_timeout(struct test_data *data)
 	check(data->destroys == 1);
 }
 
+static unsigned int self_remove_id;
+
+static bool readded_callback(void *user_data)
+{
+	struct test_data *data = user_data;
+
+	data->new_calls++;
+	mainloop_quit();
+
+	return false;
+}
+
+static bool self_remove_callback(void *user_data)
+{
+	struct test_data *data = user_data;
+
+	data->old_calls++;
+	timeout_remove(self_remove_id);
+	check(data->destroys == 1);
+	check(timeout_add(1, readded_callback, data, destroy) > 0);
+
+	return false;
+}
+
+static void test_timeout_remove_self(struct test_data *data)
+{
+	mainloop_init();
+
+	self_remove_id = timeout_add(1, self_remove_callback, data, destroy);
+	check(self_remove_id > 0);
+	check(mainloop_run() == EXIT_SUCCESS);
+	check(data->old_calls == 1);
+	check(data->new_calls == 1);
+	check(data->destroys == 2);
+}
+
 #define define_test(name, function)				\
 	do {							\
 		static struct test_data data = {};		\
@@ -167,5 +204,6 @@ int main(int argc, char *argv[])
 	define_test("Test Modify File Descriptor", test_modify_fd);
 	define_test("Test Re-Add Other Callback", test_readd_other_callback);
 	define_test("Test Timeout", test_timeout);
+	define_test("Test Timeout Remove Self", test_timeout_remove_self);
 	return 0;
 }
-- 
2.56.0

RE: [BlueZ,v1] shared/timeout: Fix use-after-free when a timeout removes itself

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 3311 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/1197185/

---Test result---

Test Summary:
CheckPatch                    PASS      0.34 seconds
GitLint                       PASS      0.21 seconds
BuildEll                      PASS      21.82 seconds
BluezMake                     PASS      314.02 seconds
MakeCheck                     PASS      14.62 seconds
MakeDistcheck                 PASS      149.26 seconds
CheckValgrind                 PASS      237.84 seconds
CheckSmatch                   PASS      298.50 seconds
bluezmakeextell               PASS      99.67 seconds
TestFunctional                FAIL      1202.82 seconds
IncrementalBuild              PASS      325.06 seconds
ScanBuild                     PASS      1181.22 seconds

Details
##############################
Test: TestFunctional - FAIL
Desc: Run test-functional
Output:
FAIL functional.test_bluetoothctl::test_bluetoothctl_pair_bredr[hosts15-vm2]: pytest_bluezenv.rpc.RemoteError: <pexpect.popen_spawn.PopenSpawn object at 0x7feaa5592510>
    searcher: searcher_re:
        0: re.compile(b'Changing pairable on succeeded')
    <pexpect.popen_spawn.PopenSpawn object at 0x7feaa5592510>
    searcher: searcher_re:
        0: re.compile(b'Changing pairable on succeeded')
    Remote traceback:
        Traceback (most recent call last):
          File "/usr/local/lib/python3.14/dist-packages/pytest_bluezenv/../pytest_bluezenv/rpc.py", line 102, in server_stream
            result = method(*msg["a"], **msg["kw"])
          File "/usr/local/lib/python3.14/dist-packages/pytest_bluezenv/../pytest_bluezenv/env.py", line 256, in call_plugin
            return getattr(self.plugins[name], method)(*a, **kw)
                   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^
          File "/usr/local/lib/python3.14/dist-packages/pytest_bluezenv/../pytest_bluezenv/host_plugins.py", line 502, in expect
            ret = self.ctl.expect(*a, **kw)
          File "/usr/lib/python3/dist-packages/pexpect/spawnbase.py", line 354, in expect
            return self.expect_list(compiled_pattern_list,
                   ~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^
                    timeout, searchwindowsize, async_)
                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
          File "/usr/lib/python3/dist-packages/pexpect/spawnbase.py", line 383, in expect_list
            return exp.expect_loop(timeout)
                   ~~~~~~~~~~~~~~~^^^^^^^^^
          File "/usr/lib/python3/dist-packages/pexpect/expect.py", line 181, in expect_loop
            return self.timeout(e)
                   ~~~~~~~~~~~~^^^
          File "/usr/lib/python3/dist-packages/pexpect/expect.py", line 144, in timeout
            raise exc
        pexpect.exceptions.TIMEOUT: <pexpect.popen_spawn.PopenSpawn object at 0x7feaa5592510>
        searcher: searcher_re:
            0: re.compile(b'Changing pairable on succeeded')
        <pexpect.popen_spawn.PopenSpawn object at 0x7feaa5592510>
        searcher: searcher_re:
            0: re.compile(b'Changing pairable on succeeded')


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

---
Regards,
Linux Bluetooth