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 3186B22055 for ; Mon, 22 Jul 2019 12:12:43 -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 tEP0aTM8OsYm for ; Mon, 22 Jul 2019 12:12:43 -0400 (EDT) Received: from smtp54.i.mail.ru (smtp54.i.mail.ru [217.69.128.34]) (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 83C0121FC3 for ; Mon, 22 Jul 2019 12:12:34 -0400 (EDT) Date: Mon, 22 Jul 2019 19:12:17 +0300 From: Alexander Turenko Subject: [tarantool-patches] Re: [PATCH] httpc: fix bug with segfault by wrong headers Message-ID: <20190722161216.xzrs3wvjzqo3u37h@tkn_work_nb> References: <20190616152409.18534-1-roman.habibov@tarantool.org> <20190624160423.ptng2eusy2osyyaj@tkn_work_nb> <01B8FFD0-AFA3-4821-AF68-2E17F08AF64A@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <01B8FFD0-AFA3-4821-AF68-2E17F08AF64A@tarantool.org> 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: Roman Khabibov Cc: tarantool-patches@freelists.org > > 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. > >> + 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. > -- 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 > Date: Sat Jun 15 19:46:28 2019 +0300 > > httpc: fix bug with segfault by wrong headers 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.