[tarantool-patches] Re: [PATCH] httpc: fix bug with segfault by wrong headers

[Alexander Turenko]

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)