fix: guard sess.pty against concurrent access - #59
Open
owenthereal wants to merge 1 commit into
Open
Conversation
Pty() copies *sess.pty while handleRequests writes sess.pty.Window in the window-change case. Neither took the mutex the session already embeds and uses for sigCh and breakCh. The handler goroutine is started from the shell case and handleRequests keeps running, so a client that resizes right after starting its shell writes sess.pty.Window while the handler reads it. Pty() is the only way for a handler to obtain the window channel, so any handler that supports resizing is exposed. Takes the lock in both places. The send on sess.winch stays outside it: winch has a capacity of one, so holding the lock across the send would let a slow consumer block every Pty() behind it. pty-req writes the same field but cannot race a handler, since it is refused once sess.handled is set and handlers only start after that, so it is left alone. TestPtyResize does not catch this because it waits for the first window delivery before resizing, which orders the two accesses. The new test resizes without synchronising against the handler; it reports a race on every run before this change and none after.
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.
Fixes #58.
Pty()copies*sess.ptywhilehandleRequestswritessess.pty.Windowin thewindow-changecase. Neither took the mutex the session already embeds and uses forsigChandbreakCh.The handler goroutine is started from the
shellcase andhandleRequestskeeps running, so a client that resizes right after starting its shell writessess.pty.Windowwhile the handler reads it.Pty()is the only way for a handler to obtain the window channel, so any handler that supports resizing is exposed.Takes the lock in both places. The send on
sess.winchstays outside it:winchhas a capacity of one, so holding the lock across the send would let a slow consumer block everyPty()behind it.pty-reqwrites the same field but cannot race a handler, since it is refused oncesess.handledis set and handlers only start after that, so it is left alone.TestPtyResizedoes not catch this because it waits for the first window delivery before resizing, which orders the two accesses. The newTestPtyResizeRaceresizes without synchronising against the handler.Verified with
go test -race, 5 runs each:TestPtyResizeRaceFull suite passes with
-race.