New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix use after free in core. #12138
Fix use after free in core. #12138
Conversation
Codecov Report
@@ Coverage Diff @@
## master #12138 +/- ##
==========================================
- Coverage 37.16% 37.15% -0.01%
==========================================
Files 902 902
Lines 288475 288475
==========================================
- Hits 107210 107185 -25
- Misses 181265 181290 +25
Continue to review full report at Codecov.
|
Cc @thestr4ng3r |
libr/core/task.c
Outdated
@@ -182,6 +182,7 @@ static void task_free (RCoreTask *task) { | |||
r_th_lock_free (task->dispatch_lock); | |||
r_cons_context_free (task->cons_context); | |||
free (task); | |||
task = NULL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the point of setting the parameter to NULL?
libr/core/core.c
Outdated
@@ -2450,7 +2450,6 @@ R_API RCore *r_core_fini(RCore *c) { | |||
r_list_free (c->files); | |||
r_list_free (c->watchers); | |||
r_list_free (c->scriptstack); | |||
r_list_free (c->tasks); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The free function saved in core->tasks is r_core_task_decref, freeing the list should just decrease the refcount for each task as you would expect. If this leads to a uaf in some case, the problem is somewhere else, maybe somewhere the refcount is not increased where it should. If you have a case where this happens, please elaborate.
Also, you still have to free the RList itself or it will leak.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you re right this was the wrong approach but there is genuinely an UAF case. With the other diff you can see why there is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you are absolutely right. Thanks for noticing, the fix is now correct.
The tasks are already freed by the reference counting.