FS#3016 - libubox: read of freed memory in runqueue_task_kill() #7866
Comments
|
ynezz: Can you please check this patch https://patchwork.ozlabs.org/project/openwrt/patch/20190621153828.20699-1-albeu@free.fr/ ? |
|
ChrisNisbet01: That patch seems to sort things out for me. Valgrind is happy.
if (running && t->type->kill)
becomes
if (t->running && t->type->kill)
One other change I'd suggest is to fix up the comment above the 'kill' field of struct runqueue_task_type in runqueue.h.
/*
* called to kill a task. must not make any calls to runqueue_task_complete,
* it has already been removed from the list.
*/
With this patch applied, it has not already been removed from the list.
/*
* called to kill a task. must not make any calls to runqueue_task_complete,
* which will be called after this returns.
*/
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 |
|
ynezz:
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) ? |
|
ynezz: Original patch author seems to lost interest. Thanks. |
ChrisNisbet01:
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.The text was updated successfully, but these errors were encountered: