Fix use-after-free in process scheduling lists#2146
Open
petermm wants to merge 1 commit intoatomvm:mainfrom
Open
Fix use-after-free in process scheduling lists#2146petermm wants to merge 1 commit intoatomvm:mainfrom
petermm wants to merge 1 commit intoatomvm:mainfrom
Conversation
When `do_spawn` fails after `context_new` has been called, `context_destroy` is invoked directly. `context_new` calls `globalcontext_init_process` which adds the context to both `processes_table` and `waiting_processes`, but `context_destroy` only removed from `processes_table`. The freed context's node remained linked in `waiting_processes`, causing a use-after-free corruption of the scheduling list. Fix by adding a spinlock-protected `list_remove` + `list_init` of `processes_list_head` in `context_destroy`. The `list_init` makes the node self-referential so that a second `list_remove` (from callers like `scheduler_terminate` that already do the removal before calling `context_destroy`) is a safe no-op. Also remove the now-redundant explicit removal in the native handler kill path of `scheduler_run`, since `context_destroy` handles it. Signed-off-by: Peter M <petermm@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
While reviewing #2139 AI found below:
https://ampcode.com/threads/T-019c9441-412f-768c-83d5-9f8982eb753c#message-21-block-0
So AI slop/hallucination danger, but it did call it a "time bomb" when I asked how severe it was - would only happen under memory pressure..
When
do_spawnfails aftercontext_newhas been called,context_destroyis invoked directly.context_newcallsglobalcontext_init_processwhich adds the context to bothprocesses_tableandwaiting_processes, butcontext_destroyonly removed fromprocesses_table. The freed context's node remained linked inwaiting_processes, causing a use-after-free corruption of the scheduling list.Fix by adding a spinlock-protected
list_remove+list_initofprocesses_list_headincontext_destroy. Thelist_initmakes the node self-referential so that a secondlist_remove(from callers likescheduler_terminatethat already do the removal before callingcontext_destroy) is a safe no-op.Also remove the now-redundant explicit removal in the native handler kill path of
scheduler_run, sincecontext_destroyhandles it.These changes are made under both the "Apache 2.0" and the "GNU Lesser General
Public License 2.1 or later" license terms (dual license).
SPDX-License-Identifier: Apache-2.0 OR LGPL-2.1-or-later