Fix response body is not read #72

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants
Contributor

koichik commented Jan 4, 2012

Related to joyent/node#2457.

With HTTP/1.1, if neither Content-Length nor Transfer-Encoding is present, section 4.4 of RFC 2616 suggests http-parser needs to read a response body until the connection is closed (except the response must not include a body).

e.g.

HTTP/1.1 200 OK\r\n
Content-Type: text/plain\r\n
\r\n
...

This response can include message body, it does not apply to the following descriptions:

1.Any response message which "MUST NOT" include a message-body (such
  as the 1xx, 204, and 304 responses and any response to a HEAD
  request) is always terminated by the first empty line after the
  header fields, regardless of the entity-header fields present in
  the message.

Transfer-Encoding is not present, it does not apply to the following:

2.If a Transfer-Encoding header field (section 14.41) is present and
  has any value other than "identity", then the transfer-length is
  defined by use of the "chunked" transfer-coding (section 3.6),
  unless the message is terminated by closing the connection.

Content-Length is not present, it does not apply to the following:

3.If a Content-Length header field (section 14.13) is present, its
  decimal value in OCTETs represents both the entity-length and the
  transfer-length. The Content-Length header field MUST NOT be sent
  if these two lengths are different (i.e., if a Transfer-Encoding
  header field is present). If a message is received with both a
  Transfer-Encoding header field and a Content-Length header field,
  the latter MUST be ignored.

Content-Type is not multipart/byteranges, it does not apply to the following:

4.If the message uses the media type "multipart/byteranges", and the
  ransfer-length is not otherwise specified, then this self-
  elimiting media type defines the transfer-length. This media type
  UST NOT be used unless the sender knows that the recipient can arse
  it; the presence in a request of a Range header with ultiple byte-
  range specifiers from a 1.1 client implies that the lient can parse
  multipart/byteranges responses.

Therefore, the following description is applied:

5.By the server closing the connection. (Closing the connection
  cannot be used to indicate the end of a request body, since that
  would leave no possibility for the server to send back a response.)
http_parser.c
+ parser->status_code == 304 || /* Not Modified */
+ parser->flags & F_SKIPBODY) { /* response to a HEAD request */
+ return 1;
+ }
@bnoordhuis

bnoordhuis Jan 5, 2012

Member

Trailing whitespace.

http_parser.c
}
+ if (parser->type == HTTP_RESPONSE) {
+ /* see RFC2616 seciont 4.4 */
@bnoordhuis

bnoordhuis Jan 5, 2012

Member

s/seciont/section/

http_parser.c
}
+ if (parser->type == HTTP_RESPONSE) {
@bnoordhuis

bnoordhuis Jan 5, 2012

Member

Maybe avoid the extra level of indentation:

if (parser->type != HTTP_RESPONSE) {
  return 1;
}
if (parser->status_code / 100 == 1 || /* etc */
Member

bnoordhuis commented Jan 5, 2012

Otherwise LGTM (though it probably won't hurt to have it reviewed by at least one more person).

Contributor

ry commented Jan 5, 2012

Hey @koichik great work. LGTM except we stupidly set this up have a separate CLA from node: please sign here https://raw.github.com/joyent/http-parser/2498961231853311675d6e3bcf4f5c988b15ed4e/CONTRIBUTIONS and then i'll land

Fix response body is not read
With HTTP/1.1, if neither Content-Length nor Transfer-Encoding is present,
section 4.4 of RFC 2616 suggests http-parser needs to read a response body
until the connection is closed (except the response must not include a body)

Refs nodejs/node-v0.x-archive#2457.
Fixes #72.
Contributor

koichik commented Jan 6, 2012

@bnoordhuis - Thanks for your review, updated.

@ry - Signed!

@ry ry closed this in b47c44d Jan 6, 2012

Contributor

koichik commented Jan 6, 2012

@ry - Thanks!

pgriess added a commit to pgriess/http-parser that referenced this pull request Jan 7, 2012

Get HTTP/1.1 message length logic working for HTTP/1.0
- Port message length logic from #72 to HTTP/1.0.
- Add a bunch of unit tests for handling 0-length messages.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment