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

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

[Roman Khabibov]

There wasn't lua_istable() checking for field 'headers'.

Closes #4281
---

Branch: https://github.com/tarantool/tarantool/compare/romanhabibov/gh-4281-header
Issue: https://github.com/tarantool/tarantool/issues/4281

 src/lua/httpc.c                   | 66 ++++++++++++++++++++-----------
 test/app-tap/http_client.test.lua | 17 ++++++++
 2 files changed, 61 insertions(+), 22 deletions(-)

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
+ * @a L Lua stack. In case of an error, push it on  @a L.
+ *
+ * Added to reduce nesting.
+ *
+ * @param L Lua stack.
+ * @param req http request.
+ *
+ * @retval 0 on success.
+ * @retval non-zero on error.
+ */
+static int
+process_headers(struct lua_State *L, struct httpc_request *req)
+{
+	while (lua_next(L, -2) != 0) {
+		int header_type = lua_type(L, -1);
+		if (header_type != LUA_TSTRING) {
+			const char *err_msg = "headers must be string or table"\
+				" with \"__tostring\"";
+			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) {
+			httpc_request_delete(req);
+			return luaT_error(L);
+		}
+		lua_pop(L, 1);
+	}
+	return 0;
+}
+
 /* }}}
  */
 
@@ -177,28 +214,13 @@ luaT_httpc_request(lua_State *L)
 	lua_getfield(L, 5, "headers");
 	if (!lua_isnil(L, -1)) {
 		lua_pushnil(L);
-		while (lua_next(L, -2) != 0) {
-			int header_type = lua_type(L, -1);
-			if (header_type != LUA_TSTRING) {
-				const char *err_msg =
-					"headers must be string or table "\
-					"with \"__tostring\"";
-				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) {
-				httpc_request_delete(req);
-				return luaT_error(L);
-			}
-			lua_pop(L, 1);
-		}
+		int ret;
+		if (lua_istable(L, -2) != 0)
+			ret = process_headers(L, req);
+		else
+			return luaL_error(L, "field \"headers\" must be table");
+		if (ret != 0)
+			return ret;
 	}
 	lua_pop(L, 1);
 
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 = 'headers must be string or table with "__tostring"'
+    local exp_err_non_table = 'field "headers" must be table'
     local cases = {
         {
             'string header',
@@ -324,6 +325,22 @@ local function test_request_headers(test, url, opts)
             opts = {headers = {aaa = setmetatable({}, {})}},
             exp_err = exp_err,
         },
+-- gh-4281 allow only field 'headers' can be table
+        {
+            'non-table \'headers\'',
+            opts = {headers = 'aaa'},
+            exp_err = exp_err_non_table,
+        },
+        {
+            'non-table \'headers\'',
+            opts = {headers = 1},
+            exp_err = exp_err_non_table,
+        },
+        {
+            'non-table \'headers\'',
+            opts = {headers = box.NULL},
+            exp_err = exp_err_non_table,
+        },
     }
     test:plan(#cases)
 
-- 
2.20.1 (Apple Git-117)

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

[Roman Khabibov]

> On Jun 24, 2019, at 7:04 PM, Alexander Turenko <alexander.turenko@tarantool.org> wrote:
> 
> On Sun, Jun 16, 2019 at 06:24:09PM +0300, Roman Khabibov wrote:
>> There wasn't lua_istable() checking for field 'headers'.
>> 
>> Closes #4281
>> ---
>> 
>> Branch: https://github.com/tarantool/tarantool/compare/romanhabibov/gh-4281-header
>> Issue: https://github.com/tarantool/tarantool/issues/4281
>> 
>> src/lua/httpc.c                   | 66 ++++++++++++++++++++-----------
>> test/app-tap/http_client.test.lua | 17 ++++++++
>> 2 files changed, 61 insertions(+), 22 deletions(-)
>> 
>> 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
> 
> Nit: the Lua table -> a Lua table.
> 
> I think it is better to provide 'int idx' argument here and pass it from
> a caller.
> 
> 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.
> 
> I would state somehow that this is about outgoing headers: maybe it
> worth to rename it to something like 'fill_outgoing_headers'.
> 
>> + * @a L Lua stack. In case of an error, push it on  @a L.
> 
> Nit: double whitespace.
> 
> 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.
> 
>> + *
>> + * Added to reduce nesting.
> 
> I would not say it here as obvious.
> 
>> + *
>> + * @param L Lua stack.
>> + * @param req http request.
>> + *
>> + * @retval 0 on success.
>> + * @retval non-zero on error.
> 
> 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.
> 
> I would say -1 explicitly instead of 'non-zero’.
>> + */
>> +static int
>> +process_headers(struct lua_State *L, struct httpc_request *req)
>> +{
>> +	while (lua_next(L, -2) != 0) {
>> +		int header_type = lua_type(L, -1);
>> +		if (header_type != LUA_TSTRING) {
>> +			const char *err_msg = "headers must be string or table"\
>> +				" with \"__tostring\"";
> 
> 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.
> 
> 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).
> 
> A whitespace is needed before a trailing backslash.
> 
>> +			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.
> 
>> +			httpc_request_delete(req);
+
+/**
+ * Returns codes from fill_outgoing_headers().
+ */
+enum headers_status {
+	HEADERS_SUCCESS = 0,
+	HEADERS_BAD_HEADERS = 1,
+	HEADERS_BAD_KEYS = 2,
+	HEADERS_INTERNAL_ERR = 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) != 0) {
+		int header_type = lua_type(L, -1);
+		if (header_type != LUA_TSTRING) {
+			if (header_type != 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) != 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.
> 
>> +			return luaT_error(L);
>> +		}
>> +		lua_pop(L, 1);
>> +	}
>> +	return 0;
>> +}
>> +
>> /* }}}
>>  */
>> 
>> @@ -177,28 +214,13 @@ luaT_httpc_request(lua_State *L)
>> 	lua_getfield(L, 5, "headers");
>> 	if (!lua_isnil(L, -1)) {
>> 		lua_pushnil(L);
> 
> `nil` here is the first key for the lua_next() call. Please move it to
> process_headers() too.
> 
>> -		while (lua_next(L, -2) != 0) {
>> -			int header_type = lua_type(L, -1);
>> -			if (header_type != LUA_TSTRING) {
>> -				const char *err_msg =
>> -					"headers must be string or table "\
>> -					"with \"__tostring\"";
>> -				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) {
>> -				httpc_request_delete(req);
>> -				return luaT_error(L);
>> -			}
>> -			lua_pop(L, 1);
>> -		}
>> +		int ret;
> 
> We usually use name 'rc' for such variables.
> 
>> +		if (lua_istable(L, -2) != 0)
>> +			ret = process_headers(L, req);
>> +		else
>> +			return luaL_error(L, "field \"headers\" must be table");
>> +		if (ret != 0)
>> +			return ret;
> 
> I would write it so:
> 
> | if (lua_istable(L, -2) == 0) {
> |     httpc_request_delete(req);
> |     return luaL_error(L, "<...>");
> | }
> | if (process_headers(L, -2, req) != 0) {
> |     httpc_request_delete(req);
> |     return luaL_error(L, "<...>");
> | }
> 
> In this way we place error handling code inside if's, but the main logic
> outside of them. This is easier to read.
> 
> 'return ret;' looks as the mistake.
> 
>> 	}
>> 	lua_pop(L, 1);
>> 
>> 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 = 'headers must be string or table with "__tostring"'
>> +    local exp_err_non_table = 'field "headers" must be table'
> 
> Nit: should be a table.
> 
>>     local cases = {
>>         {
>>             'string header',
>> @@ -324,6 +325,22 @@ local function test_request_headers(test, url, opts)
>>             opts = {headers = {aaa = setmetatable({}, {})}},
>>             exp_err = exp_err,
>>         },
>> +-- gh-4281 allow only field 'headers' can be table
@@ -176,28 +223,22 @@ luaT_httpc_request(lua_State *L)
 
 	lua_getfield(L, 5, "headers");
 	if (!lua_isnil(L, -1)) {
-		lua_pushnil(L);
-		while (lua_next(L, -2) != 0) {
-			int header_type = lua_type(L, -1);
-			if (header_type != LUA_TSTRING) {
-				const char *err_msg =
-					"headers must be string or table "\
-					"with \"__tostring\"";
-				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) {
-				httpc_request_delete(req);
-				return luaT_error(L);
-			}
-			lua_pop(L, 1);
+		if (lua_istable(L, -1) == 0) {
+			httpc_request_delete(req);
+			return luaL_error(L, "\"headers\" field should be a "
+					  "table");
+		}
+		int rc = fill_outgoing_headers(L, -1, req);
+		if (rc == 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 == HEADERS_BAD_KEYS) {
+			httpc_request_delete(req);
+			return luaL_error(L, "header key is non-defined");
+		} else if (rc == 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.
> 
> 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.
> 
>> +        {
>> +            'non-table \'headers\'',
>> +            opts = {headers = 'aaa'},
>> +            exp_err = exp_err_non_table,
>> +        },
>> +        {
>> +            'non-table \'headers\'',
>> +            opts = {headers = 1},
>> +            exp_err = exp_err_non_table,
>> +        },
>> +        {
>> +            'non-table \'headers\'',
>> +            opts = {headers = box.NULL},
>> +            exp_err = exp_err_non_table,
>> +        },
> 
> I would name the cases differently, see the cases above for examples.
> 
>>     }
>>     test:plan(#cases)
>> 
>> -- 
>> 2.20.1 (Apple Git-117)
@@ -274,8 +274,12 @@ local function test_errors(test)
 end
 
 -- 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'
     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 <roman.habibov@tarantool.org>
Date:   Sat Jun 15 19:46:28 2019 +0300

    httpc: fix bug with segfault by wrong headers
    
    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.
    
    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 = 0,
+	HEADERS_BAD_HEADERS = 1,
+	HEADERS_BAD_KEYS = 2,
+	HEADERS_INTERNAL_ERR = 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) != 0) {
+		int header_type = lua_type(L, -1);
+		if (header_type != LUA_TSTRING) {
+			if (header_type != 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) != 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;
+}
+
 /* }}}
  */
 
@@ -176,28 +223,22 @@ luaT_httpc_request(lua_State *L)
 
 	lua_getfield(L, 5, "headers");
 	if (!lua_isnil(L, -1)) {
-		lua_pushnil(L);
-		while (lua_next(L, -2) != 0) {
-			int header_type = lua_type(L, -1);
-			if (header_type != LUA_TSTRING) {
-				const char *err_msg =
-					"headers must be string or table "\
-					"with \"__tostring\"";
-				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) {
-				httpc_request_delete(req);
-				return luaT_error(L);
-			}
-			lua_pop(L, 1);
+		if (lua_istable(L, -1) == 0) {
+			httpc_request_delete(req);
+			return luaL_error(L, "\"headers\" field should be a "
+					  "table");
+		}
+		int rc = fill_outgoing_headers(L, -1, req);
+		if (rc == 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 == HEADERS_BAD_KEYS) {
+			httpc_request_delete(req);
+			return luaL_error(L, "header key is non-defined");
+		} else if (rc == 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
 
 -- 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'
     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)
 

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

[Alexander Turenko]

> > 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 <roman.habibov@tarantool.org>
> 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.

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

[Alexander Turenko]

> +/**
> + * 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)

BTW, are we really need to extract this code into the function? As I see
the change of the question can be done w/o increasing an indent level.

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

[Roman Khabibov]

> On Jul 22, 2019, at 19:12, Alexander Turenko <alexander.turenko@tarantool.org> wrote:
> 
>>> 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.
    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

> 
>>>> +			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.
@@ -176,13 +176,17 @@ luaT_httpc_request(lua_State *L)
 
 	lua_getfield(L, 5, "headers");
 	if (!lua_isnil(L, -1)) {
+		if (lua_istable(L, -1) == 0) {
+			httpc_request_delete(req);
+			return luaL_error(L, "opts.headers should be a table");
+		}
 		lua_pushnil(L);
 		while (lua_next(L, -2) != 0) {
 			int header_type = lua_type(L, -1);
 			if (header_type != LUA_TSTRING) {
 				const char *err_msg =
-					"headers must be string or table "\
-					"with \"__tostring\"";
+					"opts.headers values should be strings "
+					"or tables with \"__tostring\"";
 				if (header_type != LUA_TTABLE) {
 					return luaL_error(L, err_msg);
 				} else if (!luaL_getmetafield(L, -1,
@@ -191,6 +195,10 @@ luaT_httpc_request(lua_State *L)
 				}
 				lua_pop(L, 1);
 			}
+			if (lua_type(L, -2) != LUA_TSTRING)
+				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) {

>> -- 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 <roman.habibov@tarantool.org>
>> Date:   Sat Jun 15 19:46:28 2019 +0300
>> 
>>    httpc: fix bug with segfault by wrong headers
--- gh-3679 allow only headers can be converted to string
+-- gh-3679 Check that opts.headers values can be strings or table
+-- convertible to string only.
+-- 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 = 'headers must be string or table with "__tostring"'
+    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_key = 'opts.headers keys should be strings'
     local cases = {
         {
             'string header',
@@ -297,32 +303,52 @@ local function test_request_headers(test, url, opts)
         {
             'boolean header',
             opts = {headers = {aaa = true}},
-            exp_err = exp_err,
+            exp_err = exp_err_val,
         },
         {
             'number header',
             opts = {headers = {aaa = 10}},
-            exp_err = exp_err,
+            exp_err = exp_err_val,
         },
         {
             'cdata header (box.NULL)',
             opts = {headers = {aaa = box.NULL}},
-            exp_err = exp_err,
+            exp_err = exp_err_val,
         },
         {
             'cdata<uint64_t> header',
             opts = {headers = {aaa = 10ULL}},
-            exp_err = exp_err,
+            exp_err = exp_err_val,
         },
         {
             'table header w/o metatable',
             opts = {headers = {aaa = {}}},
-            exp_err = exp_err,
+            exp_err = exp_err_val,
         },
         {
             'table header w/o __tostring() metamethod',
             opts = {headers = {aaa = setmetatable({}, {})}},
-            exp_err = exp_err,
+            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,
         },
     }
     test:plan(#cases)

> 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.

commit 2a839a6c952f145c2979426de507ae4b88b44bf3
Author: Roman Khabibov <roman.habibov@tarantool.org>
Date:   Sat Jun 15 19:46:28 2019 +0300

    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

diff --git a/src/lua/httpc.c b/src/lua/httpc.c
index bc8d498ba..ffc9f8a0b 100644
--- a/src/lua/httpc.c
+++ b/src/lua/httpc.c
@@ -176,13 +176,17 @@ luaT_httpc_request(lua_State *L)
 
 	lua_getfield(L, 5, "headers");
 	if (!lua_isnil(L, -1)) {
+		if (lua_istable(L, -1) == 0) {
+			httpc_request_delete(req);
+			return luaL_error(L, "opts.headers should be a table");
+		}
 		lua_pushnil(L);
 		while (lua_next(L, -2) != 0) {
 			int header_type = lua_type(L, -1);
 			if (header_type != LUA_TSTRING) {
 				const char *err_msg =
-					"headers must be string or table "\
-					"with \"__tostring\"";
+					"opts.headers values should be strings "
+					"or tables with \"__tostring\"";
 				if (header_type != LUA_TTABLE) {
 					return luaL_error(L, err_msg);
 				} else if (!luaL_getmetafield(L, -1,
@@ -191,6 +195,10 @@ luaT_httpc_request(lua_State *L)
 				}
 				lua_pop(L, 1);
 			}
+			if (lua_type(L, -2) != LUA_TSTRING)
+				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 --git a/test/app-tap/http_client.test.lua b/test/app-tap/http_client.test.lua
index 680c78b35..175bf8283 100755
--- a/test/app-tap/http_client.test.lua
+++ b/test/app-tap/http_client.test.lua
@@ -273,9 +273,15 @@ local function test_errors(test)
     local r = http:get("http://do_not_exist_8ffad33e0cb01e6a01a03d00089e71e5b2b7e9930dfcba.ru")
 end
 
--- gh-3679 allow only headers can be converted to string
+-- gh-3679 Check that opts.headers values can be strings or table
+-- convertible to string only.
+-- 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 = 'headers must be string or table with "__tostring"'
+    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_key = 'opts.headers keys should be strings'
     local cases = {
         {
             'string header',
@@ -297,32 +303,52 @@ local function test_request_headers(test, url, opts)
         {
             'boolean header',
             opts = {headers = {aaa = true}},
-            exp_err = exp_err,
+            exp_err = exp_err_val,
         },
         {
             'number header',
             opts = {headers = {aaa = 10}},
-            exp_err = exp_err,
+            exp_err = exp_err_val,
         },
         {
             'cdata header (box.NULL)',
             opts = {headers = {aaa = box.NULL}},
-            exp_err = exp_err,
+            exp_err = exp_err_val,
         },
         {
             'cdata<uint64_t> header',
             opts = {headers = {aaa = 10ULL}},
-            exp_err = exp_err,
+            exp_err = exp_err_val,
         },
         {
             'table header w/o metatable',
             opts = {headers = {aaa = {}}},
-            exp_err = exp_err,
+            exp_err = exp_err_val,
         },
         {
             'table header w/o __tostring() metamethod',
             opts = {headers = {aaa = setmetatable({}, {})}},
-            exp_err = exp_err,
+            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,
         },
     }
     test:plan(#cases)

[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)

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

[Kirill Yukhin]

Hello,

On 16 Jun 18:24, Roman Khabibov wrote:
> There wasn't lua_istable() checking for field 'headers'.
> 
> Closes #4281
> ---
> 
> Branch: https://github.com/tarantool/tarantool/compare/romanhabibov/gh-4281-header
> Issue: https://github.com/tarantool/tarantool/issues/4281

I've checked your patch into master.

--
Regards, Kirill Yukhin

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

[Alexander Turenko]

On Mon, Jul 29, 2019 at 01:59:25PM +0300, Kirill Yukhin wrote:
> Hello,
> 
> On 16 Jun 18:24, Roman Khabibov wrote:
> > There wasn't lua_istable() checking for field 'headers'.
> > 
> > Closes #4281
> > ---
> > 
> > Branch: https://github.com/tarantool/tarantool/compare/romanhabibov/gh-4281-header
> > Issue: https://github.com/tarantool/tarantool/issues/4281
> 
> I've checked your patch into master.

https://github.com/tarantool/tarantool/issues/3679 was pushed to 2.1 and
1.10 too (85e1d78bc6605ab189b4253867a5f72ebf356f4a,
328ec3655021fb3eaf0441998f60d6c2630f1008).
https://github.com/tarantool/tarantool/issues/4281 is the bug according
to labels and we backport bugfixes backward as I understand our process.

I think this fix should be backported to 2.1 and 1.10.

BTW, the patch fixes a memleak: it is rare (only when arguments are
incorrect), but anyway.

WBR, Alexander Turenko.

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

[Kirill Yukhin]

On 29 Jul 15:25, Alexander Turenko wrote:
> On Mon, Jul 29, 2019 at 01:59:25PM +0300, Kirill Yukhin wrote:
> > Hello,
> > 
> > On 16 Jun 18:24, Roman Khabibov wrote:
> > > There wasn't lua_istable() checking for field 'headers'.
> > > 
> > > Closes #4281
> > > ---
> > > 
> > > Branch: https://github.com/tarantool/tarantool/compare/romanhabibov/gh-4281-header
> > > Issue: https://github.com/tarantool/tarantool/issues/4281
> > 
> > I've checked your patch into master.
> 
> https://github.com/tarantool/tarantool/issues/3679 was pushed to 2.1 and
> 1.10 too (85e1d78bc6605ab189b4253867a5f72ebf356f4a,
> 328ec3655021fb3eaf0441998f60d6c2630f1008).
> https://github.com/tarantool/tarantool/issues/4281 is the bug according
> to labels and we backport bugfixes backward as I understand our process.
> 
> I think this fix should be backported to 2.1 and 1.10.

Done.

--
Regards, Kirill Yukhin