code review 163820043: net/http: Fix Range off-by-one error

net/http: Fix Range off-by-one error

Given a file of size N, a request for "Range: bytes=N-*" should
return a 416 [1].  Currently, it returns a 206 and a body of 0
bytes, with the illegal Content-Range of "bytes N-(N-1)/N" [2].

[1]: RFC 2616, sec 14.35.1: "If a syntactically valid
byte-range-set includes at least one byte- range-spec whose
first-byte-pos is less than the current length of the
entity-body, or at least one suffix-byte-range-spec with a non-
zero suffix-length, then the byte-range-set is satisfiable.
Otherwise, the byte-range-set is unsatisfiable. If the
byte-range-set is unsatisfiable, the server SHOULD return a
response with a status of 416 (Requested range not
satisfiable)."

[2]: RFC 2616, sec 14.16: "A byte-content-range-spec with a
byte-range-resp-spec whose last- byte-pos value is less than its
first-byte-pos value, or whose instance-length value is less
than or equal to its last-byte-pos value, is invalid. The
recipient of an invalid byte-content-range- spec MUST ignore it
and any content transferred along with it."

Fixes issue 8988.

[jeddenlea, 2014-10-23 19:45:14]

Hello golang-codereviews@googlegroups.com (cc:
golang-codereviews@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go

[bradfitz, 2014-10-23 21:41:11]

https://codereview.appspot.com/163820043/diff/40001/src/net/http/fs_test.go
File src/net/http/fs_test.go (left):

https://codereview.appspot.com/163820043/diff/40001/src/net/http/fs_test.go#o...
src/net/http/fs_test.go:53: {r: "bytes=20-", code:
StatusRequestedRangeNotSatisfiable},
please redo this file so the diff is minimal with no movement, and don't remove
any test cases.

If an existing test case is wrong, I want to easily see the new value in the
diff.

[jeddenlea, 2014-10-23 22:01:40]

On 2014/10/23 21:41:11, bradfitz wrote:
> https://codereview.appspot.com/163820043/diff/40001/src/net/http/fs_test.go
> File src/net/http/fs_test.go (left):
> 
>
https://codereview.appspot.com/163820043/diff/40001/src/net/http/fs_test.go#o...
> src/net/http/fs_test.go:53: {r: "bytes=20-", code:
> StatusRequestedRangeNotSatisfiable},
> please redo this file so the diff is minimal with no movement, and don't
remove
> any test cases.
> 
> If an existing test case is wrong, I want to easily see the new value in the
> diff.

Thanks for jumping on this so quickly.

It's not that that old case was wrong, but it was the only case which attempted
to exercise asking for an out-of-bounds range.  I've added a few, and used
values I feel more clearly expose the intent of the test (N, N+1, N+more). 
Because I added a number of tests which now test out-of-bounds ranges, the old
one is redundant, and I think it's more clean for all the 416 cases to be
grouped together.

Further, the use of itoa(testFileLen) in the other cases feels untenable.  If
the value of testFileLen changed, it would likely break a bunch of other test
cases here.  And `"bytes=" + itoa(testFileLen) + "-" itoa(testFileLen+1)` sure
looks ugly (as would Sprintf, or even a helper function) and obscures the intent
of the test. It seemed odd to leave the old ones inconsistent with the new ones.
 But, I can change them back if you'd prefer.

What do you think?

[bradfitz, 2014-10-23 22:04:31]

For each change, clean or fix. But not both. It obscures fixes.

You can do two changes if you want.

We all have limited review time lately and small changes are easier to
review.

I can look at this again later tonight and try to page this part of http
and this code back into my brain but I'd rather not think too much if the
diff and change description makes it obvious.

Also cite the part of the http spec that justifies the change.
 On Oct 23, 2014 3:01 PM, <jed@fastly.com> wrote:

> On 2014/10/23 21:41:11, bradfitz wrote:
>
> https://codereview.appspot.com/163820043/diff/40001/src/
> net/http/fs_test.go
>
>> File src/net/http/fs_test.go (left):
>>
>
>
> https://codereview.appspot.com/163820043/diff/40001/src/
> net/http/fs_test.go#oldcode53
>
>> src/net/http/fs_test.go:53: {r: "bytes=20-", code:
>> StatusRequestedRangeNotSatisfiable},
>> please redo this file so the diff is minimal with no movement, and
>>
> don't remove
>
>> any test cases.
>>
>
>  If an existing test case is wrong, I want to easily see the new value
>>
> in the
>
>> diff.
>>
>
> Thanks for jumping on this so quickly.
>
> It's not that that old case was wrong, but it was the only case which
> attempted to exercise asking for an out-of-bounds range.  I've added a
> few, and used values I feel more clearly expose the intent of the test
> (N, N+1, N+more).  Because I added a number of tests which now test
> out-of-bounds ranges, the old one is redundant, and I think it's more
> clean for all the 416 cases to be grouped together.
>
> Further, the use of itoa(testFileLen) in the other cases feels
> untenable.  If the value of testFileLen changed, it would likely break a
> bunch of other test cases here.  And `"bytes=" + itoa(testFileLen) + "-"
> itoa(testFileLen+1)` sure looks ugly (as would Sprintf, or even a helper
> function) and obscures the intent of the test. It seemed odd to leave
> the old ones inconsistent with the new ones.  But, I can change them
> back if you'd prefer.
>
> What do you think?
>
> https://codereview.appspot.com/163820043/
>

[bradfitz, 2014-10-23 22:22:55]

https://codereview.appspot.com/163820043/diff/40001/src/net/http/fs_test.go
File src/net/http/fs_test.go (left):

https://codereview.appspot.com/163820043/diff/40001/src/net/http/fs_test.go#o...
src/net/http/fs_test.go:53: {r: "bytes=20-", code:
StatusRequestedRangeNotSatisfiable},
Okay, I've actually read these tests now and feel comfortable with this change.
You can keep this as-is.

Just update the CL description to reference the sentence & section of which RFC
describes the behavior we had wrong.

[bradfitz, 2014-10-23 22:24:17]

Have you guys submitted a corporate CLA for Fastly?


On Thu, Oct 23, 2014 at 3:22 PM, <bradfitz@golang.org> wrote:

>
> https://codereview.appspot.com/163820043/diff/40001/src/
> net/http/fs_test.go
> File src/net/http/fs_test.go (left):
>
> https://codereview.appspot.com/163820043/diff/40001/src/
> net/http/fs_test.go#oldcode53
> src/net/http/fs_test.go:53: {r: "bytes=20-", code:
> StatusRequestedRangeNotSatisfiable},
> Okay, I've actually read these tests now and feel comfortable with this
> change. You can keep this as-is.
>
> Just update the CL description to reference the sentence & section of
> which RFC describes the behavior we had wrong.
>
> https://codereview.appspot.com/163820043/
>

[jeddenlea, 2014-10-23 22:28:43]

On 2014/10/23 22:24:17, bradfitz wrote:
> Have you guys submitted a corporate CLA for Fastly?

Adam was working on that with our general counsel at some point, I've asked them
to get that rolling again.  We're operating under the guidance that it's
probably not necessary for a small change like this.  Do you know otherwise?

> 
> 
> On Thu, Oct 23, 2014 at 3:22 PM, <mailto:bradfitz@golang.org> wrote:
> 
> >
> > https://codereview.appspot.com/163820043/diff/40001/src/
> > net/http/fs_test.go
> > File src/net/http/fs_test.go (left):
> >
> > https://codereview.appspot.com/163820043/diff/40001/src/
> > net/http/fs_test.go#oldcode53
> > src/net/http/fs_test.go:53: {r: "bytes=20-", code:
> > StatusRequestedRangeNotSatisfiable},
> > Okay, I've actually read these tests now and feel comfortable with this
> > change. You can keep this as-is.
> >
> > Just update the CL description to reference the sentence & section of
> > which RFC describes the behavior we had wrong.
> >
> > https://codereview.appspot.com/163820043/
> >

[bradfitz, 2014-10-23 22:32:54]

Size of patch doesn't matter I don't think.
 On Oct 23, 2014 3:28 PM, <jed@fastly.com> wrote:

> On 2014/10/23 22:24:17, bradfitz wrote:
>
>> Have you guys submitted a corporate CLA for Fastly?
>>
>
> Adam was working on that with our general counsel at some point, I've
> asked them to get that rolling again.  We're operating under the
> guidance that it's probably not necessary for a small change like this.
> Do you know otherwise?
>
>
>
>  On Thu, Oct 23, 2014 at 3:22 PM, <mailto:bradfitz@golang.org> wrote:
>>
>
>  >
>> > https://codereview.appspot.com/163820043/diff/40001/src/
>> > net/http/fs_test.go
>> > File src/net/http/fs_test.go (left):
>> >
>> > https://codereview.appspot.com/163820043/diff/40001/src/
>> > net/http/fs_test.go#oldcode53
>> > src/net/http/fs_test.go:53: {r: "bytes=20-", code:
>> > StatusRequestedRangeNotSatisfiable},
>> > Okay, I've actually read these tests now and feel comfortable with
>>
> this
>
>> > change. You can keep this as-is.
>> >
>> > Just update the CL description to reference the sentence & section
>>
> of
>
>> > which RFC describes the behavior we had wrong.
>> >
>> > https://codereview.appspot.com/163820043/
>> >
>>
>
>
>
> https://codereview.appspot.com/163820043/
>

[jeddenlea, 2014-10-23 22:51:26]

Ok, I'll try to expedite that if it's necessary for you to accept this.

I appreciate the time constraints of the Go team, and understand such rules on
CLs.  I'd gone ahead and made a green-only version of this right before I saw
your reply, but I've gone ahead and switched back to the first version I
proposed.  In the future, I'll try to be more surgical in my changes, more will
be coming :)

I've updated the desc as well with the relevant parts of the HTTP spec.

Thanks again for your time.

On 2014/10/23 22:32:54, bradfitz wrote:
> Size of patch doesn't matter I don't think.
>  On Oct 23, 2014 3:28 PM, <mailto:jed@fastly.com> wrote:
> 
> > On 2014/10/23 22:24:17, bradfitz wrote:
> >
> >> Have you guys submitted a corporate CLA for Fastly?
> >>
> >
> > Adam was working on that with our general counsel at some point, I've
> > asked them to get that rolling again.  We're operating under the
> > guidance that it's probably not necessary for a small change like this.
> > Do you know otherwise?
> >
> >
> >
> >  On Thu, Oct 23, 2014 at 3:22 PM, <mailto:bradfitz@golang.org> wrote:
> >>
> >
> >  >
> >> > https://codereview.appspot.com/163820043/diff/40001/src/
> >> > net/http/fs_test.go
> >> > File src/net/http/fs_test.go (left):
> >> >
> >> > https://codereview.appspot.com/163820043/diff/40001/src/
> >> > net/http/fs_test.go#oldcode53
> >> > src/net/http/fs_test.go:53: {r: "bytes=20-", code:
> >> > StatusRequestedRangeNotSatisfiable},
> >> > Okay, I've actually read these tests now and feel comfortable with
> >>
> > this
> >
> >> > change. You can keep this as-is.
> >> >
> >> > Just update the CL description to reference the sentence & section
> >>
> > of
> >
> >> > which RFC describes the behavior we had wrong.
> >> >
> >> > https://codereview.appspot.com/163820043/
> >> >
> >>
> >
> >
> >
> > https://codereview.appspot.com/163820043/
> >

[jeddenlea, 2014-10-29 21:58:00]

Hey Brad,

We finally submitted the agreement for Fastly.  Max Sills can confirm.

Thanks!

-Jed

On 2014/10/23 22:51:26, jeddenlea wrote:
> Ok, I'll try to expedite that if it's necessary for you to accept this.
> 
> I appreciate the time constraints of the Go team, and understand such rules on
> CLs.  I'd gone ahead and made a green-only version of this right before I saw
> your reply, but I've gone ahead and switched back to the first version I
> proposed.  In the future, I'll try to be more surgical in my changes, more
will
> be coming :)
> 
> I've updated the desc as well with the relevant parts of the HTTP spec.
> 
> Thanks again for your time.
> 
> On 2014/10/23 22:32:54, bradfitz wrote:
> > Size of patch doesn't matter I don't think.
> >  On Oct 23, 2014 3:28 PM, <mailto:jed@fastly.com> wrote:
> > 
> > > On 2014/10/23 22:24:17, bradfitz wrote:
> > >
> > >> Have you guys submitted a corporate CLA for Fastly?
> > >>
> > >
> > > Adam was working on that with our general counsel at some point, I've
> > > asked them to get that rolling again.  We're operating under the
> > > guidance that it's probably not necessary for a small change like this.
> > > Do you know otherwise?
> > >
> > >
> > >
> > >  On Thu, Oct 23, 2014 at 3:22 PM, <mailto:bradfitz@golang.org> wrote:
> > >>
> > >
> > >  >
> > >> > https://codereview.appspot.com/163820043/diff/40001/src/
> > >> > net/http/fs_test.go
> > >> > File src/net/http/fs_test.go (left):
> > >> >
> > >> > https://codereview.appspot.com/163820043/diff/40001/src/
> > >> > net/http/fs_test.go#oldcode53
> > >> > src/net/http/fs_test.go:53: {r: "bytes=20-", code:
> > >> > StatusRequestedRangeNotSatisfiable},
> > >> > Okay, I've actually read these tests now and feel comfortable with
> > >>
> > > this
> > >
> > >> > change. You can keep this as-is.
> > >> >
> > >> > Just update the CL description to reference the sentence & section
> > >>
> > > of
> > >
> > >> > which RFC describes the behavior we had wrong.
> > >> >
> > >> > https://codereview.appspot.com/163820043/
> > >> >
> > >>
> > >
> > >
> > >
> > > https://codereview.appspot.com/163820043/
> > >

