protect bpool from buffer pollution by invalid buffers (minio#20342)

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]

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: <ERROR> %s", i+1, instanceType, err)
+			continue
+		}
 
 		result, err := obj.ListObjects(context.Background(), testCase.bucketName, "", "", "", 1000)
 		if err != nil {

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
+}

internal/bpool/bpool_test.go

@@ -17,75 +17,83 @@
 
 package bpool
 
-import "testing"
+import (
+	"testing"
+)
 
 // Tests - bytePool functionality.
 func TestBytePool(t *testing.T) {
 	size := uint64(4)
 	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)
 	}
 	if cap(b) != capWidth {
 		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)
 	}
 	if cap(b) != capWidth {
 		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")
+	}
+
+	// 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 = bufPoolNoCap.Get()
+
+	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)
 }