From a51488cbaaa24a639530f199b2317e717f88ccba Mon Sep 17 00:00:00 2001 From: Anis Elleuch Date: Tue, 8 Dec 2020 22:12:42 +0100 Subject: [PATCH] s3: Fix reading GET with partNumber specified (#11032) partNumber was miscalculting the start and end of parts when partNumber query is specified in the GET request. This commit fixes it and also fixes the ContentRange header in that case. --- cmd/api-headers.go | 20 ++---- cmd/object-api-utils.go | 22 +++++-- cmd/object-handlers_test.go | 125 ++++++++++++++++++++++++++++++++++++ 3 files changed, 148 insertions(+), 19 deletions(-) diff --git a/cmd/api-headers.go b/cmd/api-headers.go index 7b72b739b..c578459ca 100644 --- a/cmd/api-headers.go +++ b/cmd/api-headers.go @@ -157,19 +157,13 @@ func setObjectHeaders(w http.ResponseWriter, objInfo ObjectInfo, rs *HTTPRangeSp } if opts.PartNumber > 0 { - var start, end int64 - for i := 0; i < len(objInfo.Parts) && i < opts.PartNumber; i++ { - start = end - end = start + objInfo.Parts[i].ActualSize - 1 - } - rs = &HTTPRangeSpec{Start: start, End: end} - rangeLen = end - start + 1 - } else { - // for providing ranged content - start, rangeLen, err = rs.GetOffsetLength(totalObjectSize) - if err != nil { - return err - } + rs = partNumberToRangeSpec(objInfo, opts.PartNumber) + } + + // For providing ranged content + start, rangeLen, err = rs.GetOffsetLength(totalObjectSize) + if err != nil { + return err } // Set content length. diff --git a/cmd/object-api-utils.go b/cmd/object-api-utils.go index e892221cd..2b0d2a5ed 100644 --- a/cmd/object-api-utils.go +++ b/cmd/object-api-utils.go @@ -502,6 +502,21 @@ func getPartFile(entriesTrie *trie.Trie, partNumber int, etag string) (partFile return partFile } +func partNumberToRangeSpec(oi ObjectInfo, partNumber int) *HTTPRangeSpec { + if oi.Size == 0 || len(oi.Parts) == 0 { + return nil + } + + var start int64 + var end = int64(-1) + for i := 0; i < len(oi.Parts) && i < partNumber; i++ { + start = end + 1 + end = start + oi.Parts[i].ActualSize - 1 + } + + return &HTTPRangeSpec{Start: start, End: end} +} + // Returns the compressed offset which should be skipped. func getCompressedOffsets(objectInfo ObjectInfo, offset int64) (int64, int64) { var compressedOffset int64 @@ -571,12 +586,7 @@ func NewGetObjectReader(rs *HTTPRangeSpec, oi ObjectInfo, opts ObjectOptions, cl fn ObjReaderFn, off, length int64, err error) { if rs == nil && opts.PartNumber > 0 { - var start, end int64 - for i := 0; i < len(oi.Parts) && i < opts.PartNumber; i++ { - start = end - end = start + oi.Parts[i].ActualSize - 1 - } - rs = &HTTPRangeSpec{Start: start, End: end} + rs = partNumberToRangeSpec(oi, opts.PartNumber) } // Call the clean-up functions immediately in case of exit diff --git a/cmd/object-handlers_test.go b/cmd/object-handlers_test.go index 69ce0b758..d0c6b597b 100644 --- a/cmd/object-handlers_test.go +++ b/cmd/object-handlers_test.go @@ -844,6 +844,131 @@ func testAPIGetObjectWithMPHandler(obj ObjectLayer, instanceType, bucketName str } +// Wrapper for calling GetObject API handler tests for both Erasure multiple disks and FS single drive setup. +func TestAPIGetObjectWithPartNumberHandler(t *testing.T) { + globalPolicySys = NewPolicySys() + defer func() { globalPolicySys = nil }() + + defer DetectTestLeak(t)() + ExecObjectLayerAPITest(t, testAPIGetObjectWithPartNumberHandler, []string{"NewMultipart", "PutObjectPart", "CompleteMultipart", "GetObject", "PutObject"}) +} + +func testAPIGetObjectWithPartNumberHandler(obj ObjectLayer, instanceType, bucketName string, apiRouter http.Handler, + credentials auth.Credentials, t *testing.T) { + + // Set SSL to on to do encryption tests + globalIsSSL = true + defer func() { globalIsSSL = false }() + + var ( + oneMiB int64 = 1024 * 1024 + key32Bytes = generateBytesData(32 * humanize.Byte) + key32BytesMd5 = md5.Sum(key32Bytes) + metaWithSSEC = map[string]string{ + crypto.SSECAlgorithm: crypto.SSEAlgorithmAES256, + crypto.SSECKey: base64.StdEncoding.EncodeToString(key32Bytes), + crypto.SSECKeyMD5: base64.StdEncoding.EncodeToString(key32BytesMd5[:]), + } + mapCopy = func(m map[string]string) map[string]string { + r := make(map[string]string, len(m)) + for k, v := range m { + r[k] = v + } + return r + } + ) + + type ObjectInput struct { + objectName string + partLengths []int64 + + metaData map[string]string + } + + // set of inputs for uploading the objects before tests for + // downloading is done. Data bytes are from DummyDataGen. + objectInputs := []ObjectInput{ + // // cases 0-4: small single part objects + {"nothing", []int64{0}, make(map[string]string)}, + {"1byte", []int64{1}, make(map[string]string)}, + {"small-0", []int64{11}, make(map[string]string)}, + {"small-1", []int64{509}, make(map[string]string)}, + {"small-2", []int64{5 * oneMiB}, make(map[string]string)}, + // // // cases 5-8: multipart part objects + {"mp-0", []int64{5 * oneMiB, 1}, make(map[string]string)}, + {"mp-1", []int64{5*oneMiB + 1, 1}, make(map[string]string)}, + {"mp-2", []int64{5487701, 5487799, 3}, make(map[string]string)}, + {"mp-3", []int64{10499807, 10499963, 7}, make(map[string]string)}, + // cases 9-12: small single part objects with encryption + {"enc-nothing", []int64{0}, mapCopy(metaWithSSEC)}, + {"enc-small-0", []int64{11}, mapCopy(metaWithSSEC)}, + {"enc-small-1", []int64{509}, mapCopy(metaWithSSEC)}, + {"enc-small-2", []int64{5 * oneMiB}, mapCopy(metaWithSSEC)}, + // cases 13-16: multipart part objects with encryption + {"enc-mp-0", []int64{5 * oneMiB, 1}, mapCopy(metaWithSSEC)}, + {"enc-mp-1", []int64{5*oneMiB + 1, 1}, mapCopy(metaWithSSEC)}, + {"enc-mp-2", []int64{5487701, 5487799, 3}, mapCopy(metaWithSSEC)}, + {"enc-mp-3", []int64{10499807, 10499963, 7}, mapCopy(metaWithSSEC)}, + } + + // iterate through the above set of inputs and upload the object. + for _, input := range objectInputs { + uploadTestObject(t, apiRouter, credentials, bucketName, input.objectName, input.partLengths, input.metaData, false) + } + + mkGetReqWithPartNumber := func(oindex int, oi ObjectInput, partNumber int) { + object := oi.objectName + rec := httptest.NewRecorder() + + queries := url.Values{} + queries.Add("partNumber", strconv.Itoa(partNumber)) + targetURL := makeTestTargetURL("", bucketName, object, queries) + req, err := newTestSignedRequestV4(http.MethodGet, targetURL, + 0, nil, credentials.AccessKey, credentials.SecretKey, oi.metaData) + if err != nil { + t.Fatalf("Object: %s Object Index %d PartNumber: %d: Failed to create HTTP request for Get Object: %v", + object, oindex, partNumber, err) + } + + apiRouter.ServeHTTP(rec, req) + + // Check response code (we make only valid requests in this test) + if rec.Code != http.StatusPartialContent && rec.Code != http.StatusOK { + bd, err1 := ioutil.ReadAll(rec.Body) + t.Fatalf("%s Object: %s ObjectIndex %d PartNumber: %d: Got response status `%d` and body: %s,%v", + instanceType, object, oindex, partNumber, rec.Code, string(bd), err1) + } + + oinfo, err := obj.GetObjectInfo(context.Background(), bucketName, object, ObjectOptions{}) + if err != nil { + t.Fatalf("Object: %s Object Index %d: Unexpected err: %v", object, oindex, err) + } + + rs := partNumberToRangeSpec(oinfo, partNumber) + off, length, err := rs.GetOffsetLength(oinfo.Size) + if err != nil { + t.Fatalf("Object: %s Object Index %d: Unexpected err: %v", object, oindex, err) + } + + readers := []io.Reader{} + cumulativeSum := int64(0) + for _, p := range oi.partLengths { + readers = append(readers, NewDummyDataGen(p, cumulativeSum)) + cumulativeSum += p + } + refReader := io.LimitReader(ioutilx.NewSkipReader(io.MultiReader(readers...), off), length) + if ok, msg := cmpReaders(refReader, rec.Body); !ok { + t.Fatalf("(%s) Object: %s ObjectIndex %d PartNumber: %d --> data mismatch! (msg: %s)", instanceType, oi.objectName, oindex, partNumber, msg) + } + } + + for idx, oi := range objectInputs { + for partNum := 1; partNum <= len(oi.partLengths); partNum++ { + mkGetReqWithPartNumber(idx, oi, partNum) + } + } +} + // Wrapper for calling PutObject API handler tests using streaming signature v4 for both Erasure multiple disks and FS single drive setup. func TestAPIPutObjectStreamSigV4Handler(t *testing.T) { defer DetectTestLeak(t)()