From 8b0e8c990e68dc9b74ed3d40f4c73c9a24bacb7b Mon Sep 17 00:00:00 2001 From: Jose Garcia Date: Thu, 9 Sep 2021 17:31:18 +0000 Subject: [PATCH 1/5] ignore pf conn errors --- port_forwarder.go | 25 ++++++++----------------- 1 file changed, 8 insertions(+), 17 deletions(-) diff --git a/port_forwarder.go b/port_forwarder.go index dc91222ed..1351025cb 100644 --- a/port_forwarder.go +++ b/port_forwarder.go @@ -33,9 +33,7 @@ func NewPortForwarder(session *Session, name string, remotePort int) *PortForwar // connecting to the socket prematurely.) // // ForwardToListener accepts and handles connections on the local port -// until it encounters the first error, which may include context -// cancellation. Its error result is always non-nil. The caller is -// responsible for closing the listening port. +// until the context is cancelled. The caller is responsible for closing the listening port. func (fwd *PortForwarder) ForwardToListener(ctx context.Context, listen net.Listener) (err error) { id, err := fwd.shareRemotePort(ctx) if err != nil { @@ -124,21 +122,14 @@ func (fwd *PortForwarder) handleConnection(ctx context.Context, id channelID, co } }() - errs := make(chan error, 2) - copyConn := func(w io.Writer, r io.Reader) { - _, err := io.Copy(w, r) - errs <- err - } - go copyConn(conn, channel) - go copyConn(channel, conn) + // Bi-directional copy of data. + // If any individual connection has an error, we can safely ignore them + // and defer to connection clients to handle data loss as necessary. + go io.Copy(conn, channel) + go io.Copy(channel, conn) - // await result - for i := 0; i < 2; i++ { - if err := <-errs; err != nil && err != io.EOF { - return fmt.Errorf("tunnel connection: %v", err) - } - } - return nil + <-ctx.Done() + return ctx.Err() } // safeClose reports the error (to *err) from closing the stream only From 1ff5c514fb82458558757fc8f1fcdf6cc838afc0 Mon Sep 17 00:00:00 2001 From: Jose Garcia Date: Thu, 9 Sep 2021 18:35:05 +0000 Subject: [PATCH 2/5] fix erroneous ctx waiting and introduce back io.EOF handling --- port_forwarder.go | 27 +++++++++++++++++++++------ 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/port_forwarder.go b/port_forwarder.go index 1351025cb..f47d11565 100644 --- a/port_forwarder.go +++ b/port_forwarder.go @@ -123,13 +123,28 @@ func (fwd *PortForwarder) handleConnection(ctx context.Context, id channelID, co }() // Bi-directional copy of data. - // If any individual connection has an error, we can safely ignore them - // and defer to connection clients to handle data loss as necessary. - go io.Copy(conn, channel) - go io.Copy(channel, conn) + errs := make(chan error, 2) + copyConn := func(w io.Writer, r io.Reader) { + _, err := io.Copy(w, r) + errs <- err + } + go copyConn(conn, channel) + go copyConn(channel, conn) - <-ctx.Done() - return ctx.Err() + // wait until context is cancelled or we've received two io.EOF +Loop: + for i := 0; i < 2; i++ { + select { + case <-ctx.Done(): + return ctx.Err() + case err := <-errs: + if err != nil && err != io.EOF { + break Loop // non-EOF errors stop connection handling + } + } + } + + return nil } // safeClose reports the error (to *err) from closing the stream only From 920f793c6ddf001901308df947091c9d98563fdd Mon Sep 17 00:00:00 2001 From: Jose Garcia Date: Thu, 9 Sep 2021 19:33:16 +0000 Subject: [PATCH 3/5] pr feedback --- port_forwarder.go | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/port_forwarder.go b/port_forwarder.go index f47d11565..e8649c693 100644 --- a/port_forwarder.go +++ b/port_forwarder.go @@ -122,7 +122,7 @@ func (fwd *PortForwarder) handleConnection(ctx context.Context, id channelID, co } }() - // Bi-directional copy of data. + // bi-directional copy of data. errs := make(chan error, 2) copyConn := func(w io.Writer, r io.Reader) { _, err := io.Copy(w, r) @@ -131,20 +131,18 @@ func (fwd *PortForwarder) handleConnection(ctx context.Context, id channelID, co go copyConn(conn, channel) go copyConn(channel, conn) - // wait until context is cancelled or we've received two io.EOF -Loop: - for i := 0; i < 2; i++ { + // wait until context is cancelled or both copies are done + for i := 0; ; { select { case <-ctx.Done(): return ctx.Err() - case err := <-errs: - if err != nil && err != io.EOF { - break Loop // non-EOF errors stop connection handling + case <-errs: + i++ + if i == 2 { + return nil } } } - - return nil } // safeClose reports the error (to *err) from closing the stream only From efe519cb7af8015a0747ac20d91877627b21b8cc Mon Sep 17 00:00:00 2001 From: Jose Garcia Date: Thu, 9 Sep 2021 20:11:45 +0000 Subject: [PATCH 4/5] comments + fix Forward method --- port_forwarder.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/port_forwarder.go b/port_forwarder.go index e8649c693..be191c211 100644 --- a/port_forwarder.go +++ b/port_forwarder.go @@ -80,9 +80,7 @@ func (fwd *PortForwarder) Forward(ctx context.Context, conn io.ReadWriteCloser) // Create buffered channel so that send doesn't get stuck after context cancellation. errc := make(chan error, 1) go func() { - if err := fwd.handleConnection(ctx, id, conn); err != nil { - errc <- err - } + errc <- fwd.handleConnection(ctx, id, conn) }() return awaitError(ctx, errc) } @@ -131,7 +129,9 @@ func (fwd *PortForwarder) handleConnection(ctx context.Context, id channelID, co go copyConn(conn, channel) go copyConn(channel, conn) - // wait until context is cancelled or both copies are done + // Wait until context is cancelled or both copies are done. + // Discard errors from io.Copy; they should not cause (e.g.) ForwardToListener to fail. + // TODO: how can we proxy errors from Copy so that each peer can distinguish an error from a short file? for i := 0; ; { select { case <-ctx.Done(): From 272ea57b541c33846add2b9b493c0ca011985c93 Mon Sep 17 00:00:00 2001 From: Jose Garcia Date: Thu, 9 Sep 2021 21:00:09 +0000 Subject: [PATCH 5/5] revert comment update --- port_forwarder.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/port_forwarder.go b/port_forwarder.go index be191c211..8011d19fc 100644 --- a/port_forwarder.go +++ b/port_forwarder.go @@ -33,7 +33,9 @@ func NewPortForwarder(session *Session, name string, remotePort int) *PortForwar // connecting to the socket prematurely.) // // ForwardToListener accepts and handles connections on the local port -// until the context is cancelled. The caller is responsible for closing the listening port. +// until it encounters the first error, which may include context +// cancellation. Its error result is always non-nil. The caller is +// responsible for closing the listening port. func (fwd *PortForwarder) ForwardToListener(ctx context.Context, listen net.Listener) (err error) { id, err := fwd.shareRemotePort(ctx) if err != nil {