- Status Closed
- Percent Complete
- Task Type Bug Report
- Category Base system
-
Assigned To
Petr Štetiar - Operating System All
- Severity Low
- Priority Very Low
- Reported Version Trunk
- Due in Version Undecided
-
Due Date
Undecided
- Private
Opened by Chris Nisbet - 18.04.2020
Last edited by Petr Štetiar - 21.05.2020
FS#3016 - libubox: read of freed memory in runqueue_task_kill()
I believe there is a use-after-free bug in runqueue.c:runqueue_task_kill().
If a task is killed by calling runqueue_task_kill(), the ‘complete’ callback will be called when runqueue_task_complete() is called, which, in the case of test-runqueue.c at least, is where resources allocated for the task would normally be freed (see q_sleep_complete()).
So, after runqueue_task_kill() calls runqueue_task_complete(), the pointer ‘t’ points to freed memory. However, the very next line in runqueue_task_kill() reads from this memory to see if there is a ‘kill’ callback.
I have confirmed that this occurs by modifying test-runqueue.c slightly so that a 20ms timer is started when the task is added. When the timer expires and the timer callback is called it calls runqueue_task_kill() to kill the task. When running ‘make test’ a failed test occurs and generates the following output...
[0/1] finish 'sleep 1' ==19171== Invalid read of size 8 ==19171== at 0x4850F47: runqueue_task_kill (runqueue.c:200) ==19171== by 0x484EDB1: uloop_process_timeouts (uloop.c:505) ==19171== by 0x484EDB1: uloop_run_timeout (uloop.c:542) ==19171== by 0x10933E: uloop_run (uloop.h:111) ==19171== by 0x10933E: main (test-runqueue.c:126) ==19171== Address 0x4a5f058 is 24 bytes inside a block of size 208 free'd ==19171== at 0x483BA3F: free (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so) ==19171== by 0x4850EAA: runqueue_task_complete (runqueue.c:234) ==19171== by 0x4850F42: runqueue_task_kill (runqueue.c:199) ==19171== by 0x484EDB1: uloop_process_timeouts (uloop.c:505) ==19171== by 0x484EDB1: uloop_run_timeout (uloop.c:542) ==19171== by 0x10933E: uloop_run (uloop.h:111) ==19171== by 0x10933E: main (test-runqueue.c:126) ==19171== Block was alloc'd at ==19171== at 0x483CD99: calloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so) ==19171== by 0x10961D: add_sleeper.constprop.0 (test-runqueue.c:101) ==19171== by 0x10932C: main (test-runqueue.c:123) ==19171==
Please find attached the modified test-runqueue.c file I used to generate the test output.
test-runqueue.c
(3.1 KiB)
21.05.2020 14:06
Reason for closing: Fixed
Additional comments about closing:
Fixed via https:/ /git.openwrt.org/89fb6136ad7484e4e8f9b61 8e530e098cf573665
Can you please check this patch https://patchwork.ozlabs.org/project/openwrt/patch/20190621153828.20699-1-albeu@free.fr/ ?
That patch seems to sort things out for me. Valgrind is happy.
I've given the change a review with my inexpert eye, and it seems to be doing all the right things.
One change I'd suggest is to remove the variable 'running'.
Then
becomes
One other change I'd suggest is to fix up the comment above the 'kill' field of struct runqueue_task_type in runqueue.h.
With this patch applied, it has _not_ already been removed from the list.
I'd suggest something like ...
On the subject of these comments, the comment relating to the 'cancel' field seems a bit unclear. It might be better if the final sentence says
Can you take over that original patch, add your proposed changes including your nice reproducer/testcase in tests/test-runqueue.c (I've just verified it, works fine here as well), add your Signed-off-by: and send it as described in https://openwrt.org/submitting-patches (adding original author to Cc: loop) ?
Original patch author seems to lost interest. Thanks.