From 0b546ddfd429ef2b7d46edd7c9aa263fdb843c67 Mon Sep 17 00:00:00 2001 From: Harshavardhana Date: Fri, 6 Oct 2017 09:38:01 -0700 Subject: [PATCH] Return errors in PutObject()/PutObjectPart() if input size is -1. (#5015) Amazon S3 API expects all incoming stream has a content-length set it was superflous for us to support object layer which supports unknown sized stream as well, this PR removes such requirements and explicitly error out if input stream is less than zero. --- cmd/fs-v1-multipart.go | 5 +++++ cmd/fs-v1.go | 5 +++++ cmd/xl-v1-multipart.go | 6 ++++++ cmd/xl-v1-object.go | 23 ++++++++++++++++------- cmd/xl-v1-utils.go | 40 +++++++++++++++++++++------------------- cmd/xl-v1-utils_test.go | 11 ++++++----- 6 files changed, 59 insertions(+), 31 deletions(-) diff --git a/cmd/fs-v1-multipart.go b/cmd/fs-v1-multipart.go index 427f12ef6..f3476ccda 100644 --- a/cmd/fs-v1-multipart.go +++ b/cmd/fs-v1-multipart.go @@ -482,6 +482,11 @@ func (fs fsObjects) PutObjectPart(bucket, object, uploadID string, partID int, d return pi, toObjectErr(err, bucket) } + // Validate input data size and it can never be less than zero. + if data.Size() < 0 { + return pi, toObjectErr(traceError(errInvalidArgument)) + } + // Hold the lock so that two parallel complete-multipart-uploads // do not leave a stale uploads.json behind. objectMPartPathLock := globalNSMutex.NewNSLock(minioMetaMultipartBucket, pathJoin(bucket, object)) diff --git a/cmd/fs-v1.go b/cmd/fs-v1.go index a672791c1..11e4d8775 100644 --- a/cmd/fs-v1.go +++ b/cmd/fs-v1.go @@ -537,6 +537,11 @@ func (fs fsObjects) PutObject(bucket string, object string, data *HashReader, me return ObjectInfo{}, toObjectErr(traceError(errFileAccessDenied), bucket, object) } + // Validate input data size and it can never be less than zero. + if data.Size() < 0 { + return ObjectInfo{}, traceError(errInvalidArgument) + } + // No metadata is set, allocate a new one. if metadata == nil { metadata = make(map[string]string) diff --git a/cmd/xl-v1-multipart.go b/cmd/xl-v1-multipart.go index 4163096af..f392b177e 100644 --- a/cmd/xl-v1-multipart.go +++ b/cmd/xl-v1-multipart.go @@ -577,6 +577,11 @@ func (xl xlObjects) PutObjectPart(bucket, object, uploadID string, partID int, d return pi, err } + // Validate input data size and it can never be less than zero. + if data.Size() < 0 { + return pi, toObjectErr(traceError(errInvalidArgument)) + } + var partsMetadata []xlMetaV1 var errs []error uploadIDPath := pathJoin(bucket, object, uploadID) @@ -586,6 +591,7 @@ func (xl xlObjects) PutObjectPart(bucket, object, uploadID string, partID int, d if err := preUploadIDLock.GetRLock(globalOperationTimeout); err != nil { return pi, err } + // Validates if upload ID exists. if !xl.isUploadIDExists(bucket, object, uploadID) { preUploadIDLock.RUnlock() diff --git a/cmd/xl-v1-object.go b/cmd/xl-v1-object.go index 1dd954671..1408a4fa1 100644 --- a/cmd/xl-v1-object.go +++ b/cmd/xl-v1-object.go @@ -443,6 +443,11 @@ func (xl xlObjects) PutObject(bucket string, object string, data *HashReader, me return ObjectInfo{}, err } + // Validate input data size and it can never be less than zero. + if data.Size() < 0 { + return ObjectInfo{}, toObjectErr(traceError(errInvalidArgument)) + } + // Check if an object is present as one of the parent dir. // -- FIXME. (needs a new kind of lock). // -- FIXME (this also causes performance issue when disks are down). @@ -504,7 +509,10 @@ func (xl xlObjects) PutObject(bucket string, object string, data *HashReader, me if err != nil { return ObjectInfo{}, toObjectErr(err, bucket, object) } - buffer := make([]byte, partsMetadata[0].Erasure.BlockSize, 2*partsMetadata[0].Erasure.BlockSize) // alloc additional space for parity blocks created while erasure coding + + // Alloc additional space for parity blocks created while erasure codinga + buffer := make([]byte, partsMetadata[0].Erasure.BlockSize, 2*partsMetadata[0].Erasure.BlockSize) + // Read data and split into parts - similar to multipart mechanism for partIdx := 1; ; partIdx++ { // Compute part name @@ -512,10 +520,10 @@ func (xl xlObjects) PutObject(bucket string, object string, data *HashReader, me // Compute the path of current part tempErasureObj := pathJoin(uniqueID, partName) - // Calculate the size of the current part, if size is unknown, curPartSize wil be unknown too. - // allowEmptyPart will always be true if this is the first part and false otherwise. + // Calculate the size of the current part. AllowEmptyPart will always be true + // if this is the first part and false otherwise. var curPartSize int64 - curPartSize, err = getPartSizeFromIdx(data.Size(), globalPutPartSize, partIdx) + curPartSize, err = calculatePartSizeFromIdx(data.Size(), globalPutPartSize, partIdx) if err != nil { return ObjectInfo{}, toObjectErr(err, bucket, object) } @@ -529,7 +537,8 @@ func (xl xlObjects) PutObject(bucket string, object string, data *HashReader, me } } - file, erasureErr := storage.CreateFile(io.LimitReader(reader, globalPutPartSize), minioMetaTmpBucket, tempErasureObj, buffer, DefaultBitrotAlgorithm, xl.writeQuorum) + file, erasureErr := storage.CreateFile(io.LimitReader(reader, curPartSize), minioMetaTmpBucket, + tempErasureObj, buffer, DefaultBitrotAlgorithm, xl.writeQuorum) if erasureErr != nil { return ObjectInfo{}, toObjectErr(erasureErr, minioMetaTmpBucket, tempErasureObj) } @@ -561,7 +570,7 @@ func (xl xlObjects) PutObject(bucket string, object string, data *HashReader, me // Check part size for the next index. var partSize int64 - partSize, err = getPartSizeFromIdx(data.Size(), globalPutPartSize, partIdx+1) + partSize, err = calculatePartSizeFromIdx(data.Size(), globalPutPartSize, partIdx+1) if err != nil { return ObjectInfo{}, toObjectErr(err, bucket, object) } @@ -570,7 +579,7 @@ func (xl xlObjects) PutObject(bucket string, object string, data *HashReader, me } } - if size := data.Size(); size > 0 && sizeWritten < data.Size() { + if sizeWritten < data.Size() { return ObjectInfo{}, traceError(IncompleteBody{}) } diff --git a/cmd/xl-v1-utils.go b/cmd/xl-v1-utils.go index 087574c9a..b1f6dd674 100644 --- a/cmd/xl-v1-utils.go +++ b/cmd/xl-v1-utils.go @@ -382,35 +382,37 @@ func evalDisks(disks []StorageAPI, errs []error) []StorageAPI { return newDisks } -// Errors specifically generated by getPartSizeFromIdx function. +// Errors specifically generated by calculatePartSizeFromIdx function. var ( errPartSizeZero = errors.New("Part size cannot be zero") errPartSizeIndex = errors.New("Part index cannot be smaller than 1") ) -// getPartSizeFromIdx predicts the part size according to its index. It also -// returns -1 when totalSize is -1. -func getPartSizeFromIdx(totalSize int64, partSize int64, partIndex int) (int64, error) { +// calculatePartSizeFromIdx calculates the part size according to input index. +// returns error if totalSize is -1, partSize is 0, partIndex is 0. +func calculatePartSizeFromIdx(totalSize int64, partSize int64, partIndex int) (currPartSize int64, err error) { + if totalSize < 0 { + return 0, traceError(errInvalidArgument) + } if partSize == 0 { return 0, traceError(errPartSizeZero) } if partIndex < 1 { return 0, traceError(errPartSizeIndex) } - switch totalSize { - case -1, 0: - return totalSize, nil - } - // Compute the total count of parts - partsCount := totalSize/partSize + 1 - // Return the part's size - switch { - case int64(partIndex) < partsCount: - return partSize, nil - case int64(partIndex) == partsCount: - // Size of last part - return totalSize % partSize, nil - default: - return 0, nil + if totalSize > 0 { + // Compute the total count of parts + partsCount := totalSize/partSize + 1 + // Return the part's size + switch { + case int64(partIndex) < partsCount: + currPartSize = partSize + case int64(partIndex) == partsCount: + // Size of last part + currPartSize = totalSize % partSize + default: + currPartSize = 0 + } } + return currPartSize, nil } diff --git a/cmd/xl-v1-utils_test.go b/cmd/xl-v1-utils_test.go index de6bdb947..bc160eea1 100644 --- a/cmd/xl-v1-utils_test.go +++ b/cmd/xl-v1-utils_test.go @@ -340,8 +340,6 @@ func TestGetPartSizeFromIdx(t *testing.T) { partIndex int expectedSize int64 }{ - // Total size is - 1 - {-1, 10, 1, -1}, // Total size is zero {0, 10, 1, 0}, // part size 2MiB, total size 4MiB @@ -356,7 +354,7 @@ func TestGetPartSizeFromIdx(t *testing.T) { } for i, testCase := range testCases { - s, err := getPartSizeFromIdx(testCase.totalSize, testCase.partSize, testCase.partIndex) + s, err := calculatePartSizeFromIdx(testCase.totalSize, testCase.partSize, testCase.partIndex) if err != nil { t.Errorf("Test %d: Expected to pass but failed. %s", i+1, err) } @@ -371,13 +369,16 @@ func TestGetPartSizeFromIdx(t *testing.T) { partIndex int err error }{ - // partSize is 0, error. + // partSize is 0, returns error. {10, 0, 1, errPartSizeZero}, + // partIndex is 0, returns error. {10, 1, 0, errPartSizeIndex}, + // Total size is -1, returns error. + {-1, 10, 1, errInvalidArgument}, } for i, testCaseFailure := range testCasesFailure { - _, err := getPartSizeFromIdx(testCaseFailure.totalSize, testCaseFailure.partSize, testCaseFailure.partIndex) + _, err := calculatePartSizeFromIdx(testCaseFailure.totalSize, testCaseFailure.partSize, testCaseFailure.partIndex) if err == nil { t.Errorf("Test %d: Expected to failed but passed. %s", i+1, err) }