Tarantool development patches archive
 help / color / mirror / Atom feed
From: Alexander Turenko <alexander.turenko@tarantool.org>
To: Roman Khabibov <roman.habibov@tarantool.org>
Cc: tarantool-patches@freelists.org, Kirill Yukhin <kyukhin@tarantool.org>
Subject: [tarantool-patches] Re: [PATCH] httpc: fix bug with segfault by wrong headers
Date: Fri, 26 Jul 2019 18:17:15 +0300	[thread overview]
Message-ID: <20190726151715.f4tjknyqrszkpr7w@tkn_work_nb> (raw)
In-Reply-To: <8C721538-80AA-4926-8158-586B6907D6F2@tarantool.org>

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<uint64_t> header',
+            'cdata<uint64_t> 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)

  reply	other threads:[~2019-07-26 15:17 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
2019-07-26 15:17         ` Alexander Turenko [this message]
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=20190726151715.f4tjknyqrszkpr7w@tkn_work_nb \
    --to=alexander.turenko@tarantool.org \
    --cc=kyukhin@tarantool.org \
    --cc=roman.habibov@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