[tarantool-patches] Re: [PATCH] httpc: fix bug with segfault by wrong headers
Roman Khabibov
roman.habibov at tarantool.org
Thu Jul 25 17:35:22 MSK 2019
> On Jul 22, 2019, at 19:12, Alexander Turenko <alexander.turenko at 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 at 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 at 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)
More information about the Tarantool-patches
mailing list