From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id D943325759 for ; Thu, 25 Jul 2019 10:35:25 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id d7k36JkYYCwH for ; Thu, 25 Jul 2019 10:35:25 -0400 (EDT) Received: from smtp37.i.mail.ru (smtp37.i.mail.ru [94.100.177.97]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 1071E25756 for ; Thu, 25 Jul 2019 10:35:24 -0400 (EDT) Content-Type: text/plain; charset=us-ascii Mime-Version: 1.0 (Mac OS X Mail 12.2 \(3445.102.3\)) Subject: [tarantool-patches] Re: [PATCH] httpc: fix bug with segfault by wrong headers From: Roman Khabibov In-Reply-To: <20190722161216.xzrs3wvjzqo3u37h@tkn_work_nb> Date: Thu, 25 Jul 2019 17:35:22 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <8C721538-80AA-4926-8158-586B6907D6F2@tarantool.org> References: <20190616152409.18534-1-roman.habibov@tarantool.org> <20190624160423.ptng2eusy2osyyaj@tkn_work_nb> <01B8FFD0-AFA3-4821-AF68-2E17F08AF64A@tarantool.org> <20190722161216.xzrs3wvjzqo3u37h@tkn_work_nb> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: tarantool-patches@freelists.org Cc: Alexander Turenko > On Jul 22, 2019, at 19:12, Alexander Turenko = wrote: >=20 >>> 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. >=20 > As I see it is not done. httpc: verify "headers" option type =20 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. =20 Closes #4281 >=20 >>>> + if (header_type !=3D 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) { >>>=20 >>> It was here before, but anyway: >>>=20 >>> | 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. >>>=20 >>> http://pgl.yoyo.org/luai/i/lua_next >>>=20 >>> I would check that a key is a string too. This needs test cases and = need >>> to be mentioned in the commit message. >=20 > 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. >=20 >> +/** >> + * Returns codes from fill_outgoing_headers(). >> + */ >> +enum headers_status { >> + HEADERS_SUCCESS =3D 0, >> + HEADERS_BAD_HEADERS =3D 1, >> + HEADERS_BAD_KEYS =3D 2, >> + HEADERS_INTERNAL_ERR =3D 3 >> +}; >=20 > 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) =20 lua_getfield(L, 5, "headers"); if (!lua_isnil(L, -1)) { + if (lua_istable(L, -1) =3D=3D 0) { + httpc_request_delete(req); + return luaL_error(L, "opts.headers should be a = table"); + } lua_pushnil(L); while (lua_next(L, -2) !=3D 0) { int header_type =3D lua_type(L, -1); if (header_type !=3D LUA_TSTRING) { const char *err_msg =3D - "headers must be string or table = "\ - "with \"__tostring\""; + "opts.headers values should be = strings " + "or tables with \"__tostring\""; if (header_type !=3D 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) !=3D 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 =3D 'headers must be string or table with = "__tostring"' >> + local exp_err =3D 'headers should be a string or a table with a = "__tostring"' >> + local exp_err_non_table =3D '"headers" field should be a table' >> + local exp_err_bad_key =3D 'header key is non-defined' >=20 > Please, work on wording of comments and error messages. They are very > confusing in many ways: >=20 > * 'allow only headers can be converted to string' -- does you mean the > following? >=20 >> 'headers' option should be a table with fields that are strings or >> convertible to strings >=20 > * What is 'defined' and 'non-defined'? I don't know any common meaning > that would be applicable here. >=20 >> local cases =3D { >> { >> 'string header', >> @@ -324,6 +328,26 @@ local function test_request_headers(test, url, = opts) >> opts =3D {headers =3D {aaa =3D setmetatable({}, {})}}, >> exp_err =3D exp_err, >> }, >> + { >> + 'string \'headers\'', >> + opts =3D {headers =3D 'aaa'}, >> + exp_err =3D exp_err_non_table, >> + }, >> + { >> + 'number \'headers\'', >> + opts =3D {headers =3D 1}, >> + exp_err =3D exp_err_non_table, >> + }, >> + { >> + 'cdata (box.NULL) \'headers\'', >> + opts =3D {headers =3D box.NULL}, >> + exp_err =3D exp_err_non_table, >> + }, >> + { >> + 'non-defined key', >> + opts =3D {headers =3D {'aaa'}}, >> + exp_err =3D exp_err_bad_key, >> + }, >> } >> test:plan(#cases) >>=20 >> commit b3143a487d28b56c3965e160686a7f570d2d6e03 >> Author: Roman Khabibov >> Date: Sat Jun 15 19:46:28 2019 +0300 >>=20 >> 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 =3D 'headers must be string or table with = "__tostring"' + local exp_err_val =3D 'opts.headers values should be strings or = tables with'.. + ' \"__tostring\"' + local exp_err_non_table =3D 'opts.headers should be a table' + local exp_err_bad_key =3D 'opts.headers keys should be strings' local cases =3D { { 'string header', @@ -297,32 +303,52 @@ local function test_request_headers(test, url, = opts) { 'boolean header', opts =3D {headers =3D {aaa =3D true}}, - exp_err =3D exp_err, + exp_err =3D exp_err_val, }, { 'number header', opts =3D {headers =3D {aaa =3D 10}}, - exp_err =3D exp_err, + exp_err =3D exp_err_val, }, { 'cdata header (box.NULL)', opts =3D {headers =3D {aaa =3D box.NULL}}, - exp_err =3D exp_err, + exp_err =3D exp_err_val, }, { 'cdata header', opts =3D {headers =3D {aaa =3D 10ULL}}, - exp_err =3D exp_err, + exp_err =3D exp_err_val, }, { 'table header w/o metatable', opts =3D {headers =3D {aaa =3D {}}}, - exp_err =3D exp_err, + exp_err =3D exp_err_val, }, { 'table header w/o __tostring() metamethod', opts =3D {headers =3D {aaa =3D setmetatable({}, {})}}, - exp_err =3D exp_err, + exp_err =3D exp_err_val, + }, + { + 'string \'headers\'', + opts =3D {headers =3D 'aaa'}, + exp_err =3D exp_err_non_table, + }, + { + 'number \'headers\'', + opts =3D {headers =3D 1}, + exp_err =3D exp_err_non_table, + }, + { + 'cdata (box.NULL) \'headers\'', + opts =3D {headers =3D box.NULL}, + exp_err =3D exp_err_non_table, + }, + { + 'number key', + opts =3D {headers =3D {'aaa'}}, + exp_err =3D exp_err_bad_key, }, } test:plan(#cases) > I would say it in more certain way, say, 'httpc: verify "headers" = option > type'. >=20 >> 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. >=20 > 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 Date: Sat Jun 15 19:46:28 2019 +0300 httpc: verify "headers" option type =20 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. =20 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) =20 lua_getfield(L, 5, "headers"); if (!lua_isnil(L, -1)) { + if (lua_istable(L, -1) =3D=3D 0) { + httpc_request_delete(req); + return luaL_error(L, "opts.headers should be a = table"); + } lua_pushnil(L); while (lua_next(L, -2) !=3D 0) { int header_type =3D lua_type(L, -1); if (header_type !=3D LUA_TSTRING) { const char *err_msg =3D - "headers must be string or table = "\ - "with \"__tostring\""; + "opts.headers values should be = strings " + "or tables with \"__tostring\""; if (header_type !=3D 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) !=3D 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 =3D = http:get("http://do_not_exist_8ffad33e0cb01e6a01a03d00089e71e5b2b7e9930dfc= ba.ru") end =20 --- 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 =3D 'headers must be string or table with = "__tostring"' + local exp_err_val =3D 'opts.headers values should be strings or = tables with'.. + ' \"__tostring\"' + local exp_err_non_table =3D 'opts.headers should be a table' + local exp_err_bad_key =3D 'opts.headers keys should be strings' local cases =3D { { 'string header', @@ -297,32 +303,52 @@ local function test_request_headers(test, url, = opts) { 'boolean header', opts =3D {headers =3D {aaa =3D true}}, - exp_err =3D exp_err, + exp_err =3D exp_err_val, }, { 'number header', opts =3D {headers =3D {aaa =3D 10}}, - exp_err =3D exp_err, + exp_err =3D exp_err_val, }, { 'cdata header (box.NULL)', opts =3D {headers =3D {aaa =3D box.NULL}}, - exp_err =3D exp_err, + exp_err =3D exp_err_val, }, { 'cdata header', opts =3D {headers =3D {aaa =3D 10ULL}}, - exp_err =3D exp_err, + exp_err =3D exp_err_val, }, { 'table header w/o metatable', opts =3D {headers =3D {aaa =3D {}}}, - exp_err =3D exp_err, + exp_err =3D exp_err_val, }, { 'table header w/o __tostring() metamethod', opts =3D {headers =3D {aaa =3D setmetatable({}, {})}}, - exp_err =3D exp_err, + exp_err =3D exp_err_val, + }, + { + 'string \'headers\'', + opts =3D {headers =3D 'aaa'}, + exp_err =3D exp_err_non_table, + }, + { + 'number \'headers\'', + opts =3D {headers =3D 1}, + exp_err =3D exp_err_non_table, + }, + { + 'cdata (box.NULL) \'headers\'', + opts =3D {headers =3D box.NULL}, + exp_err =3D exp_err_non_table, + }, + { + 'number key', + opts =3D {headers =3D {'aaa'}}, + exp_err =3D exp_err_bad_key, }, } test:plan(#cases)