[bradfitz, 2014-12-16 03:00:16]

Could you try submitting this again with the updated git + Gerrit
contribution instructions?  I'm curious whether Gerrit's new automatic CLA
checking is working for corporate CLAs.


On Thu, Oct 30, 2014 at 8:58 AM, <jed@fastly.com> wrote:
>
> Hey Brad,
>
> We finally submitted the agreement for Fastly.  Max Sills can confirm.
>
> Thanks!
>
> -Jed
>
>
> On 2014/10/23 22:51:26, jeddenlea wrote:
>
>> Ok, I'll try to expedite that if it's necessary for you to accept
>>
> this.
>
>  I appreciate the time constraints of the Go team, and understand such
>>
> rules on
>
>> CLs.  I'd gone ahead and made a green-only version of this right
>>
> before I saw
>
>> your reply, but I've gone ahead and switched back to the first version
>>
> I
>
>> proposed.  In the future, I'll try to be more surgical in my changes,
>>
> more will
>
>> be coming :)
>>
>
>  I've updated the desc as well with the relevant parts of the HTTP
>>
> spec.
>
>  Thanks again for your time.
>>
>
>  On 2014/10/23 22:32:54, bradfitz wrote:
>> > Size of patch doesn't matter I don't think.
>> >  On Oct 23, 2014 3:28 PM, <mailto:jed@fastly.com> wrote:
>> >
>> > > On 2014/10/23 22:24:17, bradfitz wrote:
>> > >
>> > >> Have you guys submitted a corporate CLA for Fastly?
>> > >>
>> > >
>> > > Adam was working on that with our general counsel at some point,
>>
> I've
>
>> > > asked them to get that rolling again.  We're operating under the
>> > > guidance that it's probably not necessary for a small change like
>>
> this.
>
>> > > Do you know otherwise?
>> > >
>> > >
>> > >
>> > >  On Thu, Oct 23, 2014 at 3:22 PM, <mailto:bradfitz@golang.org>
>>
> wrote:
>
>> > >>
>> > >
>> > >  >
>> > >> > https://codereview.appspot.com/163820043/diff/40001/src/
>> > >> > net/http/fs_test.go
>> > >> > File src/net/http/fs_test.go (left):
>> > >> >
>> > >> > https://codereview.appspot.com/163820043/diff/40001/src/
>> > >> > net/http/fs_test.go#oldcode53
>> > >> > src/net/http/fs_test.go:53: {r: "bytes=20-", code:
>> > >> > StatusRequestedRangeNotSatisfiable},
>> > >> > Okay, I've actually read these tests now and feel comfortable
>>
> with
>
>> > >>
>> > > this
>> > >
>> > >> > change. You can keep this as-is.
>> > >> >
>> > >> > Just update the CL description to reference the sentence &
>>
> section
>
>> > >>
>> > > of
>> > >
>> > >> > which RFC describes the behavior we had wrong.
>> > >> >
>> > >> > https://codereview.appspot.com/163820043/
>> > >> >
>> > >>
>> > >
>> > >
>> > >
>> > > https://codereview.appspot.com/163820043/
>> > >
>>
>
>
> https://codereview.appspot.com/163820043/
>

[jeddenlea, 2014-12-16 08:51:02]

Doesn't seem to be working, at least not for fastly.com.  I worked through
the updated contribution docs, finally got to `git review mail`, only to
get:

fatal: remote error:  A Contributor Agreement must be completed before
uploading.
git push -q origin HEAD:refs/for/master
git-review: exit status 128

I have to say, I'm pretty impressed by the migration to git overall!
Pretty amazing, really.

-Jed


On Dec 15, 2014 7:00 PM, "Brad Fitzpatrick" <bradfitz@golang.org> wrote:

> Could you try submitting this again with the updated git + Gerrit
> contribution instructions?  I'm curious whether Gerrit's new automatic CLA
> checking is working for corporate CLAs.
>
>
> On Thu, Oct 30, 2014 at 8:58 AM, <jed@fastly.com> wrote:
>>
>> Hey Brad,
>>
>> We finally submitted the agreement for Fastly.  Max Sills can confirm.
>>
>> Thanks!
>>
>> -Jed
>>
>>
>> On 2014/10/23 22:51:26, jeddenlea wrote:
>>
>>> Ok, I'll try to expedite that if it's necessary for you to accept
>>>
>> this.
>>
>>  I appreciate the time constraints of the Go team, and understand such
>>>
>> rules on
>>
>>> CLs.  I'd gone ahead and made a green-only version of this right
>>>
>> before I saw
>>
>>> your reply, but I've gone ahead and switched back to the first version
>>>
>> I
>>
>>> proposed.  In the future, I'll try to be more surgical in my changes,
>>>
>> more will
>>
>>> be coming :)
>>>
>>
>>  I've updated the desc as well with the relevant parts of the HTTP
>>>
>> spec.
>>
>>  Thanks again for your time.
>>>
>>
>>  On 2014/10/23 22:32:54, bradfitz wrote:
>>> > Size of patch doesn't matter I don't think.
>>> >  On Oct 23, 2014 3:28 PM, <mailto:jed@fastly.com> wrote:
>>> >
>>> > > On 2014/10/23 22:24:17, bradfitz wrote:
>>> > >
>>> > >> Have you guys submitted a corporate CLA for Fastly?
>>> > >>
>>> > >
>>> > > Adam was working on that with our general counsel at some point,
>>>
>> I've
>>
>>> > > asked them to get that rolling again.  We're operating under the
>>> > > guidance that it's probably not necessary for a small change like
>>>
>> this.
>>
>>> > > Do you know otherwise?
>>> > >
>>> > >
>>> > >
>>> > >  On Thu, Oct 23, 2014 at 3:22 PM, <mailto:bradfitz@golang.org>
>>>
>> wrote:
>>
>>> > >>
>>> > >
>>> > >  >
>>> > >> > https://codereview.appspot.com/163820043/diff/40001/src/
>>> > >> > net/http/fs_test.go
>>> > >> > File src/net/http/fs_test.go (left):
>>> > >> >
>>> > >> > https://codereview.appspot.com/163820043/diff/40001/src/
>>> > >> > net/http/fs_test.go#oldcode53
>>> > >> > src/net/http/fs_test.go:53: {r: "bytes=20-", code:
>>> > >> > StatusRequestedRangeNotSatisfiable},
>>> > >> > Okay, I've actually read these tests now and feel comfortable
>>>
>> with
>>
>>> > >>
>>> > > this
>>> > >
>>> > >> > change. You can keep this as-is.
>>> > >> >
>>> > >> > Just update the CL description to reference the sentence &
>>>
>> section
>>
>>> > >>
>>> > > of
>>> > >
>>> > >> > which RFC describes the behavior we had wrong.
>>> > >> >
>>> > >> > https://codereview.appspot.com/163820043/
>>> > >> >
>>> > >>
>>> > >
>>> > >
>>> > >
>>> > > https://codereview.appspot.com/163820043/
>>> > >
>>>
>>
>>
>> https://codereview.appspot.com/163820043/
>>
>

[gobot, 2014-12-19 05:14:58]

R=close

To the author of this CL:

The Go project has moved to Gerrit Code Review.

If this CL should be continued, please see the latest version of
https://golang.org/doc/contribute.html for instructions on
how to set up Git and the Go project's Gerrit codereview plugin,
and then create a new change with your current code.

If there has been discussion on this CL, please give a link to it
(golang.org/cl/163820043 is best) in the description in your
new CL.

Thanks very much.