From 504e52b45e8350d0b64ae52f4b4307fd5d6b23d1 Mon Sep 17 00:00:00 2001 From: Harshavardhana Date: Wed, 28 Aug 2024 18:40:52 -0700 Subject: [PATCH] protect bpool from buffer pollution by invalid buffers (#20342) --- cmd/erasure-decode.go | 14 +++---- cmd/object-api-deleteobject_test.go | 8 +++- internal/bpool/bpool.go | 40 ++++++++++++++----- internal/bpool/bpool_test.go | 62 ++++++++++++++++------------- 4 files changed, 77 insertions(+), 47 deletions(-) diff --git a/cmd/erasure-decode.go b/cmd/erasure-decode.go index 6f3ecab02..f0cc90ab0 100644 --- a/cmd/erasure-decode.go +++ b/cmd/erasure-decode.go @@ -48,14 +48,14 @@ func newParallelReader(readers []io.ReaderAt, e Erasure, offset, totalLength int r2b[i] = i } bufs := make([][]byte, len(readers)) - // Fill buffers - b := globalBytePoolCap.Load().Get() shardSize := int(e.ShardSize()) - if cap(b) < len(readers)*shardSize { - // We should always have enough capacity, but older objects may be bigger. - globalBytePoolCap.Load().Put(b) - b = nil - } else { + var b []byte + + // We should always have enough capacity, but older objects may be bigger + // we do not need stashbuffer for them. + if globalBytePoolCap.Load().WidthCap() >= len(readers)*shardSize { + // Fill buffers + b = globalBytePoolCap.Load().Get() // Seed the buffers. for i := range bufs { bufs[i] = b[i*shardSize : (i+1)*shardSize] diff --git a/cmd/object-api-deleteobject_test.go b/cmd/object-api-deleteobject_test.go index eef575493..b7e888e4e 100644 --- a/cmd/object-api-deleteobject_test.go +++ b/cmd/object-api-deleteobject_test.go @@ -93,14 +93,18 @@ func testDeleteObject(obj ObjectLayer, instanceType string, t TestErrHandler) { md5Bytes := md5.Sum([]byte(object.content)) oi, err := obj.PutObject(context.Background(), testCase.bucketName, object.name, mustGetPutObjReader(t, strings.NewReader(object.content), int64(len(object.content)), hex.EncodeToString(md5Bytes[:]), ""), ObjectOptions{}) - t.Log(oi) if err != nil { + t.Log(oi) t.Fatalf("%s : %s", instanceType, err.Error()) } } oi, err := obj.DeleteObject(context.Background(), testCase.bucketName, testCase.pathToDelete, ObjectOptions{}) - t.Log(oi, err) + if err != nil && !isErrObjectNotFound(err) { + t.Log(oi) + t.Errorf("Test %d: %s: Expected to pass, but failed with: %s", i+1, instanceType, err) + continue + } result, err := obj.ListObjects(context.Background(), testCase.bucketName, "", "", "", 1000) if err != nil { diff --git a/internal/bpool/bpool.go b/internal/bpool/bpool.go index 2cbf88e8c..c7282f0e0 100644 --- a/internal/bpool/bpool.go +++ b/internal/bpool/bpool.go @@ -1,4 +1,4 @@ -// Copyright (c) 2015-2023 MinIO, Inc. +// Copyright (c) 2015-2024 MinIO, Inc. // // This file is part of MinIO Object Storage stack // @@ -17,7 +17,9 @@ package bpool -import "github.com/klauspost/reedsolomon" +import ( + "github.com/klauspost/reedsolomon" +) // BytePoolCap implements a leaky pool of []byte in the form of a bounded channel. type BytePoolCap struct { @@ -29,11 +31,14 @@ type BytePoolCap struct { // NewBytePoolCap creates a new BytePool bounded to the given maxSize, with new // byte arrays sized based on width. func NewBytePoolCap(maxSize uint64, width int, capwidth int) (bp *BytePoolCap) { - if capwidth > 0 && capwidth < 64 { + if capwidth <= 0 { + panic("total buffer capacity must be provided") + } + if capwidth < 64 { panic("buffer capped with smaller than 64 bytes is not supported") } - if capwidth > 0 && width > capwidth { - panic("buffer length cannot be > capacity of the buffer") + if width > capwidth { + panic("minimum buffer length cannot be > capacity of the buffer") } return &BytePoolCap{ c: make(chan []byte, maxSize), @@ -60,11 +65,7 @@ func (bp *BytePoolCap) Get() (b []byte) { // reuse existing buffer default: // create new aligned buffer - if bp.wcap > 0 { - b = reedsolomon.AllocAligned(1, bp.wcap)[0][:bp.w] - } else { - b = reedsolomon.AllocAligned(1, bp.w)[0] - } + b = reedsolomon.AllocAligned(1, bp.wcap)[0][:bp.w] } return } @@ -74,8 +75,17 @@ func (bp *BytePoolCap) Put(b []byte) { if bp == nil { return } + + if cap(b) != bp.wcap { + // someone tried to put back buffer which is not part of this buffer pool + // we simply don't put this back into pool, a modified buffer provided + // by this package is no more usable, callers make sure to not modify + // the capacity of the buffer. + return + } + select { - case bp.c <- b: + case bp.c <- b[:bp.w]: // buffer went back into pool default: // buffer didn't go back into pool, just discard @@ -97,3 +107,11 @@ func (bp *BytePoolCap) WidthCap() (n int) { } return bp.wcap } + +// CurrentSize returns current size of buffer pool +func (bp *BytePoolCap) CurrentSize() int { + if bp == nil { + return 0 + } + return len(bp.c) * bp.w +} diff --git a/internal/bpool/bpool_test.go b/internal/bpool/bpool_test.go index 1b122284e..a91d9ad43 100644 --- a/internal/bpool/bpool_test.go +++ b/internal/bpool/bpool_test.go @@ -17,7 +17,9 @@ package bpool -import "testing" +import ( + "testing" +) // Tests - bytePool functionality. func TestBytePool(t *testing.T) { @@ -25,20 +27,20 @@ func TestBytePool(t *testing.T) { width := 1024 capWidth := 2048 - bufPool := NewBytePoolCap(size, width, capWidth) + bp := NewBytePoolCap(size, width, capWidth) // Check the width - if bufPool.Width() != width { - t.Fatalf("bytepool width invalid: got %v want %v", bufPool.Width(), width) + if bp.Width() != width { + t.Fatalf("bytepool width invalid: got %v want %v", bp.Width(), width) } // Check with width cap - if bufPool.WidthCap() != capWidth { - t.Fatalf("bytepool capWidth invalid: got %v want %v", bufPool.WidthCap(), capWidth) + if bp.WidthCap() != capWidth { + t.Fatalf("bytepool capWidth invalid: got %v want %v", bp.WidthCap(), capWidth) } // Check that retrieved buffer are of the expected width - b := bufPool.Get() + b := bp.Get() if len(b) != width { t.Fatalf("bytepool length invalid: got %v want %v", len(b), width) } @@ -46,14 +48,14 @@ func TestBytePool(t *testing.T) { t.Fatalf("bytepool cap invalid: got %v want %v", cap(b), capWidth) } - bufPool.Put(b) + bp.Put(b) // Fill the pool beyond the capped pool size. for i := uint64(0); i < size*2; i++ { - bufPool.Put(make([]byte, bufPool.w)) + bp.Put(make([]byte, bp.w, bp.wcap)) } - b = bufPool.Get() + b = bp.Get() if len(b) != width { t.Fatalf("bytepool length invalid: got %v want %v", len(b), width) } @@ -61,31 +63,37 @@ func TestBytePool(t *testing.T) { t.Fatalf("bytepool length invalid: got %v want %v", cap(b), capWidth) } - bufPool.Put(b) - - // Close the channel so we can iterate over it. - close(bufPool.c) + bp.Put(b) // Check the size of the pool. - if uint64(len(bufPool.c)) != size { - t.Fatalf("bytepool size invalid: got %v want %v", len(bufPool.c), size) + if uint64(len(bp.c)) != size { + t.Fatalf("bytepool size invalid: got %v want %v", len(bp.c), size) } - bufPoolNoCap := NewBytePoolCap(size, width, 0) - // Check the width - if bufPoolNoCap.Width() != width { - t.Fatalf("bytepool width invalid: got %v want %v", bufPool.Width(), width) + // lets drain the buf channel first before we validate invalid buffers. + for i := uint64(0); i < size; i++ { + bp.Get() // discard } - // Check with width cap - if bufPoolNoCap.WidthCap() != 0 { - t.Fatalf("bytepool capWidth invalid: got %v want %v", bufPool.WidthCap(), 0) + // Try putting some invalid buffers into pool + bp.Put(make([]byte, bp.w, bp.wcap-1)) // wrong capacity is rejected (less) + bp.Put(make([]byte, bp.w, bp.wcap+1)) // wrong capacity is rejected (more) + bp.Put(make([]byte, width)) // wrong capacity is rejected (very less) + if len(bp.c) > 0 { + t.Fatal("bytepool should have rejected invalid packets") } - b = bufPoolNoCap.Get() + + // Try putting a short slice into pool + bp.Put(make([]byte, bp.w, bp.wcap)[:2]) + if len(bp.c) != 1 { + t.Fatal("bytepool should have accepted short slice with sufficient capacity") + } + + b = bp.Get() if len(b) != width { t.Fatalf("bytepool length invalid: got %v want %v", len(b), width) } - if cap(b) != width { - t.Fatalf("bytepool length invalid: got %v want %v", cap(b), width) - } + + // Close the channel. + close(bp.c) }