From 80dfa955518b4cf3d618c82eb9e36475c3281a57 Mon Sep 17 00:00:00 2001 From: Hallo_Tschuess Date: Tue, 21 Feb 2023 20:43:41 +0100 Subject: [PATCH 1/6] fix: write i/o timeout on ssh connection --- client.go | 4 ---- client_test.go | 2 +- connection.go | 31 +++++++++++++++++++++++++++++-- 3 files changed, 30 insertions(+), 7 deletions(-) diff --git a/client.go b/client.go index 2f769f3..d9c5a7d 100644 --- a/client.go +++ b/client.go @@ -211,7 +211,6 @@ func (c *Client) messageHandler() { for { if c.scanner.Scan() { line := c.scanner.Text() - //nolint: gocritic if line == "error id=0 msg=ok" { c.err <- nil } else if matches := respTrailerRe.FindStringSubmatch(line); len(matches) == 4 { @@ -253,9 +252,6 @@ func (c *Client) workHandler() { } func (c *Client) process(data string) { - if err := c.conn.SetWriteDeadline(time.Now().Add(c.timeout)); err != nil { - c.err <- err - } if _, err := c.conn.Write([]byte(data)); err != nil { c.err <- err } diff --git a/client_test.go b/client_test.go index 642dff2..3dc408c 100644 --- a/client_test.go +++ b/client_test.go @@ -128,7 +128,7 @@ func TestClientWriteFail(t *testing.T) { if !assert.NoError(t, err) { return } - assert.NoError(t, c.conn.(*legacyConnection).Conn.(*net.TCPConn).CloseWrite()) + assert.NoError(t, c.conn.(*legacyConnection).Conn.(*writeTimeoutConn).Conn.(*net.TCPConn).CloseWrite()) _, err = c.Exec("version") assert.Error(t, err) diff --git a/connection.go b/connection.go index 8f89c16..2596837 100644 --- a/connection.go +++ b/connection.go @@ -20,6 +20,21 @@ const ( DefaultSSHPort = 10022 ) +type writeTimeoutConn struct { + net.Conn + timeout time.Duration +} + +func (c *writeTimeoutConn) Write(p []byte) (n int, err error) { + if err = c.Conn.SetWriteDeadline(time.Now().Add(c.timeout)); err != nil { + return 0, fmt.Errorf("writeTimeoutConn: SetWriteDeadline: %w", err) + } + if n, err = c.Conn.Write(p); err != nil { + return n, fmt.Errorf("writeTimeoutConn: write: %w", err) + } + return n, nil +} + // legacyConnection is an insecure TCP connection. type legacyConnection struct { net.Conn @@ -32,10 +47,16 @@ func (c *legacyConnection) Connect(addr string, timeout time.Duration) error { return err } - c.Conn, err = net.DialTimeout("tcp", addr, timeout) + conn, err := net.DialTimeout("tcp", addr, timeout) if err != nil { return fmt.Errorf("legacy connection: dial: %w", err) } + + c.Conn = &writeTimeoutConn{ + Conn: conn, + timeout: timeout, + } + return nil } @@ -53,10 +74,16 @@ func (c *sshConnection) Connect(addr string, timeout time.Duration) error { return err } - if c.Conn, err = net.DialTimeout("tcp", addr, timeout); err != nil { + conn, err := net.DialTimeout("tcp", addr, timeout) + if err != nil { return fmt.Errorf("ssh connection: dial: %w", err) } + c.Conn = &writeTimeoutConn{ + Conn: conn, + timeout: timeout, + } + clientConn, chans, reqs, err := ssh.NewClientConn(c.Conn, addr, c.config) if err != nil { return fmt.Errorf("ssh connecion: ssh client conn: %w", err) From 2cea98c5097bc9e2d5d6c210bcd6d2bab8ce158c Mon Sep 17 00:00:00 2001 From: Hallo_Tschuess Date: Wed, 22 Feb 2023 17:27:36 +0100 Subject: [PATCH 2/6] docs: add documentation about timeout --- connection.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/connection.go b/connection.go index 2596837..82e03d7 100644 --- a/connection.go +++ b/connection.go @@ -20,6 +20,7 @@ const ( DefaultSSHPort = 10022 ) +// writeTimeoutConn is a net.Conn that sets the write timeout on every call to Write(). type writeTimeoutConn struct { net.Conn timeout time.Duration @@ -41,6 +42,7 @@ type legacyConnection struct { } // Connect connects to the address with the given timeout. +// The timeout is used as dial and write timeout. func (c *legacyConnection) Connect(addr string, timeout time.Duration) error { addr, err := verifyAddr(addr, DefaultPort) if err != nil { @@ -68,6 +70,7 @@ type sshConnection struct { } // Connect connects to the address with the given timeout and opens a new SSH channel with attached shell. +// The timeout is used as dial and write timeout. func (c *sshConnection) Connect(addr string, timeout time.Duration) error { addr, err := verifyAddr(addr, DefaultSSHPort) if err != nil { From a4472eb2e1dbe929faeff4931587851c743fa40a Mon Sep 17 00:00:00 2001 From: Hallo_Tschuess Date: Wed, 22 Feb 2023 19:31:31 +0100 Subject: [PATCH 3/6] fix: freezing in ExecCmd when writing to work channel --- client.go | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/client.go b/client.go index d9c5a7d..728463c 100644 --- a/client.go +++ b/client.go @@ -268,14 +268,31 @@ func (c *Client) ExecCmd(cmd *Cmd) ([]string, error) { return nil, ErrNotConnected } - c.work <- cmd.String() + timeout := time.NewTimer(c.timeout) + defer func() { + timeout.Stop() + select { + case <-timeout.C: + default: + } + }() + + select { + case c.work <- cmd.String(): + case <-c.disconnect: + return nil, ErrNotConnected + case <-timeout.C: + return nil, ErrTimeout + } select { case err := <-c.err: if err != nil { return nil, err } - case <-time.After(c.timeout): + case <-c.disconnect: + return nil, ErrNotConnected + case <-timeout.C: return nil, ErrTimeout } From bc0bd9a3ae4a949ea407345f1452361026433f54 Mon Sep 17 00:00:00 2001 From: Hallo_Tschuess Date: Wed, 22 Feb 2023 23:16:08 +0100 Subject: [PATCH 4/6] Revert "fix: freezing in ExecCmd when writing to work channel" This reverts commit a4472eb2e1dbe929faeff4931587851c743fa40a. --- client.go | 21 ++------------------- 1 file changed, 2 insertions(+), 19 deletions(-) diff --git a/client.go b/client.go index 728463c..d9c5a7d 100644 --- a/client.go +++ b/client.go @@ -268,31 +268,14 @@ func (c *Client) ExecCmd(cmd *Cmd) ([]string, error) { return nil, ErrNotConnected } - timeout := time.NewTimer(c.timeout) - defer func() { - timeout.Stop() - select { - case <-timeout.C: - default: - } - }() - - select { - case c.work <- cmd.String(): - case <-c.disconnect: - return nil, ErrNotConnected - case <-timeout.C: - return nil, ErrTimeout - } + c.work <- cmd.String() select { case err := <-c.err: if err != nil { return nil, err } - case <-c.disconnect: - return nil, ErrNotConnected - case <-timeout.C: + case <-time.After(c.timeout): return nil, ErrTimeout } From 564336802ef0cd79212cde5640c17871221e4f9f Mon Sep 17 00:00:00 2001 From: Hallo_Tschuess Date: Wed, 22 Feb 2023 23:23:23 +0100 Subject: [PATCH 5/6] fix: error handling in messageHandler() --- client.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/client.go b/client.go index d9c5a7d..861ae37 100644 --- a/client.go +++ b/client.go @@ -228,11 +228,12 @@ func (c *Client) messageHandler() { } } else { err := c.scanErr() - c.err <- err - if errors.Is(err, io.ErrUnexpectedEOF) { + if errors.Is(err, io.ErrUnexpectedEOF) || errors.Is(err, io.EOF) { close(c.disconnect) + c.err <- err return } + c.err <- err } } } From dd068f8b76eccec2c95c6f1b50d85176e715e64a Mon Sep 17 00:00:00 2001 From: Hallo_Tschuess Date: Thu, 23 Feb 2023 01:35:17 +0100 Subject: [PATCH 6/6] ci: set timeout for linting on windows to 2m --- .github/workflows/go.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/go.yml b/.github/workflows/go.yml index 5b4909a..a8a4515 100644 --- a/.github/workflows/go.yml +++ b/.github/workflows/go.yml @@ -63,7 +63,7 @@ jobs: outformat: out-format with: version: ${{ matrix.golangci }} - args: "--%outformat% colored-line-number" + args: "--%outformat% colored-line-number --timeout 2m" skip-pkg-cache: true skip-build-cache: true