Skip to content

Commit 09fae63

Browse files
author
Paulo Gomes
committed
libgit2: remove deadlock
Some scenarios may lead to deadlocks, specially in image automation controller. Signed-off-by: Paulo Gomes <paulo.gomes@weave.works>
1 parent 812f6e4 commit 09fae63

File tree

1 file changed

+13
-16
lines changed

1 file changed

+13
-16
lines changed

pkg/git/libgit2/managed/ssh.go

+13-16
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,7 @@ import (
5454
"runtime"
5555
"strings"
5656
"sync"
57+
"sync/atomic"
5758
"time"
5859

5960
"golang.org/x/crypto/ssh"
@@ -80,10 +81,12 @@ func registerManagedSSH() error {
8081
}
8182

8283
func sshSmartSubtransportFactory(remote *git2go.Remote, transport *git2go.Transport) (git2go.SmartSubtransport, error) {
84+
var closed int32 = 0
8385
return &sshSmartSubtransport{
84-
transport: transport,
85-
ctx: context.Background(),
86-
logger: logr.Discard(),
86+
transport: transport,
87+
ctx: context.Background(),
88+
logger: logr.Discard(),
89+
closedSessions: &closed,
8790
}, nil
8891
}
8992

@@ -109,6 +112,8 @@ type sshSmartSubtransport struct {
109112
stdin io.WriteCloser
110113
stdout io.Reader
111114

115+
closedSessions *int32
116+
112117
con connection
113118
}
114119

@@ -117,7 +122,6 @@ type connection struct {
117122
session *ssh.Session
118123
currentStream *sshSmartSubtransportStream
119124
connected bool
120-
m sync.RWMutex
121125
}
122126

123127
func (t *sshSmartSubtransport) Action(transportOptionsURL string, action git2go.SmartServiceAction) (git2go.SmartSubtransportStream, error) {
@@ -208,13 +212,11 @@ func (t *sshSmartSubtransport) Action(transportOptionsURL string, action git2go.
208212
return nil
209213
}
210214

211-
t.con.m.RLock()
212-
if t.con.connected == true {
215+
if t.con.connected {
213216
// The connection is no longer shared across actions, so ensures
214217
// all has been released before starting a new connection.
215218
_ = t.Close()
216219
}
217-
t.con.m.RUnlock()
218220

219221
err = t.createConn(addr, sshConfig)
220222
if err != nil {
@@ -251,7 +253,6 @@ func (t *sshSmartSubtransport) Action(transportOptionsURL string, action git2go.
251253
"recovered from libgit2 ssh smart subtransport panic")
252254
}
253255
}()
254-
255256
var cancel context.CancelFunc
256257
ctx := t.ctx
257258

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

265+
closedAlready := atomic.LoadInt32(t.closedSessions)
264266
for {
265267
select {
266268
case <-ctx.Done():
267269
t.Close()
268270
return nil
269271

270272
default:
271-
t.con.m.RLock()
272-
if !t.con.connected {
273-
t.con.m.RUnlock()
273+
if atomic.LoadInt32(t.closedSessions) > closedAlready {
274274
return nil
275275
}
276-
t.con.m.RUnlock()
277276

278277
_, err := io.Copy(w, reader)
279278
if err != nil {
@@ -311,10 +310,8 @@ func (t *sshSmartSubtransport) createConn(addr string, sshConfig *ssh.ClientConf
311310
return err
312311
}
313312

314-
t.con.m.Lock()
315313
t.con.connected = true
316314
t.con.client = ssh.NewClient(c, chans, reqs)
317-
t.con.m.Unlock()
318315

319316
return nil
320317
}
@@ -330,8 +327,6 @@ func (t *sshSmartSubtransport) createConn(addr string, sshConfig *ssh.ClientConf
330327
// SmartSubTransport (i.e. unreleased resources, staled connections).
331328
func (t *sshSmartSubtransport) Close() error {
332329
t.logger.V(logger.TraceLevel).Info("sshSmartSubtransport.Close()")
333-
t.con.m.Lock()
334-
defer t.con.m.Unlock()
335330

336331
t.con.currentStream = nil
337332
if t.con.client != nil && t.stdin != nil {
@@ -349,8 +344,10 @@ func (t *sshSmartSubtransport) Close() error {
349344
_ = t.con.client.Close()
350345
t.logger.V(logger.TraceLevel).Info("close client")
351346
}
347+
t.con.client = nil
352348

353349
t.con.connected = false
350+
atomic.AddInt32(t.closedSessions, 1)
354351

355352
return nil
356353
}

0 commit comments

Comments
 (0)