From 09fae634df1fad8e222c0f1467dff6e060644690 Mon Sep 17 00:00:00 2001 From: Paulo Gomes Date: Thu, 16 Jun 2022 13:50:14 +0100 Subject: [PATCH 1/2] libgit2: remove deadlock Some scenarios may lead to deadlocks, specially in image automation controller. Signed-off-by: Paulo Gomes --- pkg/git/libgit2/managed/ssh.go | 29 +++++++++++++---------------- 1 file changed, 13 insertions(+), 16 deletions(-) diff --git a/pkg/git/libgit2/managed/ssh.go b/pkg/git/libgit2/managed/ssh.go index 986efd937..32553797e 100644 --- a/pkg/git/libgit2/managed/ssh.go +++ b/pkg/git/libgit2/managed/ssh.go @@ -54,6 +54,7 @@ import ( "runtime" "strings" "sync" + "sync/atomic" "time" "golang.org/x/crypto/ssh" @@ -80,10 +81,12 @@ func registerManagedSSH() error { } func sshSmartSubtransportFactory(remote *git2go.Remote, transport *git2go.Transport) (git2go.SmartSubtransport, error) { + var closed int32 = 0 return &sshSmartSubtransport{ - transport: transport, - ctx: context.Background(), - logger: logr.Discard(), + transport: transport, + ctx: context.Background(), + logger: logr.Discard(), + closedSessions: &closed, }, nil } @@ -109,6 +112,8 @@ type sshSmartSubtransport struct { stdin io.WriteCloser stdout io.Reader + closedSessions *int32 + con connection } @@ -117,7 +122,6 @@ type connection struct { session *ssh.Session currentStream *sshSmartSubtransportStream connected bool - m sync.RWMutex } func (t *sshSmartSubtransport) Action(transportOptionsURL string, action git2go.SmartServiceAction) (git2go.SmartSubtransportStream, error) { @@ -208,13 +212,11 @@ func (t *sshSmartSubtransport) Action(transportOptionsURL string, action git2go. return nil } - t.con.m.RLock() - if t.con.connected == true { + if t.con.connected { // The connection is no longer shared across actions, so ensures // all has been released before starting a new connection. _ = t.Close() } - t.con.m.RUnlock() err = t.createConn(addr, sshConfig) if err != nil { @@ -251,7 +253,6 @@ func (t *sshSmartSubtransport) Action(transportOptionsURL string, action git2go. "recovered from libgit2 ssh smart subtransport panic") } }() - var cancel context.CancelFunc ctx := t.ctx @@ -261,6 +262,7 @@ func (t *sshSmartSubtransport) Action(transportOptionsURL string, action git2go. defer cancel() } + closedAlready := atomic.LoadInt32(t.closedSessions) for { select { case <-ctx.Done(): @@ -268,12 +270,9 @@ func (t *sshSmartSubtransport) Action(transportOptionsURL string, action git2go. return nil default: - t.con.m.RLock() - if !t.con.connected { - t.con.m.RUnlock() + if atomic.LoadInt32(t.closedSessions) > closedAlready { return nil } - t.con.m.RUnlock() _, err := io.Copy(w, reader) if err != nil { @@ -311,10 +310,8 @@ func (t *sshSmartSubtransport) createConn(addr string, sshConfig *ssh.ClientConf return err } - t.con.m.Lock() t.con.connected = true t.con.client = ssh.NewClient(c, chans, reqs) - t.con.m.Unlock() return nil } @@ -330,8 +327,6 @@ func (t *sshSmartSubtransport) createConn(addr string, sshConfig *ssh.ClientConf // SmartSubTransport (i.e. unreleased resources, staled connections). func (t *sshSmartSubtransport) Close() error { t.logger.V(logger.TraceLevel).Info("sshSmartSubtransport.Close()") - t.con.m.Lock() - defer t.con.m.Unlock() t.con.currentStream = nil if t.con.client != nil && t.stdin != nil { @@ -349,8 +344,10 @@ func (t *sshSmartSubtransport) Close() error { _ = t.con.client.Close() t.logger.V(logger.TraceLevel).Info("close client") } + t.con.client = nil t.con.connected = false + atomic.AddInt32(t.closedSessions, 1) return nil } From a530c5dee21e8bd612a94463a0400f7ffa923535 Mon Sep 17 00:00:00 2001 From: Sunny Date: Tue, 21 Jun 2022 17:48:49 +0530 Subject: [PATCH 2/2] libgit2/ssh: Embed connection fields in Subtransport The connection type was created to group the connection related fields and use mutex to prevent race conditions. Since that's no longer the case, this puts back those fields in sshSmartSubtransport. Signed-off-by: Sunny --- pkg/git/libgit2/managed/ssh.go | 48 ++++++++++++++++------------------ 1 file changed, 22 insertions(+), 26 deletions(-) diff --git a/pkg/git/libgit2/managed/ssh.go b/pkg/git/libgit2/managed/ssh.go index 32553797e..1c11afe86 100644 --- a/pkg/git/libgit2/managed/ssh.go +++ b/pkg/git/libgit2/managed/ssh.go @@ -114,10 +114,6 @@ type sshSmartSubtransport struct { closedSessions *int32 - con connection -} - -type connection struct { client *ssh.Client session *ssh.Session currentStream *sshSmartSubtransportStream @@ -155,17 +151,17 @@ func (t *sshSmartSubtransport) Action(transportOptionsURL string, action git2go. var cmd string switch action { case git2go.SmartServiceActionUploadpackLs, git2go.SmartServiceActionUploadpack: - if t.con.currentStream != nil { + if t.currentStream != nil { if t.lastAction == git2go.SmartServiceActionUploadpackLs { - return t.con.currentStream, nil + return t.currentStream, nil } } cmd = fmt.Sprintf("git-upload-pack '%s'", uPath) case git2go.SmartServiceActionReceivepackLs, git2go.SmartServiceActionReceivepack: - if t.con.currentStream != nil { + if t.currentStream != nil { if t.lastAction == git2go.SmartServiceActionReceivepackLs { - return t.con.currentStream, nil + return t.currentStream, nil } } cmd = fmt.Sprintf("git-receive-pack '%s'", uPath) @@ -212,7 +208,7 @@ func (t *sshSmartSubtransport) Action(transportOptionsURL string, action git2go. return nil } - if t.con.connected { + if t.connected { // The connection is no longer shared across actions, so ensures // all has been released before starting a new connection. _ = t.Close() @@ -224,18 +220,18 @@ func (t *sshSmartSubtransport) Action(transportOptionsURL string, action git2go. } t.logger.V(logger.TraceLevel).Info("creating new ssh session") - if t.con.session, err = t.con.client.NewSession(); err != nil { + if t.session, err = t.client.NewSession(); err != nil { return nil, err } - if t.stdin, err = t.con.session.StdinPipe(); err != nil { + if t.stdin, err = t.session.StdinPipe(); err != nil { return nil, err } var w *io.PipeWriter var reader io.Reader t.stdout, w = io.Pipe() - if reader, err = t.con.session.StdoutPipe(); err != nil { + if reader, err = t.session.StdoutPipe(); err != nil { return nil, err } @@ -284,16 +280,16 @@ func (t *sshSmartSubtransport) Action(transportOptionsURL string, action git2go. }() t.logger.V(logger.TraceLevel).Info("run on remote", "cmd", cmd) - if err := t.con.session.Start(cmd); err != nil { + if err := t.session.Start(cmd); err != nil { return nil, err } t.lastAction = action - t.con.currentStream = &sshSmartSubtransportStream{ + t.currentStream = &sshSmartSubtransportStream{ owner: t, } - return t.con.currentStream, nil + return t.currentStream, nil } func (t *sshSmartSubtransport) createConn(addr string, sshConfig *ssh.ClientConfig) error { @@ -310,8 +306,8 @@ func (t *sshSmartSubtransport) createConn(addr string, sshConfig *ssh.ClientConf return err } - t.con.connected = true - t.con.client = ssh.NewClient(c, chans, reqs) + t.connected = true + t.client = ssh.NewClient(c, chans, reqs) return nil } @@ -328,25 +324,25 @@ func (t *sshSmartSubtransport) createConn(addr string, sshConfig *ssh.ClientConf func (t *sshSmartSubtransport) Close() error { t.logger.V(logger.TraceLevel).Info("sshSmartSubtransport.Close()") - t.con.currentStream = nil - if t.con.client != nil && t.stdin != nil { + t.currentStream = nil + if t.client != nil && t.stdin != nil { _ = t.stdin.Close() } t.stdin = nil - if t.con.session != nil { + if t.session != nil { t.logger.V(logger.TraceLevel).Info("session.Close()") - _ = t.con.session.Close() + _ = t.session.Close() } - t.con.session = nil + t.session = nil - if t.con.client != nil { - _ = t.con.client.Close() + if t.client != nil { + _ = t.client.Close() t.logger.V(logger.TraceLevel).Info("close client") } - t.con.client = nil + t.client = nil - t.con.connected = false + t.connected = false atomic.AddInt32(t.closedSessions, 1) return nil