mirror of
https://github.com/seaweedfs/seaweedfs.git
synced 2026-07-20 06:52:24 +00:00
fix: report short S3 ReaderAt reads (#10345)
* fix: report short S3 ReaderAt reads Problem: S3BackendStorageFile.ReadAt returned a short buffer with a nil error, hiding truncated remote data. Root cause: Every terminal io.EOF was cleared regardless of how many bytes were read. Fix: Clear io.EOF only when the requested buffer was completely filled. Validation: go test ./weed/storage/backend/... Co-authored-by: Codex <noreply@openai.com> * fix: validate S3 ReaderAt requests Co-authored-by: Codex <noreply@openai.com> * s3 ReadAt: simplify negative offset error * s3 ReadAt: stop reading once the buffer is full Skips the extra Read that only collected the terminal EOF, and bounds the loop if the server ignores the Range header and returns more data than requested, where Read on an empty slice can spin forever. * s3 ReadAt: cover full reads that arrive with io.EOF --------- Co-authored-by: Codex <noreply@openai.com> Co-authored-by: Chris Lu <chris.lu@gmail.com>
This commit is contained in:
@@ -129,6 +129,15 @@ type S3BackendStorageFile struct {
|
||||
}
|
||||
|
||||
func (s3backendStorageFile S3BackendStorageFile) ReadAt(p []byte, off int64) (n int, err error) {
|
||||
if off < 0 {
|
||||
return 0, fmt.Errorf("negative offset %d", off)
|
||||
}
|
||||
|
||||
requested := len(p)
|
||||
if requested == 0 {
|
||||
return 0, nil
|
||||
}
|
||||
|
||||
datSize, _, _ := s3backendStorageFile.GetStat()
|
||||
|
||||
if datSize > 0 && off >= datSize {
|
||||
@@ -154,6 +163,9 @@ func (s3backendStorageFile S3BackendStorageFile) ReadAt(p []byte, off int64) (n
|
||||
var readCount int
|
||||
for {
|
||||
p = p[readCount:]
|
||||
if len(p) == 0 {
|
||||
break
|
||||
}
|
||||
readCount, err = getObjectOutput.Body.Read(p)
|
||||
n += readCount
|
||||
|
||||
@@ -162,7 +174,7 @@ func (s3backendStorageFile S3BackendStorageFile) ReadAt(p []byte, off int64) (n
|
||||
}
|
||||
}
|
||||
|
||||
if err == io.EOF {
|
||||
if err == io.EOF && n == requested {
|
||||
err = nil
|
||||
}
|
||||
|
||||
|
||||
@@ -0,0 +1,103 @@
|
||||
package s3_backend
|
||||
|
||||
import (
|
||||
"errors"
|
||||
"io"
|
||||
"strings"
|
||||
"testing"
|
||||
"testing/iotest"
|
||||
|
||||
"github.com/aws/aws-sdk-go/service/s3"
|
||||
"github.com/aws/aws-sdk-go/service/s3/s3iface"
|
||||
"github.com/seaweedfs/seaweedfs/weed/pb/volume_server_pb"
|
||||
)
|
||||
|
||||
type stubS3Client struct {
|
||||
s3iface.S3API
|
||||
getObject func(*s3.GetObjectInput) (*s3.GetObjectOutput, error)
|
||||
}
|
||||
|
||||
func (s *stubS3Client) GetObject(input *s3.GetObjectInput) (*s3.GetObjectOutput, error) {
|
||||
return s.getObject(input)
|
||||
}
|
||||
|
||||
func TestReadAtReturnsEOFForShortRead(t *testing.T) {
|
||||
client := &stubS3Client{getObject: func(*s3.GetObjectInput) (*s3.GetObjectOutput, error) {
|
||||
return &s3.GetObjectOutput{Body: io.NopCloser(strings.NewReader("abc"))}, nil
|
||||
}}
|
||||
file := S3BackendStorageFile{
|
||||
backendStorage: &S3BackendStorage{conn: client, bucket: "bucket"},
|
||||
key: "key",
|
||||
tierInfo: &volume_server_pb.VolumeInfo{Files: []*volume_server_pb.RemoteFile{
|
||||
{FileSize: 10},
|
||||
}},
|
||||
}
|
||||
|
||||
buffer := make([]byte, 5)
|
||||
n, err := file.ReadAt(buffer, 0)
|
||||
if n != 3 {
|
||||
t.Fatalf("ReadAt() read %d bytes, want 3", n)
|
||||
}
|
||||
if !errors.Is(err, io.EOF) {
|
||||
t.Fatalf("ReadAt() error = %v, want io.EOF", err)
|
||||
}
|
||||
}
|
||||
|
||||
func TestReadAtClearsEOFOnFullRead(t *testing.T) {
|
||||
client := &stubS3Client{getObject: func(*s3.GetObjectInput) (*s3.GetObjectOutput, error) {
|
||||
return &s3.GetObjectOutput{Body: io.NopCloser(iotest.DataErrReader(strings.NewReader("abcde")))}, nil
|
||||
}}
|
||||
file := S3BackendStorageFile{
|
||||
backendStorage: &S3BackendStorage{conn: client, bucket: "bucket"},
|
||||
key: "key",
|
||||
tierInfo: &volume_server_pb.VolumeInfo{Files: []*volume_server_pb.RemoteFile{
|
||||
{FileSize: 10},
|
||||
}},
|
||||
}
|
||||
|
||||
buffer := make([]byte, 5)
|
||||
n, err := file.ReadAt(buffer, 0)
|
||||
if n != 5 || err != nil {
|
||||
t.Fatalf("ReadAt() = (%d, %v), want (5, nil)", n, err)
|
||||
}
|
||||
if string(buffer) != "abcde" {
|
||||
t.Fatalf("ReadAt() buffer = %q, want %q", buffer, "abcde")
|
||||
}
|
||||
}
|
||||
|
||||
func TestReadAtRejectsInvalidRequestsLocally(t *testing.T) {
|
||||
tests := []struct {
|
||||
name string
|
||||
buffer []byte
|
||||
offset int64
|
||||
wantErr bool
|
||||
}{
|
||||
{name: "empty buffer", buffer: nil},
|
||||
{name: "negative offset", buffer: make([]byte, 1), offset: -1, wantErr: true},
|
||||
}
|
||||
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
called := false
|
||||
client := &stubS3Client{getObject: func(*s3.GetObjectInput) (*s3.GetObjectOutput, error) {
|
||||
called = true
|
||||
return nil, errors.New("unexpected GetObject")
|
||||
}}
|
||||
file := S3BackendStorageFile{
|
||||
backendStorage: &S3BackendStorage{conn: client, bucket: "bucket"},
|
||||
key: "key",
|
||||
tierInfo: &volume_server_pb.VolumeInfo{Files: []*volume_server_pb.RemoteFile{
|
||||
{FileSize: 10},
|
||||
}},
|
||||
}
|
||||
|
||||
n, err := file.ReadAt(tt.buffer, tt.offset)
|
||||
if n != 0 || (err != nil) != tt.wantErr {
|
||||
t.Fatalf("ReadAt() = (%d, %v), want error %t", n, err, tt.wantErr)
|
||||
}
|
||||
if called {
|
||||
t.Fatal("ReadAt() called S3 for an invalid request")
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user