From 6cf659b0bd9e7b3354facd37d739341afbe7180f Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Mon, 22 Oct 2018 23:25:01 +0300 Subject: [PATCH 1/6] http: cleanup of the request-line parsing --- http.c | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/http.c b/http.c index 7db4d782..b6bbc699 100644 --- a/http.c +++ b/http.c @@ -1825,11 +1825,11 @@ evhttp_parse_request_line(struct evhttp_request *req, char *line) req->type = type; if (evhttp_parse_http_version(version, req) < 0) - return (-1); + return -1; if ((req->uri = mm_strdup(uri)) == NULL) { event_debug(("%s: mm_strdup", __func__)); - return (-1); + return -1; } if (type == EVHTTP_REQ_CONNECT) { @@ -1854,7 +1854,7 @@ evhttp_parse_request_line(struct evhttp_request *req, char *line) !evhttp_find_vhost(req->evcon->http_server, NULL, hostname)) req->flags |= EVHTTP_PROXY_REQUEST; - return (0); + return 0; } const char * @@ -1989,9 +1989,9 @@ evhttp_parse_firstline_(struct evhttp_request *req, struct evbuffer *buffer) char *line; enum message_read_status status = ALL_DATA_READ; - size_t line_length; + size_t len; /* XXX try */ - line = evbuffer_readln(buffer, &line_length, EVBUFFER_EOL_CRLF); + line = evbuffer_readln(buffer, &len, EVBUFFER_EOL_CRLF); if (line == NULL) { if (req->evcon != NULL && evbuffer_get_length(buffer) > req->evcon->max_headers_size) @@ -2000,13 +2000,12 @@ evhttp_parse_firstline_(struct evhttp_request *req, struct evbuffer *buffer) return (MORE_DATA_EXPECTED); } - if (req->evcon != NULL && - line_length > req->evcon->max_headers_size) { + if (req->evcon != NULL && len > req->evcon->max_headers_size) { mm_free(line); return (DATA_TOO_LONG); } - req->headers_size = line_length; + req->headers_size = len; switch (req->kind) { case EVHTTP_REQUEST: @@ -2063,12 +2062,12 @@ evhttp_parse_headers_(struct evhttp_request *req, struct evbuffer* buffer) enum message_read_status status = MORE_DATA_EXPECTED; struct evkeyvalq* headers = req->input_headers; - size_t line_length; - while ((line = evbuffer_readln(buffer, &line_length, EVBUFFER_EOL_CRLF)) + size_t len; + while ((line = evbuffer_readln(buffer, &len, EVBUFFER_EOL_CRLF)) != NULL) { char *skey, *svalue; - req->headers_size += line_length; + req->headers_size += len; if (req->evcon != NULL && req->headers_size > req->evcon->max_headers_size) { From 254fbc81b43eb8de42c7723a3f129ac529ebc9f4 Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Mon, 22 Oct 2018 23:56:19 +0300 Subject: [PATCH 2/6] http: allow trailing spaces (and only them) in request-line (like nginx) --- http.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/http.c b/http.c index b6bbc699..9a641da8 100644 --- a/http.c +++ b/http.c @@ -1686,8 +1686,9 @@ evhttp_parse_response_line(struct evhttp_request *req, char *line) /* Parse the first line of a HTTP request */ static int -evhttp_parse_request_line(struct evhttp_request *req, char *line) +evhttp_parse_request_line(struct evhttp_request *req, char *line, size_t len) { + char *eos = line + len; char *method; char *uri; char *version; @@ -1696,6 +1697,12 @@ evhttp_parse_request_line(struct evhttp_request *req, char *line) size_t method_len; enum evhttp_cmd_type type; + while (eos > line && *(eos-1) == ' ') { + *(eos-1) = '\0'; + --eos; + --len; + } + /* Parse the request line */ method = strsep(&line, " "); if (line == NULL) @@ -2009,7 +2016,7 @@ evhttp_parse_firstline_(struct evhttp_request *req, struct evbuffer *buffer) switch (req->kind) { case EVHTTP_REQUEST: - if (evhttp_parse_request_line(req, line) == -1) + if (evhttp_parse_request_line(req, line, len) == -1) status = DATA_CORRUPTED; break; case EVHTTP_RESPONSE: From 64ead341a021a42084bb224100dda19311f2ac3e Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Mon, 22 Oct 2018 23:56:50 +0300 Subject: [PATCH 3/6] http: do not try to parse request-line if we do not have enough bytes --- http.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/http.c b/http.c index 9a641da8..636ac5a3 100644 --- a/http.c +++ b/http.c @@ -1702,6 +1702,8 @@ evhttp_parse_request_line(struct evhttp_request *req, char *line, size_t len) --eos; --len; } + if (len < strlen("GET / HTTP/1.0")) + return -1; /* Parse the request line */ method = strsep(&line, " "); From b94d913d90a7390ace32dbe025d69b41eb98f755 Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Mon, 22 Oct 2018 23:52:46 +0300 Subject: [PATCH 4/6] http: allow non RFC3986 conformant during parsing request-line (http server) Reported-by: lsdyst@163.com --- http.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/http.c b/http.c index 636ac5a3..181352b6 100644 --- a/http.c +++ b/http.c @@ -1707,14 +1707,14 @@ evhttp_parse_request_line(struct evhttp_request *req, char *line, size_t len) /* Parse the request line */ method = strsep(&line, " "); - if (line == NULL) - return (-1); - uri = strsep(&line, " "); - if (line == NULL) - return (-1); - version = strsep(&line, " "); - if (line != NULL) - return (-1); + if (!line) + return -1; + uri = line; + version = strrchr(uri, ' '); + if (!version || uri == version) + return -1; + *version = '\0'; + version++; method_len = (uri - method) - 1; type = EVHTTP_REQ_UNKNOWN_; From 15bfe712a77d01f367799b0ab43d015c8e140130 Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Mon, 22 Oct 2018 23:38:42 +0300 Subject: [PATCH 5/6] http: cover various non RFC3986 conformant URIs - http/basic_trailing_space -- covers cases when there is trailing space after the request line (nginx handles this) - http/simple_nonconformant -- covers non RFC3986 conformant URIs --- test/regress_http.c | 51 ++++++++++++++++++++++++++------------------- 1 file changed, 29 insertions(+), 22 deletions(-) diff --git a/test/regress_http.c b/test/regress_http.c index 7a5d2be3..c7a79650 100644 --- a/test/regress_http.c +++ b/test/regress_http.c @@ -150,6 +150,7 @@ http_setup_gencb(ev_uint16_t *pport, struct event_base *base, int mask, /* Register a callback for certain types of requests */ evhttp_set_cb(myhttp, "/test", http_basic_cb, myhttp); + evhttp_set_cb(myhttp, "/test nonconformant", http_basic_cb, myhttp); evhttp_set_cb(myhttp, "/timeout", http_timeout_cb, myhttp); evhttp_set_cb(myhttp, "/large", http_large_cb, base); evhttp_set_cb(myhttp, "/chunked", http_chunked_cb, base); @@ -493,7 +494,7 @@ create_bev(struct event_base *base, int fd, int ssl_mask) } static void -http_basic_test_impl(void *arg, int ssl) +http_basic_test_impl(void *arg, int ssl, const char *request_line) { struct basic_test_data *data = arg; struct timeval tv; @@ -503,6 +504,7 @@ http_basic_test_impl(void *arg, int ssl) ev_uint16_t port = 0, port2 = 0; int server_flags = ssl ? HTTP_BIND_SSL : 0; struct evhttp *http = http_setup(&port, data->base, server_flags); + struct evbuffer *out; exit_base = data->base; test_ok = 0; @@ -519,13 +521,13 @@ http_basic_test_impl(void *arg, int ssl) bev = create_bev(data->base, fd, ssl); bufferevent_setcb(bev, http_readcb, http_writecb, http_errorcb, data->base); + out = bufferevent_get_output(bev); /* first half of the http request */ - http_request = - "GET /test HTTP/1.1\r\n" - "Host: some"; + evbuffer_add_printf(out, + "%s\r\n" + "Host: some", request_line); - bufferevent_write(bev, http_request, strlen(http_request)); evutil_timerclear(&tv); tv.tv_usec = 100000; event_base_once(data->base, @@ -533,7 +535,7 @@ http_basic_test_impl(void *arg, int ssl) event_base_dispatch(data->base); - tt_assert(test_ok == 3); + tt_int_op(test_ok, ==, 3); /* connect to the second port */ bufferevent_free(bev); @@ -545,18 +547,17 @@ http_basic_test_impl(void *arg, int ssl) bev = create_bev(data->base, fd, ssl); bufferevent_setcb(bev, http_readcb, http_writecb, http_errorcb, data->base); + out = bufferevent_get_output(bev); - http_request = - "GET /test HTTP/1.1\r\n" + evbuffer_add_printf(out, + "%s\r\n" "Host: somehost\r\n" "Connection: close\r\n" - "\r\n"; - - bufferevent_write(bev, http_request, strlen(http_request)); + "\r\n", request_line); event_base_dispatch(data->base); - tt_assert(test_ok == 5); + tt_int_op(test_ok, ==, 5); /* Connect to the second port again. This time, send an absolute uri. */ bufferevent_free(bev); @@ -579,14 +580,17 @@ http_basic_test_impl(void *arg, int ssl) event_base_dispatch(data->base); - tt_assert(test_ok == 7); + tt_int_op(test_ok, ==, 7); evhttp_free(http); end: if (bev) bufferevent_free(bev); } -static void http_basic_test(void *arg) { http_basic_test_impl(arg, 0); } +static void http_basic_test(void *arg)\ +{ http_basic_test_impl(arg, 0, "GET /test HTTP/1.1"); } +static void http_basic_trailing_space_test(void *arg) +{ http_basic_test_impl(arg, 0, "GET /test HTTP/1.1 "); } static void @@ -3642,7 +3646,7 @@ http_make_web_server(evutil_socket_t fd, short what, void *arg) } static void -http_simple_test_impl(void *arg, int ssl, int dirty) +http_simple_test_impl(void *arg, int ssl, int dirty, const char *uri) { struct basic_test_data *data = arg; struct evhttp_connection *evcon = NULL; @@ -3667,9 +3671,8 @@ http_simple_test_impl(void *arg, int ssl, int dirty) req = evhttp_request_new(http_request_done, (void*) BASIC_REQUEST_BODY); tt_assert(req); - if (evhttp_make_request(evcon, req, EVHTTP_REQ_GET, "/test") == -1) { + if (evhttp_make_request(evcon, req, EVHTTP_REQ_GET, uri) == -1) tt_abort_msg("Couldn't make request"); - } event_base_dispatch(data->base); tt_int_op(test_ok, ==, 1); @@ -3681,7 +3684,9 @@ http_simple_test_impl(void *arg, int ssl, int dirty) evhttp_free(http); } static void http_simple_test(void *arg) -{ http_simple_test_impl(arg, 0, 0); } +{ http_simple_test_impl(arg, 0, 0, "/test"); } +static void http_simple_nonconformant_test(void *arg) +{ http_simple_test_impl(arg, 0, 0, "/test nonconformant"); } static void http_connection_retry_test_basic(void *arg, const char *addr, struct evdns_base *dns_base, int ssl) @@ -4761,17 +4766,17 @@ http_newreqcb_test(void *arg) #ifdef EVENT__HAVE_OPENSSL static void https_basic_test(void *arg) -{ http_basic_test_impl(arg, 1); } +{ http_basic_test_impl(arg, 1, "GET /test HTTP/1.1"); } static void https_filter_basic_test(void *arg) -{ http_basic_test_impl(arg, 1 | HTTP_SSL_FILTER); } +{ http_basic_test_impl(arg, 1 | HTTP_SSL_FILTER, "GET /test HTTP/1.1"); } static void https_incomplete_test(void *arg) { http_incomplete_test_(arg, 0, 1); } static void https_incomplete_timeout_test(void *arg) { http_incomplete_test_(arg, 1, 1); } static void https_simple_test(void *arg) -{ http_simple_test_impl(arg, 1, 0); } +{ http_simple_test_impl(arg, 1, 0, "/test"); } static void https_simple_dirty_test(void *arg) -{ http_simple_test_impl(arg, 1, 1); } +{ http_simple_test_impl(arg, 1, 1, "/test"); } static void https_connection_retry_conn_address_test(void *arg) { http_connection_retry_conn_address_test_impl(arg, 1); } static void https_connection_retry_test(void *arg) @@ -4801,7 +4806,9 @@ struct testcase_t http_testcases[] = { { "parse_uri_nc", http_parse_uri_test, 0, &basic_setup, (void*)"nc" }, { "uriencode", http_uriencode_test, 0, NULL, NULL }, HTTP(basic), + HTTP(basic_trailing_space), HTTP(simple), + HTTP(simple_nonconformant), HTTP_N(cancel, cancel, BASIC), HTTP_N(cancel_by_host, cancel, BY_HOST), From 5f1b4dfa021dd79971b6e0b0f3d2cc8c2f2d9593 Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Tue, 23 Oct 2018 00:06:47 +0300 Subject: [PATCH 6/6] Fix http https_basic/https_filter_basic under valgrind (increase timeout) --- test/regress_http.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/regress_http.c b/test/regress_http.c index c7a79650..9c7732d9 100644 --- a/test/regress_http.c +++ b/test/regress_http.c @@ -530,6 +530,8 @@ http_basic_test_impl(void *arg, int ssl, const char *request_line) evutil_timerclear(&tv); tv.tv_usec = 100000; + if (ssl) + tv.tv_usec *= 10; event_base_once(data->base, -1, EV_TIMEOUT, http_complete_write, bev, &tv);