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