From 0ddf29d42a1f391f3b8a221d92b80aa2b00449ca Mon Sep 17 00:00:00 2001 From: Christopher Schinnerl Date: Wed, 11 Oct 2023 17:38:11 +0200 Subject: [PATCH] Fix GetObject returning invalid object size when range is requested (#660) * worker: fix GetObject returning invalid object size when range is requested * worker: return error in case of multirange * worker: address review comment --- api/worker.go | 6 ++++++ internal/testing/cluster_test.go | 20 +++++++++++++++++++- worker/client.go | 5 +++++ worker/serve.go | 11 ++++++----- worker/worker.go | 7 ++++++- 5 files changed, 42 insertions(+), 7 deletions(-) diff --git a/api/worker.go b/api/worker.go index 41969de67..ff777ecad 100644 --- a/api/worker.go +++ b/api/worker.go @@ -252,6 +252,7 @@ func UploadWithEncryptionOffset(offset int64) UploadOption { type DownloadRange struct { Start int64 Length int64 + Size int64 } type UploadObjectResponse struct { @@ -304,8 +305,13 @@ func ParseDownloadRange(contentRange string) (DownloadRange, error) { if err != nil { return DownloadRange{}, err } + size, err := strconv.ParseInt(parts[1], 10, 64) + if err != nil { + return DownloadRange{}, err + } return DownloadRange{ Start: start, Length: end - start + 1, + Size: size, }, nil } diff --git a/internal/testing/cluster_test.go b/internal/testing/cluster_test.go index d61250f87..06316aa6b 100644 --- a/internal/testing/cluster_test.go +++ b/internal/testing/cluster_test.go @@ -1877,6 +1877,7 @@ func TestMultipartUploads(t *testing.T) { etag2 := putPart(2, len(data1), data2) etag1 := putPart(1, 0, data1) etag3 := putPart(3, len(data1)+len(data2), data3) + size := int64(len(data1) + len(data2) + len(data3)) // List parts mup, err := b.MultipartUploadParts(context.Background(), api.DefaultBucketName, objPath, mpr.UploadID, 0, 0) @@ -1914,9 +1915,26 @@ func TestMultipartUploads(t *testing.T) { // Download object gor, err := w.GetObject(context.Background(), api.DefaultBucketName, objPath) tt.OK(err) - if data, err := io.ReadAll(gor.Content); err != nil { + if gor.Range != nil { + t.Fatal("unexpected range:", gor.Range) + } else if gor.Size != size { + t.Fatal("unexpected size:", gor.Size) + } else if data, err := io.ReadAll(gor.Content); err != nil { t.Fatal(err) } else if expectedData := append(data1, append(data2, data3...)...); !bytes.Equal(data, expectedData) { t.Fatal("unexpected data:", cmp.Diff(data, expectedData)) } + + // Download a range of the object + gor, err = w.GetObject(context.Background(), api.DefaultBucketName, objPath, api.DownloadWithRange(0, 1)) + tt.OK(err) + if gor.Range == nil || gor.Range.Start != 0 || gor.Range.Length != 1 { + t.Fatal("unexpected range:", gor.Range) + } else if gor.Size != size { + t.Fatal("unexpected size:", gor.Size) + } else if data, err := io.ReadAll(gor.Content); err != nil { + t.Fatal(err) + } else if expectedData := data1[:1]; !bytes.Equal(data, expectedData) { + t.Fatal("unexpected data:", cmp.Diff(data, expectedData)) + } } diff --git a/worker/client.go b/worker/client.go index 28aece16c..ad7c06ea3 100644 --- a/worker/client.go +++ b/worker/client.go @@ -336,6 +336,11 @@ func (c *Client) GetObject(ctx context.Context, bucket, path string, opts ...api return nil, err } r = &dr + + // If a range is set, the size is the size extracted from the range + // since Content-Length will then only be the length of the returned + // range. + size = dr.Size } // Parse Last-Modified modTime, err := time.Parse(http.TimeFormat, header.Get("Last-Modified")) diff --git a/worker/serve.go b/worker/serve.go index d5cec8eb4..a7e7d75dc 100644 --- a/worker/serve.go +++ b/worker/serve.go @@ -26,6 +26,8 @@ type ( } ) +var errMultiRangeNotSupported = errors.New("multipart ranges are not supported") + func newContentReader(r io.Reader, obj api.Object, offset int64) io.ReadSeeker { return &contentReader{ r: r, @@ -92,9 +94,6 @@ func serveContent(rw http.ResponseWriter, req *http.Request, obj api.Object, dow rw.Header().Set("ETag", api.FormatETag(buildETag(req, obj.ETag))) rw.Header().Set("Content-Type", contentType) - // override the range request header to avoid seeks in http.ServeContent - req.Header.Set("Range", fmt.Sprintf("bytes=%d-%d", offset, offset+length-1)) - http.ServeContent(rw, req, obj.Name, obj.ModTime, rs) return http.StatusOK, nil } @@ -109,11 +108,13 @@ func parseRangeHeader(req *http.Request, obj api.Object) (int64, int64, error) { // extract requested offset and length offset := int64(0) length := obj.Size - if len(ranges) > 0 { + if len(ranges) == 1 { offset, length = ranges[0].Start, ranges[0].Length if offset < 0 || length < 0 || offset+length > obj.Size { - return 0, 0, fmt.Errorf("invalid range: %v %v", offset, length) + return 0, 0, fmt.Errorf("%w: %v %v", http_range.ErrInvalid, offset, length) } + } else if len(ranges) > 1 { + return 0, 0, errMultiRangeNotSupported } return offset, length, nil } diff --git a/worker/worker.go b/worker/worker.go index 748d47426..370b5c7ac 100644 --- a/worker/worker.go +++ b/worker/worker.go @@ -15,6 +15,7 @@ import ( "sync" "time" + "github.com/gotd/contrib/http_range" "go.opentelemetry.io/otel/trace" rhpv2 "go.sia.tech/core/rhp/v2" rhpv3 "go.sia.tech/core/rhp/v3" @@ -1062,7 +1063,11 @@ func (w *worker) objectsHandlerGET(jc jape.Context) { // serve the content status, err := serveContent(jc.ResponseWriter, jc.Request, *res.Object, downloadFn) - if err != nil { + if errors.Is(err, http_range.ErrInvalid) || errors.Is(err, errMultiRangeNotSupported) { + jc.Error(err, http.StatusBadRequest) + } else if errors.Is(err, http_range.ErrNoOverlap) { + jc.Error(err, http.StatusRequestedRangeNotSatisfiable) + } else if err != nil { jc.Error(err, status) } }