Fix crashing and leak#79
Closed
gabsow wants to merge 5 commits intogal-13438-internal-commands-protocolfrom
Closed
Fix crashing and leak#79gabsow wants to merge 5 commits intogal-13438-internal-commands-protocolfrom
gabsow wants to merge 5 commits intogal-13438-internal-commands-protocolfrom
Conversation
- Check for empty task list (head == NULL) and return to avoid null deref - Check for null task and return - Hold execution ref before unlock so it cannot be freed by another thread (e.g. MR_DisposeExecution) while we read task->callback and run callback - Release ref on both exit paths (finalizing callback and normal path) Co-authored-by: Cursor <cursoragent@cursor.com>
When the callback is MR_DisposeExecution or MR_ExecutionTimedOutInternal, release our ref after the callback and free the execution if refCount reaches 0 (we are the last holder). Extract actual free logic into MR_FreeExecutionInternal so both MR_FreeExecution and MR_ExecutionMain can invoke it when refCount drops to zero. Co-authored-by: Cursor <cursoragent@cursor.com>
… loop vs worker) When the event loop runs MR_DeleteExecution(e), it removes e from the dict then calls MR_ExecutionAddTask(e, MR_DisposeExecution, NULL). A worker thread in MR_ExecutionMain can free e (refCount 0) in between, so the event loop then touches freed e in MR_ExecutionAddTask (crash at 0x28). Hold a ref in MR_DeleteExecution across the dict delete and add-task so e cannot be freed until we are done; release the ref after adding the task. Co-authored-by: Cursor <cursoragent@cursor.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| mr_dictDelete(mrCtx.executionsDict, e->id); | ||
| /* Send dispose execution task, this will be last task this execution will ever receive. */ | ||
| MR_ExecutionAddTask(e, MR_DisposeExecution, NULL); | ||
| __atomic_sub_fetch(&e->refCount, 1, __ATOMIC_RELAXED); |
There was a problem hiding this comment.
Missing free when refCount reaches zero causes memory leak
High Severity
In MR_DeleteExecution, the __atomic_sub_fetch at line 990 doesn't check if refCount reached zero. If a thread pool worker picks up and fully completes the dispose task between MR_ExecutionAddTask returning and this sub_fetch executing, refCount drops to zero here with no call to MR_FreeExecutionInternal, causing a memory leak of the entire Execution and all its owned resources. The return value of the sub_fetch needs to be checked, similar to how MR_ExecutionMain handles it at line 1330.
MR_DeleteExecution can be scheduled via MR_EventLoopAddTask. By the time the event loop runs it, a worker in MR_ExecutionMain may have freed e (refCount 0), so MR_DeleteExecution(e) then touches freed e in MR_ExecutionAddTask (crash at 0x28 on macOS 26). Take a ref at the two schedule sites (MR_RunExecution for Local execution, MR_ExecutionCtxSetDone) so e stays alive until the scheduled task runs. MR_DeleteExecution releases that ref with sub_fetch at the end. Co-authored-by: Cursor <cursoragent@cursor.com>
…xecution only releases Remove the add-ref at start of MR_DeleteExecution; it didn't help when the task was scheduled (e could already be freed when the task ran). Rely on callers to hold a ref: direct callers (MR_DropExecution, MR_NotifyDone path) now take a ref before MR_DeleteExecution; schedule sites already did. So MR_DeleteExecution just does dictDelete, AddTask, sub_fetch. Co-authored-by: Cursor <cursoragent@cursor.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.
Note
Medium Risk
Touches
Executionlifetime/refcounting across the event loop and worker threads; mistakes here could cause use-after-free or leaks, but the change is localized to teardown/scheduling paths.Overview
Hardens
Executionlifetime management by adding explicit refcount ownership rules aroundMR_DeleteExecutionand all paths that schedule deletion (e.g.,MR_DropExecution,MR_NotifyDone, local completion inMR_RunExecution, andMR_ExecutionCtxSetDone).Prevents worker-thread crashes and premature frees by holding a ref while executing task callbacks in
MR_ExecutionMain, adding null-guard checks for empty task queues, and splitting cleanup intoMR_FreeExecutionInternalfor safe final disposal when the refcount reaches zero.Written by Cursor Bugbot for commit 3e30af5. This will update automatically on new commits. Configure here.