Repository navigation
Conversation
gonet's LocalAddr and RemoteAddr return nil once the connection is no
longer connected. If the client resets a connection right after the
handshake, before netstack calls the OnTCP handler, ExecConnHandler
passed those nil addresses to PeerEnv, whose remote.String() panicked
and took the whole server down with every other connection on it:
panic: runtime error: invalid memory address or nil pointer dereference
tailcat.(*Server).PeerEnv tailcat_exec.go:49
tailcat.(*Server).ExecConnHandler.func1 tailcat_exec.go:29
netstack.(*Impl).acceptTCP
ExecConnHandler now closes such a connection without running the
command, and PeerEnv leaves out an address it doesn't have (an SSH
session's connection can be reset at any time too).
Also, as net/http does per connection, recover a panic in an OnTCP or
OnTCPForward handler: log it with its stack and close that connection,
so one bad flow can't kill the server.
Signed-off-by: Kirill <kirill2003de@gmail.com>
This branch has not been deployed
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.
This PR was opened by an AI agent at the direct request of @bra1nDump.
Happy (slopus/happy-agent) ships tailcat as its computer-side transport: the desktop agent runs
tailcatin serve mode with-efor each connection, and a phone connects to it. We hit this crash while testing that setup.The bug
If a client resets a TCP flow right after the handshake, before netstack calls the handler, gonet's
RemoteAddr()andLocalAddr()return nil.ExecConnHandlerpasses them toPeerEnv, andremote.String()panics attailcat_exec.go:49. The panic kills the whole tailcat process, along with every other flow and session on it.Two crashes in one evening, from our daemon's log of tailcat's stderr (v0.7.0, darwin/arm64; the second has the same stack):
Repro
TestExecConnHandlerResetBeforeServeddoes a real gVisor handshake, has the clientAbort()(RST), waits until the server side reports a nilRemoteAddr(), then callsExecConnHandler.TestServerSurvivesHandlerPanicstarts a realServerwhose port-81 handler panics, dials 81, then checks that port 80 still answers.Before the fix (main e00cdde with only the new tests), each test kills the test binary:
After:
go test ./...passes on this branch (linux/amd64).Impact
A crash isn't contained to the flow that triggered it. Our desktop agent restarts tailcat, which comes back on a new UDP port and without its sessions, so every connected client has to notice and rejoin. In our test harness (the server process killed and restarted while the phone app was suspended, the same effect as this panic), the phone's requests on the old session hung until they timed out after ~35 s, in 7 of 7 trials (36.6 s median). Once our client re-invites after a restart, recovery takes about 0.3 s (205–348 ms, 8 of 8). Without the crash, neither happens.
gantt title tailcat restarted while the phone is suspended (ms from phone start) dateFormat x axisFormat %M:%S section Server killed, back on a new UDP port :done, 1704, 2735 section Phone, old session suspended :done, 527, 30532 5 requests hang, then time out :crit, 30532, 65896 retry, new session, done :66905, 67461 section Phone that re-invites suspended :done, 585, 30585 re-invited, direct, done :30585, 30913The fix
ExecConnHandlercloses a connection that no longer has addresses without running the command.PeerEnvleaves out a nil address instead of callingString()on it (an SSH session's connection can be reset at any time too).net/httpdoes per connection, a panic in anOnTCPorOnTCPForwardhandler is recovered, logged with its stack, and closes only that connection.