aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorEd Bardsley <ewb@uber.com>2019-06-03 21:48:08 -0700
committerEd Bardsley <ewb@uber.com>2019-06-03 21:59:19 -0700
commitec647e9f02339a93c2856cc93b6b1b6090a36c06 (patch)
tree7dc31884db392754f7bf52c027e06434e7962ab5
parent37b80726760d2e0b17dfa437f3162dd930590ecf (diff)
downloadgo-git-ec647e9f02339a93c2856cc93b6b1b6090a36c06.tar.gz
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 <ewb@uber.com>
-rw-r--r--plumbing/format/packfile/diff_delta.go13
-rw-r--r--plumbing/format/packfile/packfile.go8
2 files changed, 10 insertions, 11 deletions
diff --git a/plumbing/format/packfile/diff_delta.go b/plumbing/format/packfile/diff_delta.go
index d35e78a..43f87a0 100644
--- a/plumbing/format/packfile/diff_delta.go
+++ b/plumbing/format/packfile/diff_delta.go
@@ -40,8 +40,8 @@ func getDelta(index *deltaIndex, base, target plumbing.EncodedObject) (plumbing.
defer tr.Close()
bb := bufPool.Get().(*bytes.Buffer)
- bb.Reset()
defer bufPool.Put(bb)
+ bb.Reset()
_, err = bb.ReadFrom(br)
if err != nil {
@@ -49,8 +49,8 @@ func getDelta(index *deltaIndex, base, target plumbing.EncodedObject) (plumbing.
}
tb := bufPool.Get().(*bytes.Buffer)
- tb.Reset()
defer bufPool.Put(tb)
+ tb.Reset()
_, err = tb.ReadFrom(tr)
if err != nil {
@@ -77,6 +77,7 @@ func DiffDelta(src, tgt []byte) []byte {
func diffDelta(index *deltaIndex, src []byte, tgt []byte) []byte {
buf := bufPool.Get().(*bytes.Buffer)
+ defer bufPool.Put(buf)
buf.Reset()
buf.Write(deltaEncodeSize(len(src)))
buf.Write(deltaEncodeSize(len(tgt)))
@@ -86,6 +87,7 @@ func diffDelta(index *deltaIndex, src []byte, tgt []byte) []byte {
}
ibuf := bufPool.Get().(*bytes.Buffer)
+ defer bufPool.Put(ibuf)
ibuf.Reset()
for i := 0; i < len(tgt); i++ {
offset, l := index.findMatch(src, tgt, i)
@@ -127,12 +129,9 @@ func diffDelta(index *deltaIndex, src []byte, tgt []byte) []byte {
}
encodeInsertOperation(ibuf, buf)
- bytes := buf.Bytes()
-
- bufPool.Put(buf)
- bufPool.Put(ibuf)
- return bytes
+ // buf.Bytes() is only valid until the next modifying operation on the buffer. Copy it.
+ return append([]byte{}, buf.Bytes()...)
}
func encodeInsertOperation(ibuf, buf *bytes.Buffer) {
diff --git a/plumbing/format/packfile/packfile.go b/plumbing/format/packfile/packfile.go
index f528073..21a15de 100644
--- a/plumbing/format/packfile/packfile.go
+++ b/plumbing/format/packfile/packfile.go
@@ -133,8 +133,8 @@ func (p *Packfile) getObjectSize(h *ObjectHeader) (int64, error) {
return h.Length, nil
case plumbing.REFDeltaObject, plumbing.OFSDeltaObject:
buf := bufPool.Get().(*bytes.Buffer)
- buf.Reset()
defer bufPool.Put(buf)
+ buf.Reset()
if _, _, err := p.s.NextObject(buf); err != nil {
return 0, err
@@ -222,11 +222,11 @@ func (p *Packfile) getNextObject(h *ObjectHeader, hash plumbing.Hash) (plumbing.
// optimization only if the expanded version of the object still meets
// the small object threshold condition.
buf := bufPool.Get().(*bytes.Buffer)
+ defer bufPool.Put(buf)
buf.Reset()
if _, _, err := p.s.NextObject(buf); err != nil {
return nil, err
}
- defer bufPool.Put(buf)
size = p.getDeltaObjectSize(buf)
if size <= smallObjectThreshold {
@@ -321,12 +321,12 @@ func (p *Packfile) fillRegularObjectContent(obj plumbing.EncodedObject) error {
func (p *Packfile) fillREFDeltaObjectContent(obj plumbing.EncodedObject, ref plumbing.Hash) error {
buf := bufPool.Get().(*bytes.Buffer)
+ defer bufPool.Put(buf)
buf.Reset()
_, _, err := p.s.NextObject(buf)
if err != nil {
return err
}
- defer bufPool.Put(buf)
return p.fillREFDeltaObjectContentWithBuffer(obj, ref, buf)
}
@@ -351,12 +351,12 @@ func (p *Packfile) fillREFDeltaObjectContentWithBuffer(obj plumbing.EncodedObjec
func (p *Packfile) fillOFSDeltaObjectContent(obj plumbing.EncodedObject, offset int64) error {
buf := bufPool.Get().(*bytes.Buffer)
+ defer bufPool.Put(buf)
buf.Reset()
_, _, err := p.s.NextObject(buf)
if err != nil {
return err
}
- defer bufPool.Put(buf)
return p.fillOFSDeltaObjectContentWithBuffer(obj, offset, buf)
}