From 5bd0519ef32827e59d94003b995a62a8915f48d4 Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Thu, 2 Sep 2021 16:45:23 -0400 Subject: [PATCH 1/2] move Listen call into clients to avoid race --- port_forwarder.go | 26 ++++++++++++++++---------- port_forwarder_test.go | 10 ++++++++-- 2 files changed, 24 insertions(+), 12 deletions(-) diff --git a/port_forwarder.go b/port_forwarder.go index f4895bb60..fe0d7d80e 100644 --- a/port_forwarder.go +++ b/port_forwarder.go @@ -26,22 +26,28 @@ func NewPortForwarder(session *Session, name string, remotePort int) *PortForwar } } +// ListenTCP calls listen on the chosen local TCP port. Zero picks an arbitrary port. +// It is provided for the convenience of callers of ForwardToLocalPort. +func Listen(port int) (net.Listener, error) { + return net.Listen("tcp", fmt.Sprintf(":%d", port)) +} + // ForwardToLocalPort forwards traffic between the container's remote -// port and a local TCP port. It 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. -func (fwd *PortForwarder) ForwardToLocalPort(ctx context.Context, localPort int) (err error) { +// port and a local TCP port, which must already be listening for +// connections. (Accepting a listener rather than a port number avoids +// races against other processes opening ports, and against a client +// connecting to the socket prematurely.) +// +// ForwardToLocalPort 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. +func (fwd *PortForwarder) ForwardToLocalPort(ctx context.Context, listen net.Listener) (err error) { id, err := fwd.shareRemotePort(ctx) if err != nil { return err } - listen, err := net.Listen("tcp", fmt.Sprintf(":%d", localPort)) - if err != nil { - return fmt.Errorf("error listening on TCP port: %v", err) - } - defer safeClose(listen, &err) - errc := make(chan error, 1) sendError := func(err error) { // Use non-blocking send, to avoid goroutines getting diff --git a/port_forwarder_test.go b/port_forwarder_test.go index 6ccb3d05e..68b658b6b 100644 --- a/port_forwarder_test.go +++ b/port_forwarder_test.go @@ -46,13 +46,19 @@ func TestPortForwarderStart(t *testing.T) { } defer testServer.Close() + listen, err := Listen(8000) // local port + if err != nil { + t.Fatal(err) + } + defer listen.Close() + ctx, cancel := context.WithCancel(context.Background()) defer cancel() done := make(chan error) go func() { - const name, local, remote = "ssh", 8000, 8000 - done <- NewPortForwarder(session, name, remote).ForwardToLocalPort(ctx, local) + const name, remote = "ssh", 8000 + done <- NewPortForwarder(session, name, remote).ForwardToLocalPort(ctx, listen) }() go func() { From e2552fbd2a049a8314d83e1b1357b6b5494267dd Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Fri, 3 Sep 2021 09:43:31 -0400 Subject: [PATCH 2/2] rename to ForwardToListener --- port_forwarder.go | 17 +++++++++-------- port_forwarder_test.go | 4 ++-- 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/port_forwarder.go b/port_forwarder.go index fe0d7d80e..593b70fe7 100644 --- a/port_forwarder.go +++ b/port_forwarder.go @@ -26,23 +26,24 @@ func NewPortForwarder(session *Session, name string, remotePort int) *PortForwar } } -// ListenTCP calls listen on the chosen local TCP port. Zero picks an arbitrary port. -// It is provided for the convenience of callers of ForwardToLocalPort. -func Listen(port int) (net.Listener, error) { +// ListenTCP calls listen on the chosen local TCP port. Zero picks an +// arbitrary port. It is provided for the convenience of callers of +// ForwardToListener. +func ListenTCP(port int) (net.Listener, error) { return net.Listen("tcp", fmt.Sprintf(":%d", port)) } -// ForwardToLocalPort forwards traffic between the container's remote -// port and a local TCP port, which must already be listening for +// ForwardToListener forwards traffic between the container's remote +// port and a local port, which must already be listening for // connections. (Accepting a listener rather than a port number avoids // races against other processes opening ports, and against a client // connecting to the socket prematurely.) // -// ForwardToLocalPort accepts and handles connections on the local -// port until it encounters the first error, which may include context +// 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. -func (fwd *PortForwarder) ForwardToLocalPort(ctx context.Context, listen net.Listener) (err error) { +func (fwd *PortForwarder) ForwardToListener(ctx context.Context, listen net.Listener) (err error) { id, err := fwd.shareRemotePort(ctx) if err != nil { return err diff --git a/port_forwarder_test.go b/port_forwarder_test.go index 68b658b6b..d6a4e7708 100644 --- a/port_forwarder_test.go +++ b/port_forwarder_test.go @@ -46,7 +46,7 @@ func TestPortForwarderStart(t *testing.T) { } defer testServer.Close() - listen, err := Listen(8000) // local port + listen, err := ListenTCP(8000) // local port if err != nil { t.Fatal(err) } @@ -58,7 +58,7 @@ func TestPortForwarderStart(t *testing.T) { done := make(chan error) go func() { const name, remote = "ssh", 8000 - done <- NewPortForwarder(session, name, remote).ForwardToLocalPort(ctx, listen) + done <- NewPortForwarder(session, name, remote).ForwardToListener(ctx, listen) }() go func() {