Skip to content

feat(allowedpaths): guarantee Close on context expiry to prevent fd leaks#157

Merged
thieman merged 3 commits intomainfrom
thieman/context-close-fd-safety
Mar 26, 2026
Merged

feat(allowedpaths): guarantee Close on context expiry to prevent fd leaks#157
thieman merged 3 commits intomainfrom
thieman/context-close-fd-safety

Conversation

@thieman
Copy link
Copy Markdown
Collaborator

@thieman thieman commented Mar 26, 2026

Summary

  • Adds `WithContextClose(ctx, f)` in `allowedpaths/context_file.go` — wraps an `io.ReadWriteCloser` so `Close()` is called automatically when the context expires, preventing fd leaks if a caller omits an explicit close
  • Wires it into both `OpenFile` hook sites in `interp/runner_exec.go` (child builtin path and main builtin path)
  • The underlying file is closed at most once regardless of whether the context fires or the caller calls `Close()` explicitly — two `sync.Once` guards prevent double-close
  • Existing `defer f.Close()` calls in builtin implementations are untouched; this is purely a safety backstop

Read-unblock behaviour by file type

Linux unit tests (`context_file_linux_test.go`) verify two things per file type: (1) that `Close()` is guaranteed to be called on context cancellation, and (2) whether a goroutine blocked in `Read()` is unblocked when the context is cancelled.

Go registers FIFOs, sockets, and character devices with its epoll poller. When `Close()` is called, the runtime calls `evict()` which wakes any goroutine blocked on that fd. Regular files are not registered with epoll but never block, so they are safe regardless.

Non-blocking file types — Read returns immediately

File type CI result Read outcome
Regular file ✅ PASS Returns data then EOF immediately
`/dev/null` ✅ PASS Returns `(0, EOF)` immediately
`/dev/zero` ✅ PASS Returns zeros immediately
`/dev/urandom` ✅ PASS Returns random bytes immediately
Symbolic link → regular file ✅ PASS Identical to regular file
Directory ✅ PASS Returns `EISDIR` immediately
Block device (`/dev/sda` etc.) ⏭️ SKIP No accessible block device in GH Actions; expected to return block data immediately

Blocking file types — Read blocks; context cancel must unblock it

File type CI result Observed error after cancel
Named pipe / FIFO ✅ PASS `"file already closed"` — epoll `evict()` woke the goroutine
Unix socket (`net.Pipe()`) ✅ PASS Unblocked cleanly
Filesystem Unix socket (`/tmp/*.sock`) ✅ PASS Unblocked cleanly
`/dev/random` ✅ PASS Didn't block (Linux ≥ 5.6 behaves like `/dev/urandom`)
`/dev/tty` ⏭️ SKIP No controlling terminal in GH Actions; expected to unblock via epoll evict

Test plan

  • `TestWithContextClose_ExplicitClose` — explicit close works normally
  • `TestWithContextClose_ExplicitCloseIdempotent` — multiple explicit closes are safe
  • `TestWithContextClose_ContextCancellation` — file is closed when context is cancelled
  • `TestWithContextClose_ContextCancelledBeforeOpen` — file is closed when context is already done at wrap time
  • `TestWithContextClose_ContextCancelThenExplicitClose_ClosedOnce` — only one close after both paths fire
  • `TestWithContextClose_ErrorPropagated` — error from explicit Close is returned to caller
  • `TestWithContextClose_GoroutineExitsOnExplicitClose` — no goroutine leak when context is never cancelled
  • Linux file-type matrix tests — all ran or skipped with justification
  • Full `./interp/... ./tests/...` suite passes on Linux, macOS, Windows

🤖 Generated with Claude Code

thieman and others added 2 commits March 26, 2026 11:21
…eaks

Add WithContextClose wrapper that ensures any file opened via the
sandbox's OpenFile hook has Close() called when the call's context
expires. A background goroutine waits for ctx.Done() or an explicit
Close, whichever comes first. The underlying file is closed at most
once (sync.Once guards). Existing defer f.Close() calls in builtin
implementations are untouched — this is a safety backstop, not a
replacement for explicit cleanup.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…s allowlist

context_file.go in allowedpaths/ uses these two symbols for the
WithContextClose wrapper. They were missing from the allowlist,
causing TestAllowedPathsAllowedSymbols to fail on CI.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…file types

Adds context_file_linux_test.go covering Read-unblock behaviour for:

Non-blocking (Read returns immediately — verify no hang):
  - Regular file
  - /dev/null       (returns EOF)
  - /dev/zero       (returns zeros immediately)
  - /dev/urandom    (returns random bytes immediately)
  - Symbolic link → regular file
  - Directory       (returns EISDIR immediately)
  - Block device    (skipped if none accessible)

Blocking (Read blocks until Close — verify context cancel unblocks):
  - Named pipe / FIFO (O_RDWR, empty buffer)
  - net.Pipe() socketpair (Unix socket, no sender)
  - /dev/random     (may block on low-entropy kernels)
  - /dev/tty        (skipped if unavailable in CI)
  - Filesystem Unix domain socket (/tmp sock, server conn with no data)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@thieman thieman marked this pull request as ready for review March 26, 2026 16:11
@thieman thieman added this pull request to the merge queue Mar 26, 2026
Merged via the queue into main with commit 8d8e3c0 Mar 26, 2026
34 checks passed
@thieman thieman deleted the thieman/context-close-fd-safety branch March 26, 2026 18:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants