service/s3/s3manager: Fix index out of range when a io.Reader returns -1

[dgrr]

Fixes the S3 Upload Manager's handling of an unbounded streaming reader that returns negative bytes read.

[jasdel]

Thanks for creating this PR @dgrr. I understand to protect against a negative Read, though it would be helpful to have more background on the situtation where this error could occur. Would you be able to provide more details on how this error occurred, and the background on the usecase?

[dgrr]

Hello @jasdel

My error is happening because I am using a custom reader I made myself to read the files from S3 and process the file in between. When my custom reader detects an error returns -1 and the error. I can change that to return 0 and an error, but I think it's not the expected behavior in an io.Reader when an error happens.

[jasdel]

Thanks for the update @dgrr. In the case of an error returned by a Read call the n parameter "should" be ignored by the caller. With that said, it looks like the nextReader function's usage of the Read's returned n should be updated to only return the error and not create the buffer if there was an error.

We probably don't want to check for a value of n, but should update the code to return an err if there was an error, and otherwise return the sized buffer.

n, err := readFillBuf(r, part)
if err != nil {
    return nil, n, err
}

u.readerPos += int64(n)

return bytes.NewReader(part[0:n]), n, nil

There is no recovery when the Read method returns an error, so don't need to worry about updating u.readerPos in the case of an error.

[jasdel]

Suggest updating nextReader to check for error and return that error and n, and only creating the bytes.NewReader if there was no error.

[dgrr]

What's next? @jasdel

[jasdel]

Thanks for making these updates @dgrr. Added comment for error handling in case err is nil.

Also it would be helpful to also add a CHANGELOG_PENDING.md entry for this bug fix.

diff --git a/CHANGELOG_PENDING.md b/CHANGELOG_PENDING.md
index b1181f868..0e509847a 100644
--- a/CHANGELOG_PENDING.md
+++ b/CHANGELOG_PENDING.md
@@ -12,4 +12,5 @@
 * `aws`: Fixes bug in calculating throttled retry delay ([#373](https://github.com/aws/aws-sdk-go/pull/373))
   * The `Retry-After` duration specified in the request is now added to the Retry delay for throttled exception. Adds test for retry delays for throttled exceptions. Fixes bug where the throttled retry's math was off.
   * Fixes [#45](https://github.com/aws/aws-sdk-go/issues/45)
-
+* `service/s3/s3manager`: Fix index out of range when a streaming reader returns -1 ([#378](https://github.com/aws/aws-sdk-go-v2/pull/378))
+  * Fixes the S3 Upload Manager's handling of an unbounded streaming reader that returns negative bytes read.

[dgrr]

@jasdel updated. I also resolved the conflicts in CHANGELOG_PENDING.md

[jasdel]

Thanks for making those updates. The change looks good, we'll get this merged in.