Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix range header bounds check in SetRange() #674

Merged
merged 1 commit into from
May 4, 2017
Merged

Fix range header bounds check in SetRange() #674

merged 1 commit into from
May 4, 2017

Conversation

donatello
Copy link
Member

Should fix minio/minio#4243

@harshavardhana harshavardhana self-requested a review May 4, 2017 05:50
harshavardhana
harshavardhana requested changes May 4, 2017
View reviewed changes
request-headers.go Outdated
@@ -89,17 +89,18 @@ func (c RequestHeaders) SetRange(start, end int64) error {
case start > 0 && end == 0:
// Read everything starting from offset 'start'. `bytes=N-`
c.Set("Range", fmt.Sprintf("bytes=%d-", start))
case start > 0 && end > 0 && end >= start:
case start > 0 && end >= start:
// Read everything starting at 'start' till the 'end'. `bytes=N-M`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't this still retun an error if start is 0 and end > 0 ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Made another attempt to fix it.

@donatello donatello force-pushed the range-fix branch 2 times, most recently from a956f8a to 02a9a1c Compare May 4, 2017 08:08
harshavardhana
harshavardhana reviewed May 4, 2017
View reviewed changes
request-headers.go Outdated
c.Set("Range", fmt.Sprintf("bytes=%d", end))
case start > 0 && end == 0:
// Read everything starting from offset 'start'. `bytes=N-`
c.Set("Range", fmt.Sprintf("bytes=%d-", start))
case start > 0 && end > 0 && end >= start:
case start >= 0 && end >= start:
// Read everything starting at 'start' till the 'end'. `bytes=N-M`
c.Set("Range", fmt.Sprintf("bytes=%d-%d", start, end))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you test if 0-0 indeed reads the whole object?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does not.

@donatello
Copy link
Member Author

donatello commented May 4, 2017 •
edited
Loading

There are two conflicting cases here:

  1. Read 1 byte starting at the first byte - in the current implementation this is supported. The interpretation of start==0 && end==0 is bytes=0-0 (this is correct according to RFC).
  2. Read whole file starting from byte 0 - in the current implementation this is not supported, i.e. cannot set a header as bytes=0- though RFC supports this, the function API cannot. This is the same as not setting the header at all - so the SetHeader method should not be called.

@donatello
Copy link
Member Author

donatello commented May 4, 2017 •
edited
Loading

If we implement in the opposite way, then:

start == 0 && end == 0 would generate header as bytes=0- meaning read whole file from byte 0.

However, we lose the ability to express "read 1 byte starting from byte 0". So I chose to go with the approach in the previous comment. Still a better SetHeader API could be constructed.

harshavardhana
harshavardhana requested changes May 4, 2017
View reviewed changes
request-headers.go
c.Set("Range", fmt.Sprintf("bytes=%d-", start))
case start > 0 && end > 0 && end >= start:
// Read everything starting at 'start' till the 'end'. `bytes=N-M`
case 0 <= start && start <= end:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good, can you add some tests for this as well? @donatello

@harshavardhana
Copy link
Member

If we implement in the opposite way, then:

start == 0 && end == 0 would generate header as bytes=0- meaning read whole file from byte 0.

However, we lose the ability to express "read 1 byte starting from byte 0". So I chose to go with the approach in the previous comment. Still a better SetHeader API could be constructed.

This is good serves the purpose here.

@deekoder deekoder requested a review from nl5887 May 4, 2017 17:16
nl5887
nl5887 approved these changes May 4, 2017
View reviewed changes
Copy link
Contributor

@nl5887 nl5887 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just tested with Mint, all green again. If this PR has been merged, I'll bump Minio server with this version.

@donatello
Copy link
Member Author

@harshavardhana Tests added.

harshavardhana
harshavardhana reviewed May 4, 2017
View reviewed changes
request-headers_test.go
@@ -0,0 +1,40 @@
package minio
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add license header..

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

harshavardhana
harshavardhana previously approved these changes May 4, 2017
View reviewed changes
@harshavardhana harshavardhana merged commit 75a218a into minio:master May 4, 2017
@donatello donatello deleted the range-fix branch May 4, 2017 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nl5887 nl5887 nl5887 approved these changes

@harshavardhana harshavardhana harshavardhana approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

S3 gateway fails to get object
3 participants
@donatello @harshavardhana @nl5887