Skip to content

fix(agent/agentproc): read process info before output to prevent TOCTOU#25646

Merged
mafredri merged 1 commit into
mainfrom
fix/codagt-399-output-exceeds-buffer
May 25, 2026
Merged

fix(agent/agentproc): read process info before output to prevent TOCTOU#25646
mafredri merged 1 commit into
mainfrom
fix/codagt-399-output-exceeds-buffer

Conversation

@mafredri
Copy link
Copy Markdown
Member

@mafredri mafredri commented May 25, 2026

handleProcessOutput read proc.output() then proc.info() using separate locks (buf.mu vs proc.mu). Between the two reads, the exit goroutine could finish I/O and set running=false, pairing a stale buffer snapshot with final process status.

On Windows CI this caused TestProcessLifecycle/OutputExceedsBuffer to flake: the buffer was captured mid-write (OmittedBytes=0) while info reported the process as exited.

Swap the read order: info() first, then output(). The exit goroutine completes cmd.Wait() (draining all pipe data) before setting running=false, so Running=false guarantees the subsequent output read reflects the final buffer state.

Proof: dlv-controlled TOCTOU reproduction

Used dlv to freeze execution between the two reads and mutate process state, proving the race under controlled conditions.

Buggy order (output-first, breakpoint between reads):

=> 204:    info := proc.info()   // output() already returned

(dlv) print truncated
  OmittedBytes: 0          // stale: captured before mutation
(dlv) print proc.running
  true
(dlv) print proc.buf.totalBytes
  96

# Simulate exit goroutine via dlv:
(dlv) call proc.buf.Write(...)
(dlv) set proc.running = false
(dlv) print proc.buf.totalBytes
  500

# Continue -> info() returns Running=false
# Response: Running=false, OmittedBytes=0  <- TOCTOU proven

Fixed order (info-first, breakpoint between reads):

=> 217:    output, truncated := proc.output()  // info() already returned

(dlv) print info
  Running: true            // captured before mutation

# Same mutation via dlv
# Continue -> output() returns fresh buffer
# Response: Running=true, OmittedBytes=372  <- safe, client polls again

Closes https://linear.app/codercom/issue/CODAGT-399

🤖 This PR was created with the help of Coder Agents, and will be reviewed by a human. 🏂🏻

handleProcessOutput read proc.output() then proc.info() using
separate locks. Between the two reads the exit goroutine could
finish I/O and set running=false, pairing stale output with final
status. On Windows CI this caused OutputExceedsBuffer to flake
when the buffer snapshot caught mid-write data (OmittedBytes=0)
but info reported the process as exited.

Swap the read order so info is read first. The exit goroutine
completes cmd.Wait (draining all pipe data) before setting
running=false, so seeing Running=false guarantees the subsequent
output read reflects the final buffer state.
@mafredri mafredri force-pushed the fix/codagt-399-output-exceeds-buffer branch from 9e61962 to 6551f3d Compare May 25, 2026 14:03
@mafredri mafredri marked this pull request as ready for review May 25, 2026 14:05
@mafredri mafredri requested a review from johnstcn May 25, 2026 14:05
@coder coder deleted a comment from coder-tasks Bot May 25, 2026
@mafredri mafredri merged commit c8359d8 into main May 25, 2026
35 checks passed
@mafredri mafredri deleted the fix/codagt-399-output-exceeds-buffer branch May 25, 2026 14:27
@github-actions github-actions Bot locked and limited conversation to collaborators May 25, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants