libgit2/managed: fix race issues in ssh transport

Race conditions in ssh smart subtransport caused some goroutines to
panic, resulting in crashing the whole controller, mostly evident in
image-automation-controller CI runs. Panic recovery in the main thread
do not handle goroutine panics. So, the existing panic recovery code in
libgit2 Checkout() methods weren't able to handle it.

This change groups the fields in ssh smart subtransport that may be
accessed by multiple goroutines into a new struct with a mutex. Also
adds panic recovery in the created goroutine to handle any other
possible panics.

Signed-off-by: Sunny <[email protected]>

Author: darkowlzz

pkg/git/libgit2/managed/ssh.go

@@ -46,12 +46,14 @@ package managed
 import (
 	"context"
 	"crypto/sha256"
+	"errors"
 	"fmt"
 	"io"
 	"net"
 	"net/url"
 	"runtime"
 	"strings"
+	"sync"
 	"time"
 
 	"golang.org/x/crypto/ssh"
@@ -83,16 +85,22 @@ func sshSmartSubtransportFactory(remote *git2go.Remote, transport *git2go.Transp
 type sshSmartSubtransport struct {
 	transport *git2go.Transport
 
-	lastAction    git2go.SmartServiceAction
+	lastAction git2go.SmartServiceAction
+	stdin      io.WriteCloser
+	stdout     io.Reader
+	addr       string
+	ctx        context.Context
+
+	con connection
+}
+
+type connection struct {
 	conn          net.Conn
 	client        *ssh.Client
 	session       *ssh.Session
-	stdin         io.WriteCloser
-	stdout        io.Reader
 	currentStream *sshSmartSubtransportStream
-	addr          string
 	connected     bool
-	ctx           context.Context
+	m             sync.Mutex
 }
 
 func (t *sshSmartSubtransport) Action(transportOptionsURL string, action git2go.SmartServiceAction) (git2go.SmartSubtransportStream, error) {
@@ -128,17 +136,17 @@ func (t *sshSmartSubtransport) Action(transportOptionsURL string, action git2go.
 	var cmd string
 	switch action {
 	case git2go.SmartServiceActionUploadpackLs, git2go.SmartServiceActionUploadpack:
-		if t.currentStream != nil {
+		if t.con.currentStream != nil {
 			if t.lastAction == git2go.SmartServiceActionUploadpackLs {
-				return t.currentStream, nil
+				return t.con.currentStream, nil
 			}
 		}
 		cmd = fmt.Sprintf("git-upload-pack '%s'", uPath)
 
 	case git2go.SmartServiceActionReceivepackLs, git2go.SmartServiceActionReceivepack:
-		if t.currentStream != nil {
+		if t.con.currentStream != nil {
 			if t.lastAction == git2go.SmartServiceActionReceivepackLs {
-				return t.currentStream, nil
+				return t.con.currentStream, nil
 			}
 		}
 		cmd = fmt.Sprintf("git-receive-pack '%s'", uPath)
@@ -147,7 +155,7 @@ func (t *sshSmartSubtransport) Action(transportOptionsURL string, action git2go.
 		return nil, fmt.Errorf("unexpected action: %v", action)
 	}
 
-	if t.connected {
+	if t.con.connected {
 		// Disregard errors from previous stream, futher details inside Close().
 		_ = t.Close()
 	}
@@ -185,21 +193,23 @@ func (t *sshSmartSubtransport) Action(transportOptionsURL string, action git2go.
 	if err != nil {
 		return nil, err
 	}
-	t.connected = true
+	t.con.m.Lock()
+	t.con.connected = true
+	t.con.m.Unlock()
 
 	traceLog.Info("[ssh]: creating new ssh session")
-	if t.session, err = t.client.NewSession(); err != nil {
+	if t.con.session, err = t.con.client.NewSession(); err != nil {
 		return nil, err
 	}
 
-	if t.stdin, err = t.session.StdinPipe(); err != nil {
+	if t.stdin, err = t.con.session.StdinPipe(); err != nil {
 		return nil, err
 	}
 
 	var w *io.PipeWriter
 	var reader io.Reader
 	t.stdout, w = io.Pipe()
-	if reader, err = t.session.StdoutPipe(); err != nil {
+	if reader, err = t.con.session.StdoutPipe(); err != nil {
 		return nil, err
 	}
 
@@ -208,7 +218,15 @@ func (t *sshSmartSubtransport) Action(transportOptionsURL string, action git2go.
 	//
 	// xref: https://github.com/golang/crypto/blob/eb4f295cb31f7fb5d52810411604a2638c9b19a2/ssh/session.go#L553-L558
 	go func() error {
-		defer w.Close()
+		defer func() {
+			w.Close()
+
+			// In case this goroutine panics, handle recovery.
+			if r := recover(); r != nil {
+				traceLog.Error(errors.New(r.(string)),
+					"[ssh]: recovered from libgit2 ssh smart subtransport panic", "address", t.addr)
+			}
+		}()
 
 		var cancel context.CancelFunc
 		ctx := t.ctx
@@ -226,9 +244,12 @@ func (t *sshSmartSubtransport) Action(transportOptionsURL string, action git2go.
 				return nil
 
 			default:
-				if !t.connected {
+				t.con.m.Lock()
+				if !t.con.connected {
+					t.con.m.Unlock()
 					return nil
 				}
+				t.con.m.Unlock()
 
 				_, err := io.Copy(w, reader)
 				if err != nil {
@@ -240,16 +261,16 @@ func (t *sshSmartSubtransport) Action(transportOptionsURL string, action git2go.
 	}()
 
 	traceLog.Info("[ssh]: run on remote", "cmd", cmd)
-	if err := t.session.Start(cmd); err != nil {
+	if err := t.con.session.Start(cmd); err != nil {
 		return nil, err
 	}
 
 	t.lastAction = action
-	t.currentStream = &sshSmartSubtransportStream{
+	t.con.currentStream = &sshSmartSubtransportStream{
 		owner: t,
 	}
 
-	return t.currentStream, nil
+	return t.con.currentStream, nil
 }
 
 func (t *sshSmartSubtransport) createConn(addr string, sshConfig *ssh.ClientConfig) error {
@@ -265,8 +286,8 @@ func (t *sshSmartSubtransport) createConn(addr string, sshConfig *ssh.ClientConf
 		return err
 	}
 
-	t.conn = conn
-	t.client = ssh.NewClient(c, chans, reqs)
+	t.con.conn = conn
+	t.con.client = ssh.NewClient(c, chans, reqs)
 
 	return nil
 }
@@ -282,31 +303,35 @@ func (t *sshSmartSubtransport) createConn(addr string, sshConfig *ssh.ClientConf
 // SmartSubTransport (i.e. unreleased resources, staled connections).
 func (t *sshSmartSubtransport) Close() error {
 	traceLog.Info("[ssh]: sshSmartSubtransport.Close()", "server", t.addr)
-	t.currentStream = nil
-	if t.client != nil && t.stdin != nil {
+	t.con.m.Lock()
+	defer t.con.m.Unlock()
+	t.con.currentStream = nil
+	if t.con.client != nil && t.stdin != nil {
 		_ = t.stdin.Close()
 	}
-	t.client = nil
+	t.con.client = nil
 
-	if t.session != nil {
+	if t.con.session != nil {
 		traceLog.Info("[ssh]: session.Close()", "server", t.addr)
-		_ = t.session.Close()
+		_ = t.con.session.Close()
 	}
-	t.session = nil
+	t.con.session = nil
 
 	return nil
 }
 
 func (t *sshSmartSubtransport) Free() {
 	traceLog.Info("[ssh]: sshSmartSubtransport.Free()")
-	if t.client != nil {
-		_ = t.client.Close()
+	if t.con.client != nil {
+		_ = t.con.client.Close()
 	}
 
-	if t.conn != nil {
-		_ = t.conn.Close()
+	if t.con.conn != nil {
+		_ = t.con.conn.Close()
 	}
-	t.connected = false
+	t.con.m.Lock()
+	t.con.connected = false
+	t.con.m.Unlock()
 }
 
 type sshSmartSubtransportStream struct {