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: Thu, 25 Jul 2019 17:35:22 +0300 [thread overview] Message-ID: <8C721538-80AA-4926-8158-586B6907D6F2@tarantool.org> (raw) In-Reply-To: <20190722161216.xzrs3wvjzqo3u37h@tkn_work_nb> > On Jul 22, 2019, at 19:12, Alexander Turenko <alexander.turenko@tarantool.org> wrote: > >>> 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. > > As I see it is not done. httpc: verify "headers" option type There wasn't lua_istable() checking for field 'headers'. Raise an error if 'headers' field isn't a table. Add checking that header key is string, if it isn't then raise an error. Also clarify error message added in #3679. Closes #4281 > >>>> + 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. > > Now I see, a number is not possible here, so the comment is not actual. > However I observer that __tostring is not called here and reopened > #3679. > >> +/** >> + * Returns codes from fill_outgoing_headers(). >> + */ >> +enum headers_status { >> + HEADERS_SUCCESS = 0, >> + HEADERS_BAD_HEADERS = 1, >> + HEADERS_BAD_KEYS = 2, >> + HEADERS_INTERNAL_ERR = 3 >> +}; > > Set a diag in a function and return -1 at an error. Using enum here > looks as overkill. @@ -176,13 +176,17 @@ luaT_httpc_request(lua_State *L) lua_getfield(L, 5, "headers"); if (!lua_isnil(L, -1)) { + if (lua_istable(L, -1) == 0) { + httpc_request_delete(req); + return luaL_error(L, "opts.headers should be a table"); + } 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\""; + "opts.headers values should be strings " + "or tables with \"__tostring\""; if (header_type != LUA_TTABLE) { return luaL_error(L, err_msg); } else if (!luaL_getmetafield(L, -1, @@ -191,6 +195,10 @@ luaT_httpc_request(lua_State *L) } lua_pop(L, 1); } + if (lua_type(L, -2) != LUA_TSTRING) + return luaL_error(L, + "opts.headers keys should be " + "strings"); if (httpc_set_header(req, "%s: %s", lua_tostring(L, -2), lua_tostring(L, -1)) < 0) { >> -- 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' > > Please, work on wording of comments and error messages. They are very > confusing in many ways: > > * 'allow only headers can be converted to string' -- does you mean the > following? > >> 'headers' option should be a table with fields that are strings or >> convertible to strings > > * What is 'defined' and 'non-defined'? I don't know any common meaning > that would be applicable here. > >> 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 --- gh-3679 allow only headers can be converted to string +-- gh-3679 Check that opts.headers values can be strings or table +-- convertible to string only. +-- gh-4281 Check that opts.headers can be a table and opts.headers +-- keys can be strings only. local function test_request_headers(test, url, opts) - local exp_err = 'headers must be string or table with "__tostring"' + local exp_err_val = 'opts.headers values should be strings or tables with'.. + ' \"__tostring\"' + local exp_err_non_table = 'opts.headers should be a table' + local exp_err_bad_key = 'opts.headers keys should be strings' local cases = { { 'string header', @@ -297,32 +303,52 @@ local function test_request_headers(test, url, opts) { 'boolean header', opts = {headers = {aaa = true}}, - exp_err = exp_err, + exp_err = exp_err_val, }, { 'number header', opts = {headers = {aaa = 10}}, - exp_err = exp_err, + exp_err = exp_err_val, }, { 'cdata header (box.NULL)', opts = {headers = {aaa = box.NULL}}, - exp_err = exp_err, + exp_err = exp_err_val, }, { 'cdata<uint64_t> header', opts = {headers = {aaa = 10ULL}}, - exp_err = exp_err, + exp_err = exp_err_val, }, { 'table header w/o metatable', opts = {headers = {aaa = {}}}, - exp_err = exp_err, + exp_err = exp_err_val, }, { 'table header w/o __tostring() metamethod', opts = {headers = {aaa = setmetatable({}, {})}}, - exp_err = exp_err, + exp_err = exp_err_val, + }, + { + '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, + }, + { + 'number key', + opts = {headers = {'aaa'}}, + exp_err = exp_err_bad_key, }, } test:plan(#cases) > I would say it in more certain way, say, 'httpc: verify "headers" option > type'. > >> 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. > > How a key in a Lua table can be determined or not determined? I don't > know such term in this context. commit 2a839a6c952f145c2979426de507ae4b88b44bf3 Author: Roman Khabibov <roman.habibov@tarantool.org> Date: Sat Jun 15 19:46:28 2019 +0300 httpc: verify "headers" option type There wasn't lua_istable() checking for field 'headers'. Raise an error if 'headers' field isn't a table. Add checking that header key is string, if it isn't then raise an error. Also clarify error message added in #3679. Closes #4281 diff --git a/src/lua/httpc.c b/src/lua/httpc.c index bc8d498ba..ffc9f8a0b 100644 --- a/src/lua/httpc.c +++ b/src/lua/httpc.c @@ -176,13 +176,17 @@ luaT_httpc_request(lua_State *L) lua_getfield(L, 5, "headers"); if (!lua_isnil(L, -1)) { + if (lua_istable(L, -1) == 0) { + httpc_request_delete(req); + return luaL_error(L, "opts.headers should be a table"); + } 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\""; + "opts.headers values should be strings " + "or tables with \"__tostring\""; if (header_type != LUA_TTABLE) { return luaL_error(L, err_msg); } else if (!luaL_getmetafield(L, -1, @@ -191,6 +195,10 @@ luaT_httpc_request(lua_State *L) } lua_pop(L, 1); } + if (lua_type(L, -2) != LUA_TSTRING) + return luaL_error(L, + "opts.headers keys should be " + "strings"); if (httpc_set_header(req, "%s: %s", lua_tostring(L, -2), lua_tostring(L, -1)) < 0) { diff --git a/test/app-tap/http_client.test.lua b/test/app-tap/http_client.test.lua index 680c78b35..175bf8283 100755 --- a/test/app-tap/http_client.test.lua +++ b/test/app-tap/http_client.test.lua @@ -273,9 +273,15 @@ local function test_errors(test) local r = http:get("http://do_not_exist_8ffad33e0cb01e6a01a03d00089e71e5b2b7e9930dfcba.ru") end --- gh-3679 allow only headers can be converted to string +-- gh-3679 Check that opts.headers values can be strings or table +-- convertible to string only. +-- gh-4281 Check that opts.headers can be a table and opts.headers +-- keys can be strings only. local function test_request_headers(test, url, opts) - local exp_err = 'headers must be string or table with "__tostring"' + local exp_err_val = 'opts.headers values should be strings or tables with'.. + ' \"__tostring\"' + local exp_err_non_table = 'opts.headers should be a table' + local exp_err_bad_key = 'opts.headers keys should be strings' local cases = { { 'string header', @@ -297,32 +303,52 @@ local function test_request_headers(test, url, opts) { 'boolean header', opts = {headers = {aaa = true}}, - exp_err = exp_err, + exp_err = exp_err_val, }, { 'number header', opts = {headers = {aaa = 10}}, - exp_err = exp_err, + exp_err = exp_err_val, }, { 'cdata header (box.NULL)', opts = {headers = {aaa = box.NULL}}, - exp_err = exp_err, + exp_err = exp_err_val, }, { 'cdata<uint64_t> header', opts = {headers = {aaa = 10ULL}}, - exp_err = exp_err, + exp_err = exp_err_val, }, { 'table header w/o metatable', opts = {headers = {aaa = {}}}, - exp_err = exp_err, + exp_err = exp_err_val, }, { 'table header w/o __tostring() metamethod', opts = {headers = {aaa = setmetatable({}, {})}}, - exp_err = exp_err, + exp_err = exp_err_val, + }, + { + '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, + }, + { + 'number key', + opts = {headers = {'aaa'}}, + exp_err = exp_err_bad_key, }, } test:plan(#cases)
next prev parent reply other threads:[~2019-07-25 14:35 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 ` [tarantool-patches] " Roman Khabibov 2019-07-22 16:12 ` Alexander Turenko 2019-07-25 14:35 ` Roman Khabibov [this message] 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=8C721538-80AA-4926-8158-586B6907D6F2@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