Fix 0-length range responses. #87

Merged
merged 2 commits into from Aug 1, 2013

Conversation

Projects
None yet
2 participants
@pmundkur
Contributor

pmundkur commented Nov 6, 2012

There were two issues:

  • file:pread() returns eof in the case when the length of the
    read is 0 bytes, for any offset. This causes badarg exceptions
    later in iolist_size when the 'eof' atom is encountered instead
    of a binary

  • The range-length computation is off by 1 for 0-length ranges:
    {Skip, Skip + Length - 1, PartialBody} would result in e.g.
    {0, -1, eof}. {0, -1} is invalid HTTP according to

    http://tools.ietf.org/html/rfc2616#section-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.

This patch fixes both issues.

Fix 0-length range responses.
There were two issues:

- file:pread() returns eof in the case when the length of the
  read is 0 bytes, for any offset.  This causes badarg exceptions
  later in iolist_size when the 'eof' atom is encountered instead
  of a binary

- The range-length computation is off by 1 for 0-length ranges:
  {Skip, Skip + Length - 1, PartialBody} would result in e.g.
  {0, -1, eof}. {0, -1} is invalid HTTP according to

  http://tools.ietf.org/html/rfc2616#section-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.

This patch fixes both issues.
@etrepum

This comment has been minimized.

Show comment Hide comment
@etrepum

etrepum Nov 6, 2012

Member

It'd be really great if there were tests that showed the issue and ensured that this doesn't regress

Member

etrepum commented Nov 6, 2012

It'd be really great if there were tests that showed the issue and ensured that this doesn't regress

@pmundkur

This comment has been minimized.

Show comment Hide comment
@pmundkur

pmundkur Nov 6, 2012

Contributor

I didn't find any existing tests in mochiweb_{http, request} that could be adapted for the purpose. Where should I look?

Contributor

pmundkur commented Nov 6, 2012

I didn't find any existing tests in mochiweb_{http, request} that could be adapted for the purpose. Where should I look?

@etrepum

This comment has been minimized.

Show comment Hide comment
@etrepum

etrepum Nov 16, 2012

Member

Well, it doesn't appear that there's an obvious test in this repo that you can just copy and modify. The way that I've done this kind of test in the past is to just have the test start up a server listening on a random port on loopback, send a request to that port with the right headers, and inspect the response for correctness. A bit heavyweight but it does verify the functionality and cover all of the code in that path.

Member

etrepum commented Nov 16, 2012

Well, it doesn't appear that there's an obvious test in this repo that you can just copy and modify. The way that I've done this kind of test in the past is to just have the test start up a server listening on a random port on loopback, send a request to that port with the right headers, and inspect the response for correctness. A bit heavyweight but it does verify the functionality and cover all of the code in that path.

@pmundkur

This comment has been minimized.

Show comment Hide comment
@pmundkur

pmundkur Nov 22, 2012

Contributor

The problem with such tests is that they tend to be sensitive to timing (see for e.g. the tests I supplied for pull #44). You suggested there to use meck instead; that is probably a better approach, but I won't be able to get to it anytime soon.

Contributor

pmundkur commented Nov 22, 2012

The problem with such tests is that they tend to be sensitive to timing (see for e.g. the tests I supplied for pull #44). You suggested there to use meck instead; that is probably a better approach, but I won't be able to get to it anytime soon.

@etrepum

This comment has been minimized.

Show comment Hide comment
@etrepum

etrepum Nov 22, 2012

Member

I don't see why this kind of test would be sensitive to timing. Start a
server, connect to said server and send a request, read the full response
and verify the assumptions.

On Wed, Nov 21, 2012 at 11:14 PM, Prashanth Mundkur <
notifications@github.com> wrote:

The problem with such tests is that they tend to be sensitive to timing
(see for e.g. the tests I supplied for pull #44#44).
You suggested there to use meck instead; that is probably a better
approach, but I won't be able to get to it anytime soon.


Reply to this email directly or view it on GitHubhttps://github.com/mochi/mochiweb/pull/87#issuecomment-10625543.

Member

etrepum commented Nov 22, 2012

I don't see why this kind of test would be sensitive to timing. Start a
server, connect to said server and send a request, read the full response
and verify the assumptions.

On Wed, Nov 21, 2012 at 11:14 PM, Prashanth Mundkur <
notifications@github.com> wrote:

The problem with such tests is that they tend to be sensitive to timing
(see for e.g. the tests I supplied for pull #44#44).
You suggested there to use meck instead; that is probably a better
approach, but I won't be able to get to it anytime soon.


Reply to this email directly or view it on GitHubhttps://github.com/mochi/mochiweb/pull/87#issuecomment-10625543.

etrepum added a commit that referenced this pull request Aug 1, 2013

@etrepum etrepum merged commit 510766b into mochi:master Aug 1, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment