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 E713223042 for ; Fri, 26 Jul 2019 11:17:32 -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 Jqp9Mqu5FcUF for ; Fri, 26 Jul 2019 11:17:32 -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 478E4215F6 for ; Fri, 26 Jul 2019 11:17:32 -0400 (EDT) Date: Fri, 26 Jul 2019 18:17:15 +0300 From: Alexander Turenko Subject: [tarantool-patches] Re: [PATCH] httpc: fix bug with segfault by wrong headers Message-ID: <20190726151715.f4tjknyqrszkpr7w@tkn_work_nb> 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> <8C721538-80AA-4926-8158-586B6907D6F2@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <8C721538-80AA-4926-8158-586B6907D6F2@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, Kirill Yukhin I'm generally ok with the patch. Made minor corrections for the commit message and the test, see below. Also found a memory leak (when bad headers are passed by a user) and fixed. Kirill, I gave LGTM for this patch. Please, proceed. Issue: https://github.com/tarantool/tarantool/issues/4281 Branch: https://github.com/tarantool/tarantool/tree/romanhabibov/gh-4281-header WBR, Alexander Turenko. ---- Original commit message: | 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 Mine version: | httpc: verify "headers" option stronger | | Added the following checks: | | * opts.headers is a table. | * opts.headers keys are strings. | | Clarified an error message re Lua type of opts.headers values. | | Found and fixed a memory leak that appears in http client when invalid | opts.headers is passed. | | Closes #4281 ---- Diff for the code: diff --git a/src/lua/httpc.c b/src/lua/httpc.c index ffc9f8a0b..4e78688fa 100644 --- a/src/lua/httpc.c +++ b/src/lua/httpc.c @@ -188,17 +188,20 @@ luaT_httpc_request(lua_State *L) "opts.headers values should be strings " "or tables with \"__tostring\""; if (header_type != LUA_TTABLE) { + httpc_request_delete(req); return luaL_error(L, err_msg); } else if (!luaL_getmetafield(L, -1, "__tostring")) { + httpc_request_delete(req); return luaL_error(L, err_msg); } lua_pop(L, 1); } - if (lua_type(L, -2) != LUA_TSTRING) - return luaL_error(L, - "opts.headers keys should be " - "strings"); + if (lua_type(L, -2) != LUA_TSTRING) { + httpc_request_delete(req); + 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 for the test: diff --git a/test/app-tap/http_client.test.lua b/test/app-tap/http_client.test.lua index 175bf8283..25fab89ab 100755 --- a/test/app-tap/http_client.test.lua +++ b/test/app-tap/http_client.test.lua @@ -278,18 +278,56 @@ end -- 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_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_opts_headers = 'opts.headers should be a table' local exp_err_bad_key = 'opts.headers keys should be strings' + local exp_err_bad_value = 'opts.headers values should be strings or ' .. + 'tables with "__tostring"' local cases = { + -- Verify opts.headers type checks. { - 'string header', + 'string opts.headers', + opts = {headers = 'aaa'}, + exp_err = exp_err_bad_opts_headers, + }, + { + 'number opts.headers', + opts = {headers = 1}, + exp_err = exp_err_bad_opts_headers, + }, + { + 'cdata (box.NULL) opts.headers', + opts = {headers = box.NULL}, + exp_err = exp_err_bad_opts_headers, + }, + -- Verify a header key type checks. + { + 'number header key', + opts = {headers = {[1] = 'aaa'}}, + exp_err = exp_err_bad_key, + }, + { + 'boolean header key', + opts = {headers = {[true] = 'aaa'}}, + exp_err = exp_err_bad_key, + }, + { + 'table header key', + opts = {headers = {[{}] = 'aaa'}}, + exp_err = exp_err_bad_key, + }, + { + 'cdata header key (box.NULL)', + opts = {headers = {[box.NULL] = 'aaa'}}, + exp_err = exp_err_bad_key, + }, + -- Verify a header value type checks. + { + 'string header key & value', opts = {headers = {aaa = 'aaa'}}, exp_err = nil, }, { - 'header with __tostring() metamethod', + 'header value with __tostring() metamethod', opts = {headers = {aaa = setmetatable({}, { __tostring = function(self) return 'aaa' @@ -301,54 +339,34 @@ local function test_request_headers(test, url, opts) end, }, { - 'boolean header', + 'boolean header value', opts = {headers = {aaa = true}}, - exp_err = exp_err_val, + exp_err = exp_err_bad_value, }, { - 'number header', + 'number header value', opts = {headers = {aaa = 10}}, - exp_err = exp_err_val, + exp_err = exp_err_bad_value, }, { - 'cdata header (box.NULL)', + 'cdata header value (box.NULL)', opts = {headers = {aaa = box.NULL}}, - exp_err = exp_err_val, + exp_err = exp_err_bad_value, }, { - 'cdata header', + 'cdata header value', opts = {headers = {aaa = 10ULL}}, - exp_err = exp_err_val, + exp_err = exp_err_bad_value, }, { - 'table header w/o metatable', + 'table header value w/o metatable', opts = {headers = {aaa = {}}}, - exp_err = exp_err_val, + exp_err = exp_err_bad_value, }, { - 'table header w/o __tostring() metamethod', + 'table header value w/o __tostring() metamethod', opts = {headers = {aaa = setmetatable({}, {})}}, - 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, + exp_err = exp_err_bad_value, }, } test:plan(#cases)