Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Generic method #145

Closed
wants to merge 11 commits into from

6 participants

@cmr

Should close up #92 and pave the way for joyent/node#3192. Currently depends on #144 or #106 being merged, because I wasn't paying attention and I don't know how to rebase it. Tests pass, but I'm still not confident in the patch quality. Review desired.

@bnoordhuis
Collaborator

Indent error.

@bnoordhuis
Collaborator

This can't conceivably be a valid request, can it?

No, it can't be. Back to the drawing board.

It's not because it's an invalid method, though. I need to hit the spec.

It's invalid because "world" is not a valid URI, but also because of the lack of CRLF. Given lack of HTTP-Version would make it an 0.9 or 1.0 Simple-Request, if there were a CRLF and a valid URI. How should this be handled?

Collaborator

Not sure. Gut feeling says it should fail because of the missing CRLF because you could argue that "world" is an unqualified domain name (as in CONNECT world - though that's a bad example because CONNECT requires a host:port combo.)

Maybe stick with HPE_INVALID_PATH for now and wait for the bug reports to come in? :-)

Sounds good to me, I'll poke at it later today.

@cmr

I'm having some trouble fixing this one. The parser runs out of data before it actually finds anything invalid. At http_parser.c:1819, the default behaviour seems to be to assume that the request was valid. Mind taking a peek?

@cmr

I think adding a bit to http_parser indicating whether, given the current state, the request/response is valid if parsing were to stop immediately is a good solution. Any thoughts before I code it up later today?

http_parser.c
@@ -679,13 +682,12 @@ size_t http_parser_execute (http_parser *parser,
parser->type = HTTP_RESPONSE;
parser->state = s_res_HT;
} else {
- if (ch != 'E') {
- SET_ERRNO(HPE_INVALID_CONSTANT);
- goto error;
+ if (ch == 'E') {
+ parser->method = HTTP_HEAD;
+ } else {
+ parser->type = HTTP_REQUEST;
}

I think this should always set parser->type to HTTP_REQUEST, and set parser->method to HTTP_HEAD or HTTP_GENERIC depending on ch.

@cmr
cmr added a note

Looking back the logic I wrote doesn't even make sense. "HTTP" is a valid extension method, so setting the type to RESPONSE before encountering a / doesn't make sense.

@cmr
cmr added a note

Fixed up in ff4d643

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
http_parser.c
@@ -918,7 +921,8 @@ size_t http_parser_execute (http_parser *parser,
}
matcher = method_strings[parser->method];
- if (ch == ' ' && matcher[parser->index] == '\0') {
+ if (ch == ' ' && (matcher[parser->index] == '\0' || parser->method == HTTP_GENERIC)) {
+ CALLBACK_DATA(method);
parser->state = s_req_spaces_before_url;

If parser->method is HTTP_GENERIC, you need to avoid checking matcher[parser->index] or REALLYLONGMETHODNAME will have you walking off the end of the array.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
cmr added some commits
@cmr cmr Add generic method test
Reverted a simple test to succeed even though the result is invalid, because
there are other more important things that need doing in this patch (like
getting it to actually work)
7d7852d
@cmr cmr More proper HTTP_BOTH handling ff4d643
@cmr

What should this be? Not exactly sure what the semantics of it are (it doesn't help that I'm not all that familiar with the http spec)

@cmr

To be fixed later

@cmr

What should be done here? Isn't there going to be a relatively significant performance hit having to go though the loop for detecting every supported method?

@kolbyjack

I think you need to goto reexecute_byte when switching to a request here in case ch is a space

Since it's walking through the full HTTP now, would it be better to deal with s_req_or_resp as a single state kind of like the request methods? Use a string "HTTP" and parser->index?

That would simplify the code a lot, great suggestion (thanks for all the help!)

@kolbyjack

This will still fail on GENERICLONGMETHOD, since that 'L' isn't a space, it'll fall through to where it still indexes method_strings

@cmr

Herp, I knew there was a reason I initially had a nested if.

@udp udp referenced this pull request in udp/lacewing
Open

Add support for the OPTIONS method #53

@udp

Any update on accepting this? I really need support for custom methods but I'd rather not use a fork.

@cmr

@udp The patch still doesn't quite work iirc. Want me to pick it back up?

@udp

That'd be great if you could. I don't mind doing some testing.

@cmr
@mikedeboer

@cmr to achieve full-spec CalDAV support for our project, mikedeboer/jsDAV, this'd be awesome to have!

I'm guessing more and more ppl will stumble upon this sooner or later, but you'll be my hero to start with ;)

@cmr

I'm no longer interested in working on this. Sorry!

@cmr cmr closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 24, 2012
  1. @tomika
  2. @tomika
Commits on Dec 16, 2012
  1. @cmr
  2. @cmr

    Add tests for @tomika's patch

    cmr authored
  3. @cmr
Commits on Dec 17, 2012
  1. @cmr

    Fix indentation

    cmr authored
  2. @cmr

    Tweak test (red)

    cmr authored
Commits on Dec 20, 2012
  1. @cmr
Commits on Dec 21, 2012
  1. @cmr

    Add generic method test

    cmr authored
    Reverted a simple test to succeed even though the result is invalid, because
    there are other more important things that need doing in this patch (like
    getting it to actually work)
  2. @cmr

    More proper HTTP_BOTH handling

    cmr authored
  3. @cmr
This page is out of date. Refresh to see the latest.
Showing with 107 additions and 15 deletions.
  1. +57 −11 http_parser.c
  2. +5 −0 http_parser.h
  3. +45 −4 test.c
View
68 http_parser.c
@@ -237,6 +237,9 @@ enum state
, s_start_req_or_res
, s_res_or_resp_H
+ , s_res_or_resp_HT
+ , s_res_or_resp_HTT
+ , s_res_or_resp_HTTP
, s_start_res
, s_res_H
, s_res_HT
@@ -459,6 +462,8 @@ parse_url_char(enum state s, const char ch)
return s_req_schema_slash;
}
+ return s_dead; /* handles the case of an invalid URI (no path, no colon
+ to mark the schema) */
break;
case s_req_schema_slash:
@@ -581,6 +586,7 @@ size_t http_parser_execute (http_parser *parser,
const char *header_value_mark = 0;
const char *url_mark = 0;
const char *body_mark = 0;
+ const char *method_mark = 0;
/* We're in an error state. Don't bother doing anything. */
if (HTTP_PARSER_ERRNO(parser) != HPE_OK) {
@@ -676,21 +682,50 @@ size_t http_parser_execute (http_parser *parser,
case s_res_or_resp_H:
if (ch == 'T') {
- parser->type = HTTP_RESPONSE;
- parser->state = s_res_HT;
+ parser->state = s_res_or_resp_HT;
} else {
- if (ch != 'E') {
- SET_ERRNO(HPE_INVALID_CONSTANT);
- goto error;
+ parser->type = HTTP_REQUEST;
+ if (ch == 'E') {
+ parser->method = HTTP_HEAD;
+ } else {
+ parser->method = HTTP_GENERIC;
}
- parser->type = HTTP_REQUEST;
- parser->method = HTTP_HEAD;
parser->index = 2;
parser->state = s_req_method;
}
break;
+ case s_res_or_resp_HT:
+ if (ch == 'T') {
+ parser->state = s_res_or_resp_HTT;
+ } else {
+ parser->type = HTTP_REQUEST;
+ parser->method = HTTP_GENERIC;
+ parser->state = s_req_method;
+ }
+ break;
+
+ case s_res_or_resp_HTT:
+ if (ch == 'P') {
+ parser->state = s_res_or_resp_HTTP;
+ } else {
+ parser->type = HTTP_REQUEST;
+ parser->method = HTTP_GENERIC;
+ parser->state = s_req_method;
+ }
+ break;
+
+ case s_res_or_resp_HTTP:
+ if (ch == '/') {
+ parser->state = s_res_first_http_major;
+ } else {
+ parser->type = HTTP_REQUEST;
+ parser->method = HTTP_GENERIC;
+ parser->state = s_req_method;
+ }
+ break;
+
case s_start_res:
{
parser->flags = 0;
@@ -866,6 +901,7 @@ size_t http_parser_execute (http_parser *parser,
case s_res_line_almost_done:
STRICT_CHECK(ch != LF);
parser->state = s_header_field_start;
+ CALLBACK_NOTIFY(status_complete);
break;
case s_start_req:
@@ -880,6 +916,7 @@ size_t http_parser_execute (http_parser *parser,
goto error;
}
+ MARK(method);
parser->method = (enum http_method) 0;
parser->index = 1;
switch (ch) {
@@ -899,8 +936,7 @@ size_t http_parser_execute (http_parser *parser,
case 'T': parser->method = HTTP_TRACE; break;
case 'U': parser->method = HTTP_UNLOCK; /* or UNSUBSCRIBE */ break;
default:
- SET_ERRNO(HPE_INVALID_METHOD);
- goto error;
+ parser->method = HTTP_GENERIC; break;
}
parser->state = s_req_method;
@@ -917,8 +953,19 @@ size_t http_parser_execute (http_parser *parser,
goto error;
}
+ if (parser->method == HTTP_GENERIC) {
+ if (ch == ' ') {
+ CALLBACK_DATA(method);
+ parser->state = s_req_spaces_before_url;
+ }
+ break;
+ }
+
matcher = method_strings[parser->method];
+
+ /* TODO: parse full method before deciding it isn't generic */
if (ch == ' ' && matcher[parser->index] == '\0') {
+ CALLBACK_DATA(method);
parser->state = s_req_spaces_before_url;
} else if (ch == matcher[parser->index]) {
; /* nada */
@@ -967,8 +1014,7 @@ size_t http_parser_execute (http_parser *parser,
} else if (parser->index == 4 && parser->method == HTTP_PROPFIND && ch == 'P') {
parser->method = HTTP_PROPPATCH;
} else {
- SET_ERRNO(HPE_INVALID_METHOD);
- goto error;
+ parser->method = HTTP_GENERIC;
}
++parser->index;
View
5 http_parser.h
@@ -109,6 +109,7 @@ typedef int (*http_cb) (http_parser*);
/* RFC-5789 */ \
XX(24, PATCH, PATCH) \
XX(25, PURGE, PURGE) \
+ XX(26, GENERIC, GENERIC) \
enum http_method
{
@@ -142,12 +143,14 @@ enum flags
\
/* Callback-related errors */ \
XX(CB_message_begin, "the on_message_begin callback failed") \
+ XX(CB_status_complete, "the on_status_complete callback failed") \
XX(CB_url, "the on_url callback failed") \
XX(CB_header_field, "the on_header_field callback failed") \
XX(CB_header_value, "the on_header_value callback failed") \
XX(CB_headers_complete, "the on_headers_complete callback failed") \
XX(CB_body, "the on_body callback failed") \
XX(CB_message_complete, "the on_message_complete callback failed") \
+ XX(CB_method, "the on_method callback failed") \
\
/* Parsing-related errors */ \
XX(INVALID_EOF_STATE, "stream ended at an unexpected time") \
@@ -221,7 +224,9 @@ struct http_parser {
struct http_parser_settings {
http_cb on_message_begin;
+ http_data_cb on_method;
http_data_cb on_url;
+ http_cb on_status_complete;
http_data_cb on_header_field;
http_data_cb on_header_value;
http_cb on_headers_complete;
View
49 test.c
@@ -897,6 +897,24 @@ const struct message requests[] =
,.body= ""
}
+#define GENERIC_METHOD 34
+, {.name= "use a generic extension method"
+ ,.type= HTTP_REQUEST
+ ,.raw= "LOOKOVERHERE http://example.com/ HTTP/1.1\r\n"
+ "\r\n"
+ ,.should_keep_alive= TRUE
+ ,.message_complete_on_eof= TRUE
+ ,.http_major= 1
+ ,.http_minor= 1
+ ,.method= HTTP_GENERIC
+ ,.fragment= ""
+ ,.request_path= "/"
+ ,.request_url= "http://example.com/"
+ ,.host= "example.com"
+ ,.num_headers= 0
+ ,.headers= { }
+ ,.body= ""
+ }
, {.name= NULL } /* sentinel */
};
@@ -1492,6 +1510,13 @@ request_url_cb (http_parser *p, const char *buf, size_t len)
}
int
+status_complete_cb (http_parser *p) {
+ assert(p == parser);
+ p->data++;
+ return 0;
+}
+
+int
header_field_cb (http_parser *p, const char *buf, size_t len)
{
assert(p == parser);
@@ -3089,6 +3114,20 @@ create_large_chunked_message (int body_size_in_kb, const char* headers)
return buf;
}
+void
+test_status_complete (void)
+{
+ parser_init(HTTP_RESPONSE);
+ parser->data = 0;
+ http_parser_settings settings = settings_null;
+ settings.on_status_complete = status_complete_cb;
+
+ char *response = "don't mind me, just a simple response";
+ http_parser_execute(parser, &settings, response, strlen(response));
+ assert(parser->data == (void*)0); // the status_complete callback was never called
+ assert(parser->http_errno == HPE_INVALID_CONSTANT); // the errno for an invalid status line
+}
+
/* Verify that we can pause parsing at any of the bytes in the
* message and still get the result that we're expecting. */
void
@@ -3242,13 +3281,13 @@ main (void)
/// REQUESTS
- test_simple("hello world", HPE_INVALID_METHOD);
+ test_simple("hello world", HPE_OK);
test_simple("GET / HTP/1.1\r\n\r\n", HPE_INVALID_VERSION);
- test_simple("ASDF / HTTP/1.1\r\n\r\n", HPE_INVALID_METHOD);
- test_simple("PROPPATCHA / HTTP/1.1\r\n\r\n", HPE_INVALID_METHOD);
- test_simple("GETA / HTTP/1.1\r\n\r\n", HPE_INVALID_METHOD);
+ test_simple("ASDF / HTTP/1.1\r\n\r\n", HPE_OK);
+ test_simple("PROPPATCHA / HTTP/1.1\r\n\r\n", HPE_OK);
+ test_simple("GETA / HTTP/1.1\r\n\r\n", HPE_OK);
// Well-formed but incomplete
test_simple("GET / HTTP/1.1\r\n"
@@ -3396,6 +3435,8 @@ main (void)
, &requests[CONNECT_REQUEST]
);
+ test_status_complete();
+
puts("requests okay");
return 0;
Something went wrong with that request. Please try again.