Skip to content
This repository was archived by the owner on Sep 11, 2020. It is now read-only.

Commit ec647e9

Browse files
ebardsleyebardsley
committed
plumbing: format/packfile, Fix data race and resource leak.
Two problems are fixed: - Buffers are not returned to the pool in the case of errors. - Per https://golang.org/pkg/bytes/#Buffer.Bytes, the slice returned from bytes.Buffer.Bytes() is only valid until the next modification of the buffer, so it must be copied before the buffer is returned to the pool. Running `go test -race` detected the following: ``` ================== WARNING: DATA RACE Write at 0x00c000224020 by goroutine 15: bytes.(*Buffer).WriteByte() /usr/local/Cellar/go/1.11.5/libexec/src/bytes/buffer.go:271 +0xc8 vendor/gopkg.in/src-d/go-git.v4/plumbing/format/packfile.diffDelta() vendor/gopkg.in/src-d/go-git.v4/plumbing/format/packfile/diff_delta.go:95 +0x505 vendor/gopkg.in/src-d/go-git.v4/plumbing/format/packfile.getDelta() vendor/gopkg.in/src-d/go-git.v4/plumbing/format/packfile/diff_delta.go:60 +0x4ae vendor/gopkg.in/src-d/go-git.v4/plumbing/format/packfile.(*deltaSelector).tryToDeltify() vendor/gopkg.in/src-d/go-git.v4/plumbing/format/packfile/delta_selector.go:309 +0x398 vendor/gopkg.in/src-d/go-git.v4/plumbing/format/packfile.(*deltaSelector).walk() vendor/gopkg.in/src-d/go-git.v4/plumbing/format/packfile/delta_selector.go:262 +0x33b vendor/gopkg.in/src-d/go-git.v4/plumbing/format/packfile.(*deltaSelector).ObjectsToPack.func1() vendor/gopkg.in/src-d/go-git.v4/plumbing/format/packfile/delta_selector.go:70 +0x6a Previous read at 0x00c000224020 by goroutine 8: runtime.slicecopy() /usr/local/Cellar/go/1.11.5/libexec/src/runtime/slice.go:221 +0x0 vendor/gopkg.in/src-d/go-git.v4/plumbing/format/packfile.getDelta() vendor/gopkg.in/src-d/go-git.v4/plumbing/memory.go:53 +0x5e2 vendor/gopkg.in/src-d/go-git.v4/plumbing/format/packfile.(*deltaSelector).tryToDeltify() vendor/gopkg.in/src-d/go-git.v4/plumbing/format/packfile/delta_selector.go:309 +0x398 vendor/gopkg.in/src-d/go-git.v4/plumbing/format/packfile.(*deltaSelector).walk() vendor/gopkg.in/src-d/go-git.v4/plumbing/format/packfile/delta_selector.go:262 +0x33b vendor/gopkg.in/src-d/go-git.v4/plumbing/format/packfile.(*deltaSelector).ObjectsToPack.func1() vendor/gopkg.in/src-d/go-git.v4/plumbing/format/packfile/delta_selector.go:70 +0x6a Goroutine 15 (running) created at: vendor/gopkg.in/src-d/go-git.v4/plumbing/format/packfile.(*deltaSelector).ObjectsToPack() vendor/gopkg.in/src-d/go-git.v4/plumbing/format/packfile/delta_selector.go:69 +0x761 vendor/gopkg.in/src-d/go-git.v4/plumbing/format/packfile.(*Encoder).Encode() vendor/gopkg.in/src-d/go-git.v4/plumbing/format/packfile/encoder.go:52 +0xb1 vendor/gopkg.in/src-d/go-git%2ev4.pushHashes.func1() vendor/gopkg.in/src-d/go-git.v4/remote.go:1026 +0x102 Goroutine 8 (finished) created at: vendor/gopkg.in/src-d/go-git.v4/plumbing/format/packfile.(*deltaSelector).ObjectsToPack() vendor/gopkg.in/src-d/go-git.v4/plumbing/format/packfile/delta_selector.go:69 +0x761 vendor/gopkg.in/src-d/go-git.v4/plumbing/format/packfile.(*Encoder).Encode() vendor/gopkg.in/src-d/go-git.v4/plumbing/format/packfile/encoder.go:52 +0xb1 vendor/gopkg.in/src-d/go-git%2ev4.pushHashes.func1() vendor/gopkg.in/src-d/go-git.v4/remote.go:1026 +0x102 ================== ``` Signed-off-by: Ed Bardsley <[email protected]>
1 parent 37b8072 commit ec647e9

File tree

2 files changed

+10
-11
lines changed

2 files changed

+10
-11
lines changed

‎plumbing/format/packfile/diff_delta.go

+6-7
Original file line numberDiff line numberDiff line change
@@ -40,17 +40,17 @@ func getDelta(index *deltaIndex, base, target plumbing.EncodedObject) (plumbing.
4040
defer tr.Close()
4141

4242
bb := bufPool.Get().(*bytes.Buffer)
43-
bb.Reset()
4443
defer bufPool.Put(bb)
44+
bb.Reset()
4545

4646
_, err = bb.ReadFrom(br)
4747
if err != nil {
4848
return nil, err
4949
}
5050

5151
tb := bufPool.Get().(*bytes.Buffer)
52-
tb.Reset()
5352
defer bufPool.Put(tb)
53+
tb.Reset()
5454

5555
_, err = tb.ReadFrom(tr)
5656
if err != nil {
@@ -77,6 +77,7 @@ func DiffDelta(src, tgt []byte) []byte {
7777

7878
func diffDelta(index *deltaIndex, src []byte, tgt []byte) []byte {
7979
buf := bufPool.Get().(*bytes.Buffer)
80+
defer bufPool.Put(buf)
8081
buf.Reset()
8182
buf.Write(deltaEncodeSize(len(src)))
8283
buf.Write(deltaEncodeSize(len(tgt)))
@@ -86,6 +87,7 @@ func diffDelta(index *deltaIndex, src []byte, tgt []byte) []byte {
8687
}
8788

8889
ibuf := bufPool.Get().(*bytes.Buffer)
90+
defer bufPool.Put(ibuf)
8991
ibuf.Reset()
9092
for i := 0; i < len(tgt); i++ {
9193
offset, l := index.findMatch(src, tgt, i)
@@ -127,12 +129,9 @@ func diffDelta(index *deltaIndex, src []byte, tgt []byte) []byte {
127129
}
128130

129131
encodeInsertOperation(ibuf, buf)
130-
bytes := buf.Bytes()
131-
132-
bufPool.Put(buf)
133-
bufPool.Put(ibuf)
134132

135-
return bytes
133+
// buf.Bytes() is only valid until the next modifying operation on the buffer. Copy it.
134+
return append([]byte{}, buf.Bytes()...)
136135
}
137136

138137
func encodeInsertOperation(ibuf, buf *bytes.Buffer) {

‎plumbing/format/packfile/packfile.go

+4-4
Original file line numberDiff line numberDiff line change
@@ -133,8 +133,8 @@ func (p *Packfile) getObjectSize(h *ObjectHeader) (int64, error) {
133133
return h.Length, nil
134134
case plumbing.REFDeltaObject, plumbing.OFSDeltaObject:
135135
buf := bufPool.Get().(*bytes.Buffer)
136-
buf.Reset()
137136
defer bufPool.Put(buf)
137+
buf.Reset()
138138

139139
if _, _, err := p.s.NextObject(buf); err != nil {
140140
return 0, err
@@ -222,11 +222,11 @@ func (p *Packfile) getNextObject(h *ObjectHeader, hash plumbing.Hash) (plumbing.
222222
// optimization only if the expanded version of the object still meets
223223
// the small object threshold condition.
224224
buf := bufPool.Get().(*bytes.Buffer)
225+
defer bufPool.Put(buf)
225226
buf.Reset()
226227
if _, _, err := p.s.NextObject(buf); err != nil {
227228
return nil, err
228229
}
229-
defer bufPool.Put(buf)
230230

231231
size = p.getDeltaObjectSize(buf)
232232
if size <= smallObjectThreshold {
@@ -321,12 +321,12 @@ func (p *Packfile) fillRegularObjectContent(obj plumbing.EncodedObject) error {
321321

322322
func (p *Packfile) fillREFDeltaObjectContent(obj plumbing.EncodedObject, ref plumbing.Hash) error {
323323
buf := bufPool.Get().(*bytes.Buffer)
324+
defer bufPool.Put(buf)
324325
buf.Reset()
325326
_, _, err := p.s.NextObject(buf)
326327
if err != nil {
327328
return err
328329
}
329-
defer bufPool.Put(buf)
330330

331331
return p.fillREFDeltaObjectContentWithBuffer(obj, ref, buf)
332332
}
@@ -351,12 +351,12 @@ func (p *Packfile) fillREFDeltaObjectContentWithBuffer(obj plumbing.EncodedObjec
351351

352352
func (p *Packfile) fillOFSDeltaObjectContent(obj plumbing.EncodedObject, offset int64) error {
353353
buf := bufPool.Get().(*bytes.Buffer)
354+
defer bufPool.Put(buf)
354355
buf.Reset()
355356
_, _, err := p.s.NextObject(buf)
356357
if err != nil {
357358
return err
358359
}
359-
defer bufPool.Put(buf)
360360

361361
return p.fillOFSDeltaObjectContentWithBuffer(obj, offset, buf)
362362
}

0 commit comments

Comments
 (0)