diff options
author | Ed Bardsley <ewb@uber.com> | 2019-06-03 21:48:08 -0700 |
---|---|---|
committer | Ed Bardsley <ewb@uber.com> | 2019-06-03 21:59:19 -0700 |
commit | ec647e9f02339a93c2856cc93b6b1b6090a36c06 (patch) | |
tree | 7dc31884db392754f7bf52c027e06434e7962ab5 | |
parent | 37b80726760d2e0b17dfa437f3162dd930590ecf (diff) | |
download | go-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.go | 13 | ||||
-rw-r--r-- | plumbing/format/packfile/packfile.go | 8 |
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) } |