From c6acfbc94ba17b477e5310c9e6c0ee559e3533c7 Mon Sep 17 00:00:00 2001 From: joshii-h Date: Wed, 4 Mar 2026 10:00:02 +0100 Subject: [PATCH] fix: prevent bufio panics and goroutine leaks in SSH client The default bufio.Reader buffer (4096 bytes) is too small for large ServerQuery responses (channellist, clientlist), causing runtime panics that leave the viewer in an unrecoverable state. Changes: - Increase SSH read buffer from 4096 to 64KB to handle large responses - Add done channel to SSHClient for clean keepAlive goroutine shutdown - Stop orphaned keepAlive goroutines after reconnect to prevent leaks - Add panic recovery in exec() so panics are handled as errors - Treat recovered panics as connection errors to trigger reconnect - Wrap HTTP handlers with recovery middleware as a safety net --- http/router.go | 16 ++++++++++- internal/ts6/ssh.go | 65 +++++++++++++++++++++++++++++++++------------ 2 files changed, 63 insertions(+), 18 deletions(-) diff --git a/http/router.go b/http/router.go index 02eb271..e16fad7 100644 --- a/http/router.go +++ b/http/router.go @@ -26,6 +26,20 @@ var ( mu sync.Mutex ) +// recoveryMiddleware catches panics in HTTP handlers and returns a 500 error +// instead of crashing the process. +func recoveryMiddleware(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + defer func() { + if err := recover(); err != nil { + log.Printf("[HTTP] Recovered from panic on %s: %v\n", r.URL.Path, err) + http.Error(w, "Internal Server Error", http.StatusInternalServerError) + } + }() + next.ServeHTTP(w, r) + }) +} + // NewRouter sets up all HTTP routes and returns the router. func NewRouter(cfg config.Config) http.Handler { // parse refresh interval @@ -144,5 +158,5 @@ func NewRouter(cfg config.Config) http.Handler { w.Write([]byte("TS6Viewer is running!")) }) - return mux + return recoveryMiddleware(mux) } diff --git a/internal/ts6/ssh.go b/internal/ts6/ssh.go index f9d4826..9c692ce 100644 --- a/internal/ts6/ssh.go +++ b/internal/ts6/ssh.go @@ -18,6 +18,11 @@ import ( "golang.org/x/crypto/ssh" ) +// sshReadBufferSize is the size of the bufio.Reader buffer used for reading +// ServerQuery responses. The default 4096 is too small for responses like +// channellist or clientlist on servers with many channels/clients. +const sshReadBufferSize = 65536 + // SSHClient represents a persistent SSH ServerQuery connection. type SSHClient struct { cfg *config.Config @@ -28,7 +33,9 @@ type SSHClient struct { stdin io.WriteCloser reader *bufio.Reader - mu sync.Mutex // protects command execution and reconnect + mu sync.Mutex // protects command execution and reconnect + done chan struct{} + once sync.Once } var ( @@ -193,7 +200,8 @@ func newSSHClientBase(cfg *config.Config) (*SSHClient, error) { ssh: client, session: session, stdin: stdin, - reader: bufio.NewReader(stdout), + reader: bufio.NewReaderSize(stdout, sshReadBufferSize), + done: make(chan struct{}), } log.Println("[SSH] Waiting for welcome message") @@ -238,19 +246,27 @@ func newSSHClientBase(cfg *config.Config) (*SSHClient, error) { } // keepAlive sends periodic version commands to prevent idle timeout. +// It stops when the client is closed via the done channel. func (c *SSHClient) keepAlive() { ticker := time.NewTicker(30 * time.Second) defer ticker.Stop() - for range ticker.C { - if c.IsClosed() { + for { + select { + case <-c.done: + log.Println("[SSH] keepAlive stopped (connection closed)") return - } - log.Println("[SSH] Sending keepalive ping") - _, err := c.Exec("version") - if err != nil { - log.Printf("[SSH] Keepalive failed: %v. Attempting reconnect\n", err) - _ = c.reconnect() + case <-ticker.C: + if c.IsClosed() { + return + } + log.Println("[SSH] Sending keepalive ping") + _, err := c.Exec("version") + if err != nil { + log.Printf("[SSH] Keepalive failed: %v. Attempting reconnect\n", err) + _ = c.reconnect() + return // stop this goroutine; reconnect spawns a new one + } } } } @@ -265,8 +281,18 @@ func (c *SSHClient) Exec(cmd string) (string, error) { } // exec sends a raw command and reads the response. -func (c *SSHClient) exec(cmd string) (string, error) { - _, err := c.stdin.Write([]byte(cmd + "\n")) +// It includes panic recovery to handle unexpected bufio errors gracefully +// instead of crashing the process. +func (c *SSHClient) exec(cmd string) (result string, err error) { + defer func() { + if r := recover(); r != nil { + log.Printf("[SSH] Recovered from panic during exec(%q): %v\n", cmd, r) + err = fmt.Errorf("panic during command execution: %v", r) + result = "" + } + }() + + _, err = c.stdin.Write([]byte(cmd + "\n")) if err != nil { return "", err } @@ -275,9 +301,9 @@ func (c *SSHClient) exec(cmd string) (string, error) { var last string for { - line, err := c.reader.ReadString('\n') - if err != nil { - return "", err + line, readErr := c.reader.ReadString('\n') + if readErr != nil { + return "", readErr } line = strings.TrimSpace(line) lines = append(lines, line) @@ -398,7 +424,8 @@ func isConnectionError(err error) bool { return strings.Contains(msg, "eof") || strings.Contains(msg, "broken pipe") || strings.Contains(msg, "connection reset") || - strings.Contains(msg, "use of closed network connection") + strings.Contains(msg, "use of closed network connection") || + strings.Contains(msg, "panic during command execution") } // IsClosed checks whether the client is closed. @@ -406,10 +433,14 @@ func (c *SSHClient) IsClosed() bool { return c == nil || c.ssh == nil } -// Close terminates the SSH session. +// Close terminates the SSH session and signals the keepAlive goroutine to stop. func (c *SSHClient) Close() { log.Println("[SSH] Closing SSH connection") + c.once.Do(func() { + close(c.done) + }) + if c.session != nil { _ = c.session.Close() c.session = nil