Fix 4 critical correctness bugs in daemon server#3
Merged
Conversation
Phase 1 release blockers: negative search context crash (DoS via makeslice panic), handleKill blocking the entire daemon under mutex, PTY double-close race between captureOutput and stop/kill/shutdown, and data race on s.listener between Start and Shutdown. - Validate before/after non-negative at server, CLI, and MCP boundaries - Make handleKill async (mirror handleStop pattern, release lock before process cleanup) - Introduce ptyHandle with sync.Once to guarantee single PTY close - Guard s.listener nil check in Start() with mutex
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.
Bug
The daemon server has four correctness bugs that can cause crashes, hangs, or undefined behavior under normal operation. Negative search context values panic the daemon (DoS vector for any client including MCP agents). The kill handler blocks the entire daemon under mutex while waiting for process death. Multiple code paths can double-close PTY file descriptors. A data race exists on
s.listenerbetweenStart()andShutdown().Solution
All four issues are fixed with minimal, targeted changes. Input validation rejects negative
before/aftervalues at three boundaries (server, CLI, MCP). The kill handler releases the mutex before async process cleanup, mirroring the existinghandleStoppattern. AptyHandlewrapper withsync.Onceguarantees each PTY fd is closed exactly once regardless of which code path runs first. The listener nil-check inStart()now runs under the mutex to matchShutdown().Changes
Search validation (
server.go,cmd/search.go,internal/mcp/tools.go):before < 0 || after < 0before touching any state--aroundexpansionaroundexpansionKill handler (
server.go):handleKillcaptures process reference under lock, releases lock, then spawns goroutine for SIGTERM/SIGKILL/WaitPTY double-close (
server.go):ptyHandletype wraps*os.Filewithsync.Oncecloses.ptysmap stores*ptyHandleinstead of*os.FilecaptureOutput,handleStop,handleKill,Shutdown,handleSnapshot,handleSend,handleResize) use the handleListener race (
server.go):s.listener == nilcheck inStart()accept loop now holdss.mu