* [tarantool-patches] [http.client 0/2] Fix parsing headers
@ 2018-06-15 14:24 Ilya Markov
2018-06-15 14:24 ` [tarantool-patches] [http.client 1/2] http: Remove parsed status line from headers Ilya Markov
2018-06-15 14:24 ` [tarantool-patches] [http.client 2/2] http: Fix parse long headers names Ilya Markov
0 siblings, 2 replies; 3+ messages in thread
From: Ilya Markov @ 2018-06-15 14:24 UTC (permalink / raw)
To: georgy; +Cc: tarantool-patches
Ilya Markov (2):
http: Remove parsed status line from headers
http: Fix parse long headers names
branch: gh-3451-http-client-bad-headers-parse
src/http_parser.c | 81 ++++++++++++++++++++++++++-------------
src/http_parser.h | 14 ++++---
src/lua/httpc.c | 45 ++++++++++++++--------
test/app-tap/http_client.test.lua | 9 ++++-
test/app-tap/httpd.py | 1 +
5 files changed, 102 insertions(+), 48 deletions(-)
--
2.7.4
^ permalink raw reply [flat|nested] 3+ messages in thread
* [tarantool-patches] [http.client 1/2] http: Remove parsed status line from headers
2018-06-15 14:24 [tarantool-patches] [http.client 0/2] Fix parsing headers Ilya Markov
@ 2018-06-15 14:24 ` Ilya Markov
2018-06-15 14:24 ` [tarantool-patches] [http.client 2/2] http: Fix parse long headers names Ilya Markov
1 sibling, 0 replies; 3+ messages in thread
From: Ilya Markov @ 2018-06-15 14:24 UTC (permalink / raw)
To: georgy; +Cc: tarantool-patches
Bug: Header parser validates http status line and besides saving http
status, saves valid characters to header name, which is wrong.
Fix this with skipping status line after validation without saving it as
a header.
In scope of #3451
---
src/http_parser.c | 30 +++++++++++++++++++++++++++++-
src/http_parser.h | 1 +
src/lua/httpc.c | 3 +--
test/app-tap/http_client.test.lua | 3 ++-
4 files changed, 33 insertions(+), 4 deletions(-)
diff --git a/src/http_parser.c b/src/http_parser.c
index 7166903..64f20e5 100644
--- a/src/http_parser.c
+++ b/src/http_parser.c
@@ -220,6 +220,8 @@ http_parse_header_line(struct http_parser *parser, char **bufp,
enum {
sw_start = 0,
+ skip_status_line,
+ skipped_status_line_almost_done,
sw_name,
sw_space_before_value,
sw_value,
@@ -271,6 +273,25 @@ http_parse_header_line(struct http_parser *parser, char **bufp,
break;
}
break;
+ case skip_status_line:
+ switch (ch) {
+ case LF:
+ goto skipped_status;
+ case CR:
+ state = skipped_status_line_almost_done;
+ default:
+ break;
+ }
+ break;
+ case skipped_status_line_almost_done:
+ switch (ch) {
+ case LF:
+ goto skipped_status;
+ case CR:
+ break;
+ default:
+ return HTTP_PARSE_INVALID;
+ }
/* http_header name */
case sw_name:
c = lowcase[ch];
@@ -304,8 +325,11 @@ http_parse_header_line(struct http_parser *parser, char **bufp,
if (rc == HTTP_PARSE_INVALID) {
parser->http_minor = -1;
parser->http_major = -1;
+ state = sw_start;
+ } else {
+ /* Skip it till end of line. */
+ state = skip_status_line;
}
- state = sw_start;
break;
}
if (ch == '\0')
@@ -389,6 +413,10 @@ http_parse_header_line(struct http_parser *parser, char **bufp,
}
}
+skipped_status:
+ *bufp = p + 1;
+ return HTTP_PARSE_CONTINUE;
+
done:
*bufp = p + 1;
return HTTP_PARSE_OK;
diff --git a/src/http_parser.h b/src/http_parser.h
index 5e20f53..c4d59a9 100644
--- a/src/http_parser.h
+++ b/src/http_parser.h
@@ -36,6 +36,7 @@
enum {
HTTP_PARSE_OK,
+ HTTP_PARSE_CONTINUE,
HTTP_PARSE_DONE,
HTTP_PARSE_INVALID
};
diff --git a/src/lua/httpc.c b/src/lua/httpc.c
index 45abb98..0fe27d4 100644
--- a/src/lua/httpc.c
+++ b/src/lua/httpc.c
@@ -69,13 +69,12 @@ parse_headers(lua_State *L, char *buffer, size_t len)
lua_newtable(L);
while (true) {
int rc = http_parse_header_line(&parser, &buffer, end_buf);
- if (rc == HTTP_PARSE_INVALID) {
+ if (rc == HTTP_PARSE_INVALID || rc == HTTP_PARSE_CONTINUE) {
continue;
}
if (rc == HTTP_PARSE_DONE) {
break;
}
-
if (rc == HTTP_PARSE_OK) {
lua_pushlstring(L, parser.header_name,
parser.header_name_idx);
diff --git a/test/app-tap/http_client.test.lua b/test/app-tap/http_client.test.lua
index 887395d..d9380c0 100755
--- a/test/app-tap/http_client.test.lua
+++ b/test/app-tap/http_client.test.lua
@@ -197,7 +197,7 @@ local function test_errors(test)
end
local function test_headers(test, url, opts)
- test:plan(15)
+ test:plan(16)
local http = client:new()
local r = http:get(url .. 'headers', opts)
test:is(type(r.headers["set-cookie"]), 'string', "set-cookie check")
@@ -205,6 +205,7 @@ local function test_headers(test, url, opts)
test:ok(r.headers["set-cookie"]:match("age = 17"), "set-cookie check")
test:is(r.headers["content-type"], "application/json", "content-type check")
test:is(r.headers["my_header"], "value1,value2", "other header check")
+ test:isnil(r.headers["11200ok"], "http status line not included in headers")
test:is(r.cookies["likes"][1], "cheese", "cookie value check")
test:ok(r.cookies["likes"][2][1]:match("Expires"), "cookie option check")
test:ok(r.cookies["likes"][2][3]:match("HttpOnly"), "cookie option check")
--
2.7.4
^ permalink raw reply [flat|nested] 3+ messages in thread
* [tarantool-patches] [http.client 2/2] http: Fix parse long headers names
2018-06-15 14:24 [tarantool-patches] [http.client 0/2] Fix parsing headers Ilya Markov
2018-06-15 14:24 ` [tarantool-patches] [http.client 1/2] http: Remove parsed status line from headers Ilya Markov
@ 2018-06-15 14:24 ` Ilya Markov
1 sibling, 0 replies; 3+ messages in thread
From: Ilya Markov @ 2018-06-15 14:24 UTC (permalink / raw)
To: georgy; +Cc: tarantool-patches
Bug: During parsing http headers, long headers names are truncated
to zero length, but values are not ignored.
Fix this with adding parameter max_header_name_length to http request.
If header name is bigger than this value, header name is truncated to
this length. Default value of max_header_name_length is 32.
Do some refactoring with renaming long names in http_parser.
Closes #3451
---
src/http_parser.c | 51 ++++++++++++++++++++-------------------
src/http_parser.h | 13 +++++-----
src/lua/httpc.c | 42 ++++++++++++++++++++++----------
test/app-tap/http_client.test.lua | 8 +++++-
test/app-tap/httpd.py | 1 +
5 files changed, 70 insertions(+), 45 deletions(-)
diff --git a/src/http_parser.c b/src/http_parser.c
index 64f20e5..94d7bc0 100644
--- a/src/http_parser.c
+++ b/src/http_parser.c
@@ -210,13 +210,13 @@ done:
}
int
-http_parse_header_line(struct http_parser *parser, char **bufp,
- const char *end_buf)
+http_parse_header_line(struct http_parser *prsr, char **bufp,
+ const char *end_buf, int max_hname_len)
{
char c, ch;
char *p = *bufp;
char *header_name_start = p;
- parser->header_name_idx = 0;
+ prsr->hdr_name_idx = 0;
enum {
sw_start = 0,
@@ -252,19 +252,19 @@ http_parse_header_line(struct http_parser *parser, char **bufp,
case sw_start:
switch (ch) {
case CR:
- parser->header_value_end = p;
+ prsr->hdr_value_end = p;
state = sw_header_almost_done;
break;
case LF:
- parser->header_value_end = p;
+ prsr->hdr_value_end = p;
goto header_done;
default:
state = sw_name;
c = lowcase[ch];
if (c != 0) {
- parser->header_name[0] = c;
- parser->header_name_idx = 1;
+ prsr->hdr_name[0] = c;
+ prsr->hdr_name_idx = 1;
break;
}
if (ch == '\0') {
@@ -296,9 +296,10 @@ http_parse_header_line(struct http_parser *parser, char **bufp,
case sw_name:
c = lowcase[ch];
if (c != 0) {
- parser->header_name[parser->header_name_idx] = c;
- parser->header_name_idx++;
- parser->header_name_idx &= (HEADER_LEN - 1);
+ if (prsr->hdr_name_idx < max_hname_len) {
+ prsr->hdr_name[prsr->hdr_name_idx] = c;
+ prsr->hdr_name_idx++;
+ }
break;
}
if (ch == ':') {
@@ -306,25 +307,25 @@ http_parse_header_line(struct http_parser *parser, char **bufp,
break;
}
if (ch == CR) {
- parser->header_value_start = p;
- parser->header_value_end = p;
+ prsr->hdr_value_start = p;
+ prsr->hdr_value_end = p;
state = sw_almost_done;
break;
}
if (ch == LF) {
- parser->header_value_start = p;
- parser->header_value_end = p;
+ prsr->hdr_value_start = p;
+ prsr->hdr_value_end = p;
goto done;
}
/* handle "HTTP/1.1 ..." lines */
if (ch == '/' && p - header_name_start == 4 &&
strncmp(header_name_start, "HTTP", 4) == 0) {
- int rc = http_parse_status_line(parser,
+ int rc = http_parse_status_line(prsr,
&header_name_start,
end_buf);
if (rc == HTTP_PARSE_INVALID) {
- parser->http_minor = -1;
- parser->http_major = -1;
+ prsr->http_minor = -1;
+ prsr->http_major = -1;
state = sw_start;
} else {
/* Skip it till end of line. */
@@ -341,18 +342,18 @@ http_parse_header_line(struct http_parser *parser, char **bufp,
case ' ':
break;
case CR:
- parser->header_value_start = p;
- parser->header_value_end = p;
+ prsr->hdr_value_start = p;
+ prsr->hdr_value_end = p;
state = sw_almost_done;
break;
case LF:
- parser->header_value_start = p;
- parser->header_value_end = p;
+ prsr->hdr_value_start = p;
+ prsr->hdr_value_end = p;
goto done;
case '\0':
return HTTP_PARSE_INVALID;
default:
- parser->header_value_start = p;
+ prsr->hdr_value_start = p;
state = sw_value;
break;
}
@@ -362,15 +363,15 @@ http_parse_header_line(struct http_parser *parser, char **bufp,
case sw_value:
switch (ch) {
case ' ':
- parser->header_value_end = p;
+ prsr->hdr_value_end = p;
state = sw_space_after_value;
break;
case CR:
- parser->header_value_end = p;
+ prsr->hdr_value_end = p;
state = sw_almost_done;
break;
case LF:
- parser->header_value_end = p;
+ prsr->hdr_value_end = p;
goto done;
case '\0':
return HTTP_PARSE_INVALID;
diff --git a/src/http_parser.h b/src/http_parser.h
index c4d59a9..7a37ad3 100644
--- a/src/http_parser.h
+++ b/src/http_parser.h
@@ -32,7 +32,7 @@
#ifndef TARANTOOL_HTTP_PARSER_H
#define TARANTOOL_HTTP_PARSER_H
-#define HEADER_LEN 32
+#define HEADER_NAME_LEN 32
enum {
HTTP_PARSE_OK,
@@ -42,14 +42,14 @@ enum {
};
struct http_parser {
- char *header_value_start;
- char *header_value_end;
+ char *hdr_value_start;
+ char *hdr_value_end;
int http_major;
int http_minor;
- char header_name[HEADER_LEN];
- int header_name_idx;
+ char *hdr_name;
+ int hdr_name_idx;
};
/*
@@ -62,6 +62,7 @@ struct http_parser {
* HTTP_PARSE_INVALID - error during parsing
*/
int
-http_parse_header_line(struct http_parser *parser, char **bufp, const char *end_buf);
+http_parse_header_line(struct http_parser *prsr, char **bufp,
+ const char *end_buf, int max_hname_len);
#endif //TARANTOOL_HTTP_PARSER_H
diff --git a/src/lua/httpc.c b/src/lua/httpc.c
index 0fe27d4..5f4e2e9 100644
--- a/src/lua/httpc.c
+++ b/src/lua/httpc.c
@@ -60,15 +60,23 @@ lua_add_key_u64(lua_State *L, const char *key, uint64_t value)
lua_settable(L, -3);
}
-static void
-parse_headers(lua_State *L, char *buffer, size_t len)
+static int
+parse_headers(lua_State *L, char *buffer, size_t len,
+ int max_header_name_len)
{
struct http_parser parser;
+ parser.hdr_name = (char *) calloc(max_header_name_len, sizeof(char));
+ if (parser.hdr_name == NULL) {
+ diag_set(OutOfMemory, max_header_name_len * sizeof(char),
+ "malloc", "hdr_name");
+ return -1;
+ }
char *end_buf = buffer + len;
lua_pushstring(L, "headers");
lua_newtable(L);
while (true) {
- int rc = http_parse_header_line(&parser, &buffer, end_buf);
+ int rc = http_parse_header_line(&parser, &buffer, end_buf,
+ max_header_name_len);
if (rc == HTTP_PARSE_INVALID || rc == HTTP_PARSE_CONTINUE) {
continue;
}
@@ -76,26 +84,26 @@ parse_headers(lua_State *L, char *buffer, size_t len)
break;
}
if (rc == HTTP_PARSE_OK) {
- lua_pushlstring(L, parser.header_name,
- parser.header_name_idx);
+ lua_pushlstring(L, parser.hdr_name,
+ parser.hdr_name_idx);
/* check value of header, if exists */
- lua_pushlstring(L, parser.header_name,
- parser.header_name_idx);
+ lua_pushlstring(L, parser.hdr_name,
+ parser.hdr_name_idx);
lua_gettable(L, -3);
- int value_len = parser.header_value_end -
- parser.header_value_start;
+ int value_len = parser.hdr_value_end -
+ parser.hdr_value_start;
/* table of values to handle duplicates*/
if (lua_isnil(L, -1)) {
lua_pop(L, 1);
lua_newtable(L);
lua_pushinteger(L, 1);
- lua_pushlstring(L, parser.header_value_start,
+ lua_pushlstring(L, parser.hdr_value_start,
value_len);
lua_settable(L, -3);
} else if (lua_istable(L, -1)) {
lua_pushinteger(L, lua_objlen(L, -1) + 1);
- lua_pushlstring(L, parser.header_value_start,
+ lua_pushlstring(L, parser.hdr_value_start,
value_len);
lua_settable(L, -3);
}
@@ -120,6 +128,7 @@ parse_headers(lua_State *L, char *buffer, size_t len)
/* proto */
lua_settable(L, -3);
+ return 0;
}
/* }}}
*/
@@ -142,7 +151,7 @@ luaT_httpc_request(lua_State *L)
return luaT_error(L);
double timeout = TIMEOUT_INFINITY;
-
+ int max_header_name_length = HEADER_NAME_LEN;
if (lua_isstring(L, 4)) {
size_t len = 0;
const char *body = lua_tolstring(L, 4, &len);
@@ -250,6 +259,11 @@ luaT_httpc_request(lua_State *L)
timeout = lua_tonumber(L, -1);
lua_pop(L, 1);
+ lua_getfield(L, 5, "max_header_name_length");
+ if (!lua_isnil(L, -1))
+ max_header_name_length = lua_tonumber(L, -1);
+ lua_pop(L, 1);
+
lua_getfield(L, 5, "verbose");
if (!lua_isnil(L, -1) && lua_isboolean(L, -1))
httpc_set_verbose(req, true);
@@ -278,7 +292,9 @@ luaT_httpc_request(lua_State *L)
httpc_request_delete(req);
return luaT_error(L);
}
- parse_headers(L, headers, headers_len);
+ if (parse_headers(L, headers, headers_len,
+ max_header_name_length) < 0)
+ diag_log();
}
size_t body_len = region_used(&req->resp_body);
diff --git a/test/app-tap/http_client.test.lua b/test/app-tap/http_client.test.lua
index d9380c0..a64d75f 100755
--- a/test/app-tap/http_client.test.lua
+++ b/test/app-tap/http_client.test.lua
@@ -197,7 +197,7 @@ local function test_errors(test)
end
local function test_headers(test, url, opts)
- test:plan(16)
+ test:plan(19)
local http = client:new()
local r = http:get(url .. 'headers', opts)
test:is(type(r.headers["set-cookie"]), 'string', "set-cookie check")
@@ -216,6 +216,12 @@ local function test_headers(test, url, opts)
test:ok(r.cookies["bad@name"] == nil , "cookie name check")
test:ok(r.cookies["badname"] == nil , "cookie name check")
test:ok(r.cookies["badcookie"] == nil , "cookie name check")
+ test:isnil(r.headers["very_very_very_long_headers_name1"], "no long header name")
+ test:is(r.headers["very_very_very_long_headers_name"], "true", "truncated name")
+ opts["max_header_name_length"] = 64
+ local r = http:get(url .. 'headers', opts)
+ test:is(r.headers["very_very_very_long_headers_name1"], "true", "truncated max_header_name_length")
+ opts["max_header_name_length"] = nil
end
local function test_special_methods(test, url, opts)
diff --git a/test/app-tap/httpd.py b/test/app-tap/httpd.py
index 9a09637..30172ac 100755
--- a/test/app-tap/httpd.py
+++ b/test/app-tap/httpd.py
@@ -33,6 +33,7 @@ def headers():
('Set-Cookie', 'age = 17; NOSuchOption; EmptyOption=Value;Secure'),
('my_header', 'value1'),
('my_header', 'value2'),
+ ('very_very_very_long_headers_name1', 'true'),
]
return code, body, headers
--
2.7.4
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-06-15 14:24 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-06-15 14:24 [tarantool-patches] [http.client 0/2] Fix parsing headers Ilya Markov
2018-06-15 14:24 ` [tarantool-patches] [http.client 1/2] http: Remove parsed status line from headers Ilya Markov
2018-06-15 14:24 ` [tarantool-patches] [http.client 2/2] http: Fix parse long headers names Ilya Markov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox