[Fix] Avoid double-free in overlap_loop if Req is aborted while finishing - #111
Open
MisakaVan wants to merge 1 commit into
Open
[Fix] Avoid double-free in overlap_loop if Req is aborted while finishing#111MisakaVan wants to merge 1 commit into
MisakaVan wants to merge 1 commit into
Conversation
…aneously In overlap_loop, when a request is aborted via AbortBackendMsg and also finishes (EOS) in the same iteration, _free_req_resources was called twice: once in the abort path and once in the finish path. The abort path did not add the req to finished_reqs, so the finish path did not skip it. The fix defers abort resource freeing: instead of calling _free_req_resources immediately in the abort path, we queue the req in pending_aborts and free it in _free_pending_aborts which runs after _process_last_data populates finished_reqs, so already-finished reqs are properly skipped.
Contributor
Author
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
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.
Fix double-free (and what appears to be a UAF) race condition in
Scheduler.overlap_loop.Problem
The
_process_one_msgmay abort aReqthat is in-flight (inlast_data), thus freeing its corresponding resource (thetable_idx). Then such race conditions may happen:Reqis finishing (e.g. just decoded an EOS token), it will be double-freed inself._process_last_data(last_data).forward_input = self._schedule_next_batch()assigns a new prefill Req/ChunkedReq, it may be assigned the sametable_idxjust freed, while the originalReqis still in-flight (to be handled by_process_last_data). While this is actually not a UAF condition since_process_last_dataonly calls cpu-sideReq.append_host, it still creates unstable semantic where two livingReqobjects are sharing the sametable_idxand makes the code vulnerable.Fix
The fix defers aborted-request's resource freeing until after
_process_last_data(which populates finished_reqs and finishes the previous in-flight batch)._process_one_msgjust removes theReqfrom the managers so the request won't be scheduled for the next batch.To-be-aborted reqs are queued in pending_aborts. The freeing procedure skips any reqs just finished or in-flight.
Test
This commit also contains a testcase of the double-free scenario. The testcase fails without the fix.