Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Fix response body is not read #72

Closed
wants to merge 1 commit into from

3 participants

@koichik

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
((6 lines not shown))
}
+ if (parser->type == HTTP_RESPONSE) {
+ /* see RFC2616 seciont 4.4 */
+ if (parser->status_code / 100 == 1 || /* 1xx e.g. Continue */
+ parser->status_code == 204 || /* No Content */
+ parser->status_code == 304 || /* Not Modified */
+ parser->flags & F_SKIPBODY) { /* response to a HEAD request */
+ return 1;
+ }

Trailing whitespace.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
http_parser.c
((6 lines not shown))
}
+ if (parser->type == HTTP_RESPONSE) {
+ /* see RFC2616 seciont 4.4 */

s/seciont/section/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
http_parser.c
((6 lines not shown))
}
+ if (parser->type == HTTP_RESPONSE) {

Maybe avoid the extra level of indentation:

if (parser->type != HTTP_RESPONSE) {
  return 1;
}
if (parser->status_code / 100 == 1 || /* etc */
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@bnoordhuis

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

@ry
ry commented

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

@koichik koichik 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 joyent/node#2457.
Fixes #72.
c6d3bf3
@koichik

@bnoordhuis - Thanks for your review, updated.

@ry - Signed!

@ry ry closed this pull request from a commit
@koichik koichik 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)

See also joyent/node#2457.
Fixes #72
b47c44d
@ry ry closed this in b47c44d
@koichik

@ry - Thanks!

@pgriess pgriess referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
@pgriess pgriess referenced this pull request from a commit in pgriess/http-parser
@pgriess pgriess 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.
248fbc3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jan 6, 2012
  1. @koichik

    Fix response body is not read

    koichik authored
    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 joyent/node#2457.
    Fixes #72.
This page is out of date. Refresh to see the latest.
Showing with 57 additions and 7 deletions.
  1. +13 −1 http_parser.c
  2. +44 −6 test.c
View
14 http_parser.c
@@ -1733,9 +1733,21 @@ http_should_keep_alive (http_parser *parser)
/* HTTP/1.1 */
if (parser->flags & F_CONNECTION_CLOSE) {
return 0;
- } else {
+ }
+ if (parser->type == HTTP_REQUEST) {
+ return 1;
+ }
+ /* see RFC2616 section 4.4 */
+ if (parser->status_code / 100 == 1 || /* 1xx e.g. Continue */
+ parser->status_code == 204 || /* No Content */
+ parser->status_code == 304 || /* Not Modified */
+ parser->flags & F_SKIPBODY) { /* response to a HEAD request */
return 1;
}
+ if (!(parser->flags & F_CHUNKED) && parser->content_length == -1) {
+ return 0;
+ }
+ return 1;
} else {
/* HTTP/1.0 or earlier */
if (parser->flags & F_CONNECTION_KEEP_ALIVE) {
View
50 test.c
@@ -780,8 +780,8 @@ const struct message responses[] =
, {.name= "404 no headers no body"
,.type= HTTP_RESPONSE
,.raw= "HTTP/1.1 404 Not Found\r\n\r\n"
- ,.should_keep_alive= TRUE
- ,.message_complete_on_eof= FALSE
+ ,.should_keep_alive= FALSE
+ ,.message_complete_on_eof= TRUE
,.http_major= 1
,.http_minor= 1
,.status_code= 404
@@ -795,8 +795,8 @@ const struct message responses[] =
, {.name= "301 no response phrase"
,.type= HTTP_RESPONSE
,.raw= "HTTP/1.1 301\r\n\r\n"
- ,.should_keep_alive = TRUE
- ,.message_complete_on_eof= FALSE
+ ,.should_keep_alive = FALSE
+ ,.message_complete_on_eof= TRUE
,.http_major= 1
,.http_minor= 1
,.status_code= 301
@@ -1057,8 +1057,46 @@ const struct message responses[] =
{}
,.body= ""
}
-, {.name= NULL } /* sentinel */
+#define NO_CONTENT_LENGTH_NO_TRANSFER_ENCODING_RESPONSE 13
+/* The client should wait for the server's EOF. That is, when neither
+ * content-length nor transfer-encoding is specified, the end of body
+ * is specified by the EOF.
+ */
+, {.name= "neither content-length nor transfer-encoding response"
+ ,.type= HTTP_RESPONSE
+ ,.raw= "HTTP/1.1 200 OK\r\n"
+ "Content-Type: text/plain\r\n"
+ "\r\n"
+ "hello world"
+ ,.should_keep_alive= FALSE
+ ,.message_complete_on_eof= TRUE
+ ,.http_major= 1
+ ,.http_minor= 1
+ ,.status_code= 200
+ ,.num_headers= 1
+ ,.headers=
+ { { "Content-Type", "text/plain" }
+ }
+ ,.body= "hello world"
+ }
+
+#define NO_HEADERS_NO_BODY_204 14
+, {.name= "204 no headers no body"
+ ,.type= HTTP_RESPONSE
+ ,.raw= "HTTP/1.1 204 No Content\r\n\r\n"
+ ,.should_keep_alive= TRUE
+ ,.message_complete_on_eof= FALSE
+ ,.http_major= 1
+ ,.http_minor= 1
+ ,.status_code= 204
+ ,.num_headers= 0
+ ,.headers= {}
+ ,.body_size= 0
+ ,.body= ""
+ }
+
+, {.name= NULL } /* sentinel */
};
int
@@ -1888,7 +1926,7 @@ main (void)
printf("response scan 1/2 ");
test_scan( &responses[TRAILING_SPACE_ON_CHUNKED_BODY]
- , &responses[NO_HEADERS_NO_BODY_404]
+ , &responses[NO_HEADERS_NO_BODY_204]
, &responses[NO_REASON_PHRASE]
);
Something went wrong with that request. Please try again.