From 5f26c11c5f634a3c6d8b3fec0e3f6ce3cfb6ef88 Mon Sep 17 00:00:00 2001 From: Jakub Nyckowski Date: Mon, 8 May 2023 15:58:50 -0400 Subject: [PATCH 1/2] Prevent panic when fileTransferRequestC is closed. This PR fixes two issues. 1. Closes fileTransferRequestC and fileTransferDecisionC on Close() 2. Prevent panic when the channel is closed and nil is returned in select. --- lib/web/terminal.go | 39 ++++++++++++++++++++++++++++++--------- 1 file changed, 30 insertions(+), 9 deletions(-) diff --git a/lib/web/terminal.go b/lib/web/terminal.go index 34e1daefa27f1..76771e50bc1da 100644 --- a/lib/web/terminal.go +++ b/lib/web/terminal.go @@ -883,16 +883,31 @@ func (t *TerminalHandler) handleFileTransfer(fileTransferRequestC <-chan *sessio case <-t.terminalContext.Done(): return case transferRequest := <-fileTransferRequestC: - t.sshSession.RequestFileTransfer(t.terminalContext, tracessh.FileTransferReq{ + if transferRequest == nil { + // channel closed + continue + } + if err := t.sshSession.RequestFileTransfer(t.terminalContext, tracessh.FileTransferReq{ Download: transferRequest.Download, Location: transferRequest.Location, Filename: transferRequest.Filename, - }) + }); err != nil { + t.log.WithError(err).Error("Unable to request file transfer") + } case transferResponse := <-fileTransferDecisionC: + if transferResponse == nil { + // channel closed + continue + } + + var err error if transferResponse.Approved { - t.sshSession.ApproveFileTransferRequest(t.terminalContext, transferResponse.RequestID) + err = t.sshSession.ApproveFileTransferRequest(t.terminalContext, transferResponse.RequestID) } else { - t.sshSession.DenyFileTransferRequest(t.terminalContext, transferResponse.RequestID) + err = t.sshSession.DenyFileTransferRequest(t.terminalContext, transferResponse.RequestID) + } + if err != nil { + t.log.WithError(err).Error("Unable to respond to file transfer request") } } @@ -1041,13 +1056,19 @@ type TerminalStream struct { // fit into the buffer provided by the callee to Read method buffer []byte - // once ensures that resizeC is closed at most one time - once sync.Once + // ResizeOnce ensures that resizeC is closed at most one time + ResizeOnce sync.Once // resizeC a channel to forward resize events so that // they happen out of band and don't block reads resizeC chan<- *session.TerminalParams + + // fileTransferRequestOnce ensures that fileTransferRequestC is closed at most one time + fileTransferRequestOnce sync.Once // fileTransferRequestC is a channel to facilitate requesting a file transfer fileTransferRequestC chan<- *session.FileTransferRequestParams + + // fileTransferDecisionOnce ensures that fileTransferDecisionC is closed at most one time + fileTransferDecisionOnce sync.Once // fileTransferDecisionC is a channel to facilitate responding to a file transfer // with an approval or denial fileTransferDecisionC chan<- *session.FileTransferDecisionParams @@ -1281,19 +1302,19 @@ func (t *TerminalStream) Read(out []byte) (n int, err error) { // prior to closing the web socket altogether. func (t *TerminalStream) Close() error { if t.resizeC != nil { - t.once.Do(func() { + t.ResizeOnce.Do(func() { close(t.resizeC) }) } if t.fileTransferRequestC != nil { - t.once.Do(func() { + t.fileTransferRequestOnce.Do(func() { close(t.fileTransferRequestC) }) } if t.fileTransferDecisionC != nil { - t.once.Do(func() { + t.fileTransferDecisionOnce.Do(func() { close(t.fileTransferDecisionC) }) } From e382294cc9589438c8c6d67a4384fc0377fb1f49 Mon Sep 17 00:00:00 2001 From: Jakub Nyckowski Date: Mon, 8 May 2023 16:05:42 -0400 Subject: [PATCH 2/2] Clean the code --- lib/web/terminal.go | 30 ++++++++++-------------------- 1 file changed, 10 insertions(+), 20 deletions(-) diff --git a/lib/web/terminal.go b/lib/web/terminal.go index 76771e50bc1da..668b6162b352f 100644 --- a/lib/web/terminal.go +++ b/lib/web/terminal.go @@ -1056,19 +1056,13 @@ type TerminalStream struct { // fit into the buffer provided by the callee to Read method buffer []byte - // ResizeOnce ensures that resizeC is closed at most one time - ResizeOnce sync.Once + // once ensures that all channels are closed at most one time. + once sync.Once // resizeC a channel to forward resize events so that // they happen out of band and don't block reads resizeC chan<- *session.TerminalParams - - // fileTransferRequestOnce ensures that fileTransferRequestC is closed at most one time - fileTransferRequestOnce sync.Once // fileTransferRequestC is a channel to facilitate requesting a file transfer fileTransferRequestC chan<- *session.FileTransferRequestParams - - // fileTransferDecisionOnce ensures that fileTransferDecisionC is closed at most one time - fileTransferDecisionOnce sync.Once // fileTransferDecisionC is a channel to facilitate responding to a file transfer // with an approval or denial fileTransferDecisionC chan<- *session.FileTransferDecisionParams @@ -1301,23 +1295,19 @@ func (t *TerminalStream) Read(out []byte) (n int, err error) { // Close send a close message on the web socket // prior to closing the web socket altogether. func (t *TerminalStream) Close() error { - if t.resizeC != nil { - t.ResizeOnce.Do(func() { + t.once.Do(func() { + if t.resizeC != nil { close(t.resizeC) - }) - } + } - if t.fileTransferRequestC != nil { - t.fileTransferRequestOnce.Do(func() { + if t.fileTransferRequestC != nil { close(t.fileTransferRequestC) - }) - } + } - if t.fileTransferDecisionC != nil { - t.fileTransferDecisionOnce.Do(func() { + if t.fileTransferDecisionC != nil { close(t.fileTransferDecisionC) - }) - } + } + }) // Send close envelope to web terminal upon exit without an error. envelope := &Envelope{