From: Roman Khabibov <roman.habibov@tarantool.org> To: tarantool-patches@freelists.org Cc: Alexander Turenko <alexander.turenko@tarantool.org> Subject: [tarantool-patches] Re: [PATCH] httpc: fix bug with segfault by wrong headers Date: Fri, 28 Jun 2019 17:37:24 +0300 [thread overview] Message-ID: <01B8FFD0-AFA3-4821-AF68-2E17F08AF64A@tarantool.org> (raw) In-Reply-To: <20190624160423.ptng2eusy2osyyaj@tkn_work_nb> > On Jun 24, 2019, at 7:04 PM, Alexander Turenko <alexander.turenko@tarantool.org> wrote: > > On Sun, Jun 16, 2019 at 06:24:09PM +0300, Roman Khabibov wrote: >> There wasn't lua_istable() checking for field 'headers'. >> >> Closes #4281 >> --- >> >> Branch: https://github.com/tarantool/tarantool/compare/romanhabibov/gh-4281-header >> Issue: https://github.com/tarantool/tarantool/issues/4281 >> >> src/lua/httpc.c | 66 ++++++++++++++++++++----------- >> test/app-tap/http_client.test.lua | 17 ++++++++ >> 2 files changed, 61 insertions(+), 22 deletions(-) >> >> diff --git a/src/lua/httpc.c b/src/lua/httpc.c >> index bc8d498ba..bd6a7995e 100644 >> --- a/src/lua/httpc.c >> +++ b/src/lua/httpc.c >> @@ -133,6 +133,43 @@ parse_headers(lua_State *L, char *buffer, size_t len, >> lua_settable(L, -3); >> return 0; >> } >> + >> +/** >> + * Process the Lua table of headers that lies 2 below the top of > > Nit: the Lua table -> a Lua table. > > I think it is better to provide 'int idx' argument here and pass it from > a caller. > > I found the comment too common: what does 'process of headers' means? > Say, "Traverse headers in a Lua table and save them into 'struct > httpc_request'" would give much more for a reader. > > I would state somehow that this is about outgoing headers: maybe it > worth to rename it to something like 'fill_outgoing_headers'. > >> + * @a L Lua stack. In case of an error, push it on @a L. > > Nit: double whitespace. > > The last sentence is redundant for a function description title and you > anyway described retvals below. Also the error is not pushed on a Lua > stack it raises (longjump). However I propose return -1 at an error, see > below. > >> + * >> + * Added to reduce nesting. > > I would not say it here as obvious. > >> + * >> + * @param L Lua stack. >> + * @param req http request. >> + * >> + * @retval 0 on success. >> + * @retval non-zero on error. > > It raises an error in the case and does not return. However see notes > below, it seems we should really return here 0 / -1 for success / error. > > I would say -1 explicitly instead of 'non-zero’. >> + */ >> +static int >> +process_headers(struct lua_State *L, struct httpc_request *req) >> +{ >> + while (lua_next(L, -2) != 0) { >> + int header_type = lua_type(L, -1); >> + if (header_type != LUA_TSTRING) { >> + const char *err_msg = "headers must be string or table"\ >> + " with \"__tostring\""; > > It worth to fix this message, see > https://github.com/tarantool/tarantool/issues/4281#issuecomment-500477691 > Please, mention the fix in the commit message, because it is the > behaviour change and it is not easy to observe in the patch itself due > to the code move. > > I would also move the variable to the begin of the function or, maybe > better, to the caller (because of the propsed change from raising an > error to returning -1). > > A whitespace is needed before a trailing backslash. > >> + if (header_type != LUA_TTABLE) >> + return luaL_error(L, err_msg); >> + else if (!luaL_getmetafield(L, -1, "__tostring")) >> + return luaL_error(L, err_msg); >> + lua_pop(L, 1); >> + } >> + if (httpc_set_header(req, "%s: %s", lua_tostring(L, -2), >> + lua_tostring(L, -1)) < 0) { > > It was here before, but anyway: > > | While traversing a table, do not call lua_tolstring directly on a > | key, unless you know that the key is actually a string. Recall that > | lua_tolstring changes the value at the given index; this confuses the > | next call to lua_next. > > http://pgl.yoyo.org/luai/i/lua_next > > I would check that a key is a string too. This needs test cases and need > to be mentioned in the commit message. > >> + httpc_request_delete(req); + +/** + * Returns codes from fill_outgoing_headers(). + */ +enum headers_status { + HEADERS_SUCCESS = 0, + HEADERS_BAD_HEADERS = 1, + HEADERS_BAD_KEYS = 2, + HEADERS_INTERNAL_ERR = 3 +}; + + +/** + * Traverse headers in a Lua table and save them into 'struct + * httpc_request'. + * + * @param L Lua stack. + * @param req http request. + * + * @retval HEADERS_SUCCESS on success. + * @retval HEADERS_BAD_HEADERS on 'bad headers' error. + * @retval HEADERS_BAD_KEYS on 'bad keys' error. + * @retval HEADERS_INTERNAL_ERR on internal error. + */ +static enum headers_status +fill_outgoing_headers(struct lua_State *L, int idx, struct httpc_request *req) +{ + lua_pushnil(L); + while (lua_next(L, idx - 1) != 0) { + int header_type = lua_type(L, -1); + if (header_type != LUA_TSTRING) { + if (header_type != LUA_TTABLE) + return HEADERS_BAD_HEADERS; + else if (!luaL_getmetafield(L, -1, "__tostring")) + return HEADERS_BAD_HEADERS; + lua_pop(L, 1); + } + if (lua_type(L, -2) != LUA_TSTRING) + return HEADERS_BAD_KEYS; + if (httpc_set_header(req, "%s: %s", lua_tostring(L, -2), + lua_tostring(L, -1)) < 0) + return HEADERS_INTERNAL_ERR; + lua_pop(L, 1); + } + return HEADERS_SUCCESS; +} + /* }}} */ > It is better to free a resource where you acquired it: it looks more > symmetric. I mean, in luaT_httpc_request(). So I propose to return 0 / > -1 here and doing freeing in the caller. > >> + return luaT_error(L); >> + } >> + lua_pop(L, 1); >> + } >> + return 0; >> +} >> + >> /* }}} >> */ >> >> @@ -177,28 +214,13 @@ luaT_httpc_request(lua_State *L) >> lua_getfield(L, 5, "headers"); >> if (!lua_isnil(L, -1)) { >> lua_pushnil(L); > > `nil` here is the first key for the lua_next() call. Please move it to > process_headers() too. > >> - while (lua_next(L, -2) != 0) { >> - int header_type = lua_type(L, -1); >> - if (header_type != LUA_TSTRING) { >> - const char *err_msg = >> - "headers must be string or table "\ >> - "with \"__tostring\""; >> - if (header_type != LUA_TTABLE) { >> - return luaL_error(L, err_msg); >> - } else if (!luaL_getmetafield(L, -1, >> - "__tostring")) { >> - return luaL_error(L, err_msg); >> - } >> - lua_pop(L, 1); >> - } >> - if (httpc_set_header(req, "%s: %s", >> - lua_tostring(L, -2), >> - lua_tostring(L, -1)) < 0) { >> - httpc_request_delete(req); >> - return luaT_error(L); >> - } >> - lua_pop(L, 1); >> - } >> + int ret; > > We usually use name 'rc' for such variables. > >> + if (lua_istable(L, -2) != 0) >> + ret = process_headers(L, req); >> + else >> + return luaL_error(L, "field \"headers\" must be table"); >> + if (ret != 0) >> + return ret; > > I would write it so: > > | if (lua_istable(L, -2) == 0) { > | httpc_request_delete(req); > | return luaL_error(L, "<...>"); > | } > | if (process_headers(L, -2, req) != 0) { > | httpc_request_delete(req); > | return luaL_error(L, "<...>"); > | } > > In this way we place error handling code inside if's, but the main logic > outside of them. This is easier to read. > > 'return ret;' looks as the mistake. > >> } >> lua_pop(L, 1); >> >> diff --git a/test/app-tap/http_client.test.lua b/test/app-tap/http_client.test.lua >> index 680c78b35..9cb8c3a74 100755 >> --- a/test/app-tap/http_client.test.lua >> +++ b/test/app-tap/http_client.test.lua >> @@ -276,6 +276,7 @@ end >> -- gh-3679 allow only headers can be converted to string >> local function test_request_headers(test, url, opts) >> local exp_err = 'headers must be string or table with "__tostring"' >> + local exp_err_non_table = 'field "headers" must be table' > > Nit: should be a table. > >> local cases = { >> { >> 'string header', >> @@ -324,6 +325,22 @@ local function test_request_headers(test, url, opts) >> opts = {headers = {aaa = setmetatable({}, {})}}, >> exp_err = exp_err, >> }, >> +-- gh-4281 allow only field 'headers' can be table @@ -176,28 +223,22 @@ luaT_httpc_request(lua_State *L) lua_getfield(L, 5, "headers"); if (!lua_isnil(L, -1)) { - lua_pushnil(L); - while (lua_next(L, -2) != 0) { - int header_type = lua_type(L, -1); - if (header_type != LUA_TSTRING) { - const char *err_msg = - "headers must be string or table "\ - "with \"__tostring\""; - if (header_type != LUA_TTABLE) { - return luaL_error(L, err_msg); - } else if (!luaL_getmetafield(L, -1, - "__tostring")) { - return luaL_error(L, err_msg); - } - lua_pop(L, 1); - } - if (httpc_set_header(req, "%s: %s", - lua_tostring(L, -2), - lua_tostring(L, -1)) < 0) { - httpc_request_delete(req); - return luaT_error(L); - } - lua_pop(L, 1); + if (lua_istable(L, -1) == 0) { + httpc_request_delete(req); + return luaL_error(L, "\"headers\" field should be a " + "table"); + } + int rc = fill_outgoing_headers(L, -1, req); + if (rc == HEADERS_BAD_HEADERS) { + httpc_request_delete(req); + return luaL_error(L, "headers should be a string or a " + "table with a \"__tostring\""); + } else if (rc == HEADERS_BAD_KEYS) { + httpc_request_delete(req); + return luaL_error(L, "header key is non-defined"); + } else if (rc == HEADERS_INTERNAL_ERR) { + httpc_request_delete(req); + return luaT_error(L); } > Wrong indent of the comment. However I would add it to the comment of > the function, because now it states that only 'gh-3679 allow only > headers can be converted to string' cases are here, but it is not true > now. > > I would write it "'headers' field should be a table". In your wording it > says that other then 'headers' fields cannot be a table, that is the > different thing. > >> + { >> + 'non-table \'headers\'', >> + opts = {headers = 'aaa'}, >> + exp_err = exp_err_non_table, >> + }, >> + { >> + 'non-table \'headers\'', >> + opts = {headers = 1}, >> + exp_err = exp_err_non_table, >> + }, >> + { >> + 'non-table \'headers\'', >> + opts = {headers = box.NULL}, >> + exp_err = exp_err_non_table, >> + }, > > I would name the cases differently, see the cases above for examples. > >> } >> test:plan(#cases) >> >> -- >> 2.20.1 (Apple Git-117) @@ -274,8 +274,12 @@ local function test_errors(test) end -- gh-3679 allow only headers can be converted to string +-- gh-4281 allow only field 'headers' can be table and header keys +-- are defined local function test_request_headers(test, url, opts) - local exp_err = 'headers must be string or table with "__tostring"' + local exp_err = 'headers should be a string or a table with a "__tostring"' + local exp_err_non_table = '"headers" field should be a table' + local exp_err_bad_key = 'header key is non-defined' local cases = { { 'string header', @@ -324,6 +328,26 @@ local function test_request_headers(test, url, opts) opts = {headers = {aaa = setmetatable({}, {})}}, exp_err = exp_err, }, + { + 'string \'headers\'', + opts = {headers = 'aaa'}, + exp_err = exp_err_non_table, + }, + { + 'number \'headers\'', + opts = {headers = 1}, + exp_err = exp_err_non_table, + }, + { + 'cdata (box.NULL) \'headers\'', + opts = {headers = box.NULL}, + exp_err = exp_err_non_table, + }, + { + 'non-defined key', + opts = {headers = {'aaa'}}, + exp_err = exp_err_bad_key, + }, } test:plan(#cases) commit b3143a487d28b56c3965e160686a7f570d2d6e03 Author: Roman Khabibov <roman.habibov@tarantool.org> Date: Sat Jun 15 19:46:28 2019 +0300 httpc: fix bug with segfault by wrong headers There wasn't lua_istable() checking for field 'headers'. Raise an error if 'headers' field isn't a table. Also do it if a header key isn't determined. Closes #4281 diff --git a/src/lua/httpc.c b/src/lua/httpc.c index bc8d498ba..61eb61d1e 100644 --- a/src/lua/httpc.c +++ b/src/lua/httpc.c @@ -133,6 +133,53 @@ parse_headers(lua_State *L, char *buffer, size_t len, lua_settable(L, -3); return 0; } + +/** + * Returns codes from fill_outgoing_headers(). + */ +enum headers_status { + HEADERS_SUCCESS = 0, + HEADERS_BAD_HEADERS = 1, + HEADERS_BAD_KEYS = 2, + HEADERS_INTERNAL_ERR = 3 +}; + + +/** + * Traverse headers in a Lua table and save them into 'struct + * httpc_request'. + * + * @param L Lua stack. + * @param req http request. + * + * @retval HEADERS_SUCCESS on success. + * @retval HEADERS_BAD_HEADERS on 'bad headers' error. + * @retval HEADERS_BAD_KEYS on 'bad keys' error. + * @retval HEADERS_INTERNAL_ERR on internal error. + */ +static enum headers_status +fill_outgoing_headers(struct lua_State *L, int idx, struct httpc_request *req) +{ + lua_pushnil(L); + while (lua_next(L, idx - 1) != 0) { + int header_type = lua_type(L, -1); + if (header_type != LUA_TSTRING) { + if (header_type != LUA_TTABLE) + return HEADERS_BAD_HEADERS; + else if (!luaL_getmetafield(L, -1, "__tostring")) + return HEADERS_BAD_HEADERS; + lua_pop(L, 1); + } + if (lua_type(L, -2) != LUA_TSTRING) + return HEADERS_BAD_KEYS; + if (httpc_set_header(req, "%s: %s", lua_tostring(L, -2), + lua_tostring(L, -1)) < 0) + return HEADERS_INTERNAL_ERR; + lua_pop(L, 1); + } + return HEADERS_SUCCESS; +} + /* }}} */ @@ -176,28 +223,22 @@ luaT_httpc_request(lua_State *L) lua_getfield(L, 5, "headers"); if (!lua_isnil(L, -1)) { - lua_pushnil(L); - while (lua_next(L, -2) != 0) { - int header_type = lua_type(L, -1); - if (header_type != LUA_TSTRING) { - const char *err_msg = - "headers must be string or table "\ - "with \"__tostring\""; - if (header_type != LUA_TTABLE) { - return luaL_error(L, err_msg); - } else if (!luaL_getmetafield(L, -1, - "__tostring")) { - return luaL_error(L, err_msg); - } - lua_pop(L, 1); - } - if (httpc_set_header(req, "%s: %s", - lua_tostring(L, -2), - lua_tostring(L, -1)) < 0) { - httpc_request_delete(req); - return luaT_error(L); - } - lua_pop(L, 1); + if (lua_istable(L, -1) == 0) { + httpc_request_delete(req); + return luaL_error(L, "\"headers\" field should be a " + "table"); + } + int rc = fill_outgoing_headers(L, -1, req); + if (rc == HEADERS_BAD_HEADERS) { + httpc_request_delete(req); + return luaL_error(L, "headers should be a string or a " + "table with a \"__tostring\""); + } else if (rc == HEADERS_BAD_KEYS) { + httpc_request_delete(req); + return luaL_error(L, "header key is non-defined"); + } else if (rc == HEADERS_INTERNAL_ERR) { + httpc_request_delete(req); + return luaT_error(L); } } lua_pop(L, 1); diff --git a/test/app-tap/http_client.test.lua b/test/app-tap/http_client.test.lua index 680c78b35..692a504cc 100755 --- a/test/app-tap/http_client.test.lua +++ b/test/app-tap/http_client.test.lua @@ -274,8 +274,12 @@ local function test_errors(test) end -- gh-3679 allow only headers can be converted to string +-- gh-4281 allow only field 'headers' can be table and header keys +-- are defined local function test_request_headers(test, url, opts) - local exp_err = 'headers must be string or table with "__tostring"' + local exp_err = 'headers should be a string or a table with a "__tostring"' + local exp_err_non_table = '"headers" field should be a table' + local exp_err_bad_key = 'header key is non-defined' local cases = { { 'string header', @@ -324,6 +328,26 @@ local function test_request_headers(test, url, opts) opts = {headers = {aaa = setmetatable({}, {})}}, exp_err = exp_err, }, + { + 'string \'headers\'', + opts = {headers = 'aaa'}, + exp_err = exp_err_non_table, + }, + { + 'number \'headers\'', + opts = {headers = 1}, + exp_err = exp_err_non_table, + }, + { + 'cdata (box.NULL) \'headers\'', + opts = {headers = box.NULL}, + exp_err = exp_err_non_table, + }, + { + 'non-defined key', + opts = {headers = {'aaa'}}, + exp_err = exp_err_bad_key, + }, } test:plan(#cases)
next prev parent reply other threads:[~2019-06-28 14:37 UTC|newest] Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-06-16 15:24 [tarantool-patches] " Roman Khabibov [not found] ` <20190624160423.ptng2eusy2osyyaj@tkn_work_nb> 2019-06-28 14:37 ` Roman Khabibov [this message] 2019-07-22 16:12 ` [tarantool-patches] " Alexander Turenko 2019-07-25 14:35 ` Roman Khabibov 2019-07-26 15:17 ` Alexander Turenko 2019-07-23 7:48 ` Alexander Turenko 2019-07-29 10:59 ` Kirill Yukhin 2019-07-29 12:25 ` Alexander Turenko 2019-07-29 14:24 ` Kirill Yukhin
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=01B8FFD0-AFA3-4821-AF68-2E17F08AF64A@tarantool.org \ --to=roman.habibov@tarantool.org \ --cc=alexander.turenko@tarantool.org \ --cc=tarantool-patches@freelists.org \ --subject='[tarantool-patches] Re: [PATCH] httpc: fix bug with segfault by wrong headers' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox