[tarantool-patches] Re: [PATCH] httpc: fix bug with segfault by wrong headers
Roman Khabibov
roman.habibov at tarantool.org
Fri Jun 28 17:37:24 MSK 2019
> On Jun 24, 2019, at 7:04 PM, Alexander Turenko <alexander.turenko at 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 at 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)
More information about the Tarantool-patches
mailing list