libp2p / go-buffer-pool Goto Github PK

Running package tests locally occasionally fails with

--- FAIL: TestAllocations (0.00s)
	~/go/src/github.com/libp2p/go-buffer-pool/pool_test.go:31: expected less than 100 frees after GC, got 527
FAIL
coverage: 96.4% of statements
FAIL	github.com/libp2p/go-buffer-pool	0.323s
Error: Tests failed.

@Stebalien should I investigate the failure? I see most of the code was taken from Go source with small modifications. I wanted to add Travis build and coverage files to the repo and ran into this.

This is what we get from time to time. Unfortunately I can not provide a way to replicate this.
Shouldn't have been something like:

if length > MaxLength {
		return make([]byte, MaxLength)
}

Thanks

panic: runtime error: makeslice: len out of range

goroutine 12857 [running]:
github.com/libp2p/go-buffer-pool.(*BufferPool).Get(0xc0000ef080, 0xff898405317b5, 0x400, 0x400, 0x2a3)
C:/Users/user/go/pkg/mod/github.com/libp2p/[email protected]/pool.go:62 +0x1a9
github.com/libp2p/go-buffer-pool.Get(...)
C:/Users/user/go/pkg/mod/github.com/libp2p/[email protected]/pool.go:95
github.com/libp2p/go-libp2p-secio.(*etmWriter).WriteMsg(0xc00d26a100, 0xc00abf8000, 0xff89840531791, 0xf8ff837e5bfd2, 0x0, 0x0)
C:/Users/user/go/pkg/mod/github.com/libp2p/[email protected]/rw.go:46 +0xd3
github.com/libp2p/go-libp2p-secio.(*etmWriter).Write(0xc00d26a100, 0xc00abf8000, 0xff89840531791, 0xf8ff837e5bfd2, 0xbf61f94125d7448b, 0x163e931c553, 0x2b66840)
C:/Users/user/go/pkg/mod/github.com/libp2p/[email protected]/rw.go:34 +0x4d
github.com/libp2p/go-yamux.(*Session).sendLoop(0xc00d19c0d0, 0x0, 0x0)
C:/Users/user/go/pkg/mod/github.com/libp2p/[email protected]/session.go:472 +0x25f
github.com/libp2p/go-yamux.(*Session).send(0xc00d19c0d0)
C:/Users/user/go/pkg/mod/github.com/libp2p/[email protected]/session.go:384 +0x2b
created by github.com/libp2p/go-yamux.newSession
C:/Users/user/go/pkg/mod/github.com/libp2p/[email protected]/session.go:130 +0x3a8

Hi,

Do you think it is possible to make Buffer implement io.ByteScanner interface? Specifically I mean UnreadByte() error method which is often used together with ReadByte to peek into the byte stream. Looking at the code I guess buffer shrinking may be a problem...

Since #1, I believe the number of pools should be 1 less. However, fixing #1 took priority so we can figure this out later.

I am not sure why this wasn't a problem before but this constant:

const MaxLength = 1 << 32

should probably change to

const MaxLength = math.MaxInt32

Otherwise we will get these errors:

[go-ipfs] Running shell script
+ go build -i -ldflags=-X github.com/ipfs/go-ipfs/repo/config.CurrentCommit=pr-5553-5 -o cmd/ipfs/ipfs github.com/ipfs/go-ipfs/cmd/ipfs
# gx/ipfs/QmUQy76yspPa3fRyY3GzXFTg9n8JVwFru6ue3KFRt4MeTw/go-buffer-pool
../../../gx/ipfs/QmUQy76yspPa3fRyY3GzXFTg9n8JVwFru6ue3KFRt4MeTw/go-buffer-pool/pool.go:55:12: constant 4294967296 overflows int
../../../gx/ipfs/QmUQy76yspPa3fRyY3GzXFTg9n8JVwFru6ue3KFRt4MeTw/go-buffer-pool/pool.go:68:31: constant 4294967296 overflows int
script returned exit code 2

Forgive me for the unsolicited question. I saw this package being used somewhere and didn't quite understand what was going on. It seems to me that a collection of size-classed sync.Pool of *bytes.Buffer would get you all of the advantages enumerated in the README, and none of the drawbacks. But I'm sure I'm missing something! Is there a test or benchmark that shows the case(s) where this approach is better?

var bufferPool = sync.Pool{New: func() interface{} { return &bytes.Buffer{} }

func getBuffer() *bytes.Buffer {
    b := bufferPool.Get().(*bytes.Buffer)
    b.Reset() // important! :)
    return b
}

func putBuffer(b *bytes.Buffer) {
    bufferPool.Put(b)
}

If I pass in a negative value whose uint32 overflown value fits within 31 bits, the code in (*BufferPool).Get or just .Get blindly converts it to a uint32 which wraps around to a very large positive number and causes massive memory allocations that'll cause out of memory errors to be triggered and the process will freeze and hopefully your OS will kill it but essentially it can freeze.

Repro

Here is the repro test that'll consume your RAM fast and freeze your other processes

var sink interface{}

func TestBufferPoolGetWithNegativeValue(t *testing.T) {
    for i := 0; i < 50; i++ {
        b := Get(-20008899456)
        sink = b
        Put(b)
    }
    if sink == nil { 
        t.Fatal("Test didn't run!")
    }   
    sink = (interface{})(nil)
} 

Remedy

The fix for this is rather trivial; when values are less than 0, please return nil ASAP

diff --git a/pool.go b/pool.go
index d812840..a83420e 100644
--- a/pool.go
+++ b/pool.go
@@ -55,7 +55,7 @@ type bufp struct {
 //
 // If no suitable buffer exists in the pool, Get creates one.
 func (p *BufferPool) Get(length int) []byte {
-       if length == 0 {
+       if length <= 0 {
                return nil
        }
        if length > MaxLength {

Suggestion

I kindly ask that if y'all have a bug bounty/security vulnerability reporting process that you paste something in your README.md so that I can report sensitive issues there. This work is courtesy of the Cosmos Network security team program to secure all dependencies. Thank you!

With this package, after a long run, the allocated buffers in the pool seems not GCed even after the Put and got no new Get

rt

On my local machine, this test fails in ~50% of the cases.

❯ go test
--- FAIL: TestPool (0.00s)
    pool_test.go:86: got a; want new slice after GC
FAIL
exit status 1
FAIL	github.com/libp2p/go-buffer-pool	0.271s