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 3C53C20340 for ; Fri, 28 Jun 2019 10:37:30 -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 BTS_-NE7NH87 for ; Fri, 28 Jun 2019 10:37:30 -0400 (EDT) Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 9C6792030E for ; Fri, 28 Jun 2019 10:37:26 -0400 (EDT) Content-Type: text/plain; charset=utf-8 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: <20190624160423.ptng2eusy2osyyaj@tkn_work_nb> Date: Fri, 28 Jun 2019 17:37:24 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <01B8FFD0-AFA3-4821-AF68-2E17F08AF64A@tarantool.org> References: <20190616152409.18534-1-roman.habibov@tarantool.org> <20190624160423.ptng2eusy2osyyaj@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 Jun 24, 2019, at 7:04 PM, Alexander Turenko = wrote: >=20 > On Sun, Jun 16, 2019 at 06:24:09PM +0300, Roman Khabibov wrote: >> There wasn't lua_istable() checking for field 'headers'. >>=20 >> Closes #4281 >> --- >>=20 >> Branch: = https://github.com/tarantool/tarantool/compare/romanhabibov/gh-4281-header= >> Issue: https://github.com/tarantool/tarantool/issues/4281 >>=20 >> src/lua/httpc.c | 66 = ++++++++++++++++++++----------- >> test/app-tap/http_client.test.lua | 17 ++++++++ >> 2 files changed, 61 insertions(+), 22 deletions(-) >>=20 >> 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 >=20 > Nit: the Lua table -> a Lua table. >=20 > I think it is better to provide 'int idx' argument here and pass it = from > a caller. >=20 > 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. >=20 > I would state somehow that this is about outgoing headers: maybe it > worth to rename it to something like 'fill_outgoing_headers'. >=20 >> + * @a L Lua stack. In case of an error, push it on @a L. >=20 > Nit: double whitespace. >=20 > 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. >=20 >> + * >> + * Added to reduce nesting. >=20 > I would not say it here as obvious. >=20 >> + * >> + * @param L Lua stack. >> + * @param req http request. >> + * >> + * @retval 0 on success. >> + * @retval non-zero on error. >=20 > 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. >=20 > I would say -1 explicitly instead of 'non-zero=E2=80=99. >> + */ >> +static int >> +process_headers(struct lua_State *L, struct httpc_request *req) >> +{ >> + 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\""; >=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 > 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). >=20 > A whitespace is needed before a trailing backslash. >=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 >> + httpc_request_delete(req); + +/** + * 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 +}; + + +/** + * 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) !=3D 0) { + int header_type =3D lua_type(L, -1); + if (header_type !=3D LUA_TSTRING) { + if (header_type !=3D 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) !=3D 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. >=20 >> + return luaT_error(L); >> + } >> + lua_pop(L, 1); >> + } >> + return 0; >> +} >> + >> /* }}} >> */ >>=20 >> @@ -177,28 +214,13 @@ luaT_httpc_request(lua_State *L) >> lua_getfield(L, 5, "headers"); >> if (!lua_isnil(L, -1)) { >> lua_pushnil(L); >=20 > `nil` here is the first key for the lua_next() call. Please move it to > process_headers() too. >=20 >> - 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\""; >> - 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) { >> - httpc_request_delete(req); >> - return luaT_error(L); >> - } >> - lua_pop(L, 1); >> - } >> + int ret; >=20 > We usually use name 'rc' for such variables. >=20 >> + if (lua_istable(L, -2) !=3D 0) >> + ret =3D process_headers(L, req); >> + else >> + return luaL_error(L, "field \"headers\" must be = table"); >> + if (ret !=3D 0) >> + return ret; >=20 > I would write it so: >=20 > | if (lua_istable(L, -2) =3D=3D 0) { > | httpc_request_delete(req); > | return luaL_error(L, "<...>"); > | } > | if (process_headers(L, -2, req) !=3D 0) { > | httpc_request_delete(req); > | return luaL_error(L, "<...>"); > | } >=20 > In this way we place error handling code inside if's, but the main = logic > outside of them. This is easier to read. >=20 > 'return ret;' looks as the mistake. >=20 >> } >> lua_pop(L, 1); >>=20 >> 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 =3D 'headers must be string or table with = "__tostring"' >> + local exp_err_non_table =3D 'field "headers" must be table' >=20 > Nit: should be a table. >=20 >> local cases =3D { >> { >> 'string header', >> @@ -324,6 +325,22 @@ local function test_request_headers(test, url, = opts) >> opts =3D {headers =3D {aaa =3D setmetatable({}, {})}}, >> exp_err =3D exp_err, >> }, >> +-- gh-4281 allow only field 'headers' can be table @@ -176,28 +223,22 @@ luaT_httpc_request(lua_State *L) =20 lua_getfield(L, 5, "headers"); if (!lua_isnil(L, -1)) { - 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\""; - 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) { - httpc_request_delete(req); - return luaT_error(L); - } - lua_pop(L, 1); + if (lua_istable(L, -1) =3D=3D 0) { + httpc_request_delete(req); + return luaL_error(L, "\"headers\" field should = be a " + "table"); + } + int rc =3D fill_outgoing_headers(L, -1, req); + if (rc =3D=3D 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 =3D=3D HEADERS_BAD_KEYS) { + httpc_request_delete(req); + return luaL_error(L, "header key is = non-defined"); + } else if (rc =3D=3D 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. >=20 > 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. >=20 >> + { >> + 'non-table \'headers\'', >> + opts =3D {headers =3D 'aaa'}, >> + exp_err =3D exp_err_non_table, >> + }, >> + { >> + 'non-table \'headers\'', >> + opts =3D {headers =3D 1}, >> + exp_err =3D exp_err_non_table, >> + }, >> + { >> + 'non-table \'headers\'', >> + opts =3D {headers =3D box.NULL}, >> + exp_err =3D exp_err_non_table, >> + }, >=20 > I would name the cases differently, see the cases above for examples. >=20 >> } >> test:plan(#cases) >>=20 >> --=20 >> 2.20.1 (Apple Git-117) @@ -274,8 +274,12 @@ local function test_errors(test) end =20 -- 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' 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) commit b3143a487d28b56c3965e160686a7f570d2d6e03 Author: Roman Khabibov Date: Sat Jun 15 19:46:28 2019 +0300 httpc: fix bug with segfault by wrong headers =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 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 =3D 0, + HEADERS_BAD_HEADERS =3D 1, + HEADERS_BAD_KEYS =3D 2, + HEADERS_INTERNAL_ERR =3D 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) !=3D 0) { + int header_type =3D lua_type(L, -1); + if (header_type !=3D LUA_TSTRING) { + if (header_type !=3D 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) !=3D 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; +} + /* }}} */ =20 @@ -176,28 +223,22 @@ luaT_httpc_request(lua_State *L) =20 lua_getfield(L, 5, "headers"); if (!lua_isnil(L, -1)) { - 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\""; - 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) { - httpc_request_delete(req); - return luaT_error(L); - } - lua_pop(L, 1); + if (lua_istable(L, -1) =3D=3D 0) { + httpc_request_delete(req); + return luaL_error(L, "\"headers\" field should = be a " + "table"); + } + int rc =3D fill_outgoing_headers(L, -1, req); + if (rc =3D=3D 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 =3D=3D HEADERS_BAD_KEYS) { + httpc_request_delete(req); + return luaL_error(L, "header key is = non-defined"); + } else if (rc =3D=3D 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 =20 -- 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' 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