Skip to content

Commit

Permalink
libgit2: remove deadlock
Browse files Browse the repository at this point in the history
Some scenarios may lead to deadlocks, specially
in image automation controller.

Signed-off-by: Paulo Gomes <paulo.gomes@weave.works>
  • Loading branch information
Paulo Gomes committed Jun 16, 2022
1 parent 0a993a8 commit d5535b2
Showing 1 changed file with 13 additions and 16 deletions.
29 changes: 13 additions & 16 deletions pkg/git/libgit2/managed/ssh.go
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ import (
"runtime"
"strings"
"sync"
"sync/atomic"
"time"

"golang.org/x/crypto/ssh"
Expand All @@ -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
}

Expand All @@ -109,6 +112,8 @@ type sshSmartSubtransport struct {
stdin io.WriteCloser
stdout io.Reader

closedSessions *int32

con connection
}

Expand All @@ -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) {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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

Expand All @@ -261,19 +262,17 @@ func (t *sshSmartSubtransport) Action(transportOptionsURL string, action git2go.
defer cancel()
}

closedAlready := atomic.LoadInt32(t.closedSessions)
for {
select {
case <-ctx.Done():
t.Close()
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 {
Expand Down Expand Up @@ -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
}
Expand All @@ -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 {
Expand All @@ -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
}
Expand Down

0 comments on commit d5535b2

Please sign in to comment.