[tarantool-patches] Re: [PATCH v2 1/3] lua-yaml: verify arguments count

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hi! Thanks for the fixes!

>>> functions.
>>>
>>> Without these checks the functions could read garbage outside of a Lua
>>> stack when called w/o arguments.
>>
>> Honestly, I do not understand how is it possible. Please,
>> provide a test for both functions. See my 3 doubts below.
> 
> lua_isstring(L, 1) checks a garbage w/o preliminary lua_gettop() check.
> yaml.encode() gives me "unsupported Lua type 'thread'" on the current
> tarantool 2.1.

I looked at lua_isstring implementation, and I see, that it checks
top. If an index is above top, then the type is nil.

	static TValue *index2adr(lua_State *L, int idx)
	{
	  if (idx > 0) {
	    TValue *o = L->base + (idx - 1);
	    return o < L->top ? o : niltv(L);
	...

> 
> Anyway, added bad API usage test cases. Also I changed this:
> 
> diff --git a/third_party/lua-yaml/lyaml.cc b/third_party/lua-yaml/lyaml.cc
> index 3a427263e..46374970f 100644
> --- a/third_party/lua-yaml/lyaml.cc
> +++ b/third_party/lua-yaml/lyaml.cc
> @@ -453,7 +453,7 @@ usage_error:
>         return luaL_error(L, OOM_ERRMSG);
>      yaml_parser_set_input_string(&loader.parser, (yaml_char_t *) document, len);
>      bool tag_only;
> -   if (lua_gettop(L) == 2) {
> +   if (lua_gettop(L) == 2 && ! lua_isnil(L, 2)) {
>         if (! lua_istable(L, 2))
>            goto usage_error;
>         lua_getfield(L, 2, "tag_only");
> 
> We should not raise an usage error for yaml.decode(object, nil).

Why? It is said, that the second value either does not exist, or
is a table. Nil is not a table. So why? If your logic was about
considering nil as a not existing value, then why don't we handle
cases like this: yaml.decode(object, nil, nil, nil, nil) ? The same
for l_dump() and encode.

>>>    usage_error:
>>>          return luaL_error(L, "Usage: yaml.decode(document, "\
>>>                            "[{tag_only = boolean}])");
>>> @@ -416,7 +417,7 @@ usage_error:
>>>          return luaL_error(L, OOM_ERRMSG);
>>>       yaml_parser_set_input_string(&loader.parser, (yaml_char_t *) document, len);
>>>       bool tag_only;
>>> -   if (lua_gettop(L) > 1) {
>>> +   if (lua_gettop(L) == 2) {
>>
>> 2. This function never touches anything beyond second value on
>> the stack, so here lua_gettop(L) > 1 means the same as
>> lua_gettop(L) == 2 - the second argument exist. Third and next
>> values do not matter.
> 
> I read this as 'those are equivalent' (correct me if I'm wrong). Ok. I'd
> prefer to leave it with ==. Also note the fix I pasted above.

Why? Again. I do not see any reason behind this change except personal
preference. I reverted all the changes about l_load() function, and the
tests passed. So why do we need to make diff bigger?

> 
>>
>>>          if (! lua_istable(L, 2))
>>>             goto usage_error;
>>>          lua_getfield(L, 2, "tag_only");
>>> @@ -794,7 +795,7 @@ error:
>>>    static int l_dump(lua_State *L) {
>>>       struct luaL_serializer *serializer = luaL_checkserializer(L);
>>>       int top = lua_gettop(L);
>>> -   if (top > 2) {
>>> +   if (!(top == 1 || top == 2)) {
>>
>> 3. Here my reasoning is the same - the previous checking works
>> as well.
> 
> It will not give an error in case of yaml.encode() and yaml.encode({},
> {}, {}).

Decent. Here you are right.

> 
>>
>>>    usage_error:
>>>          return luaL_error(L, "Usage: encode(object, {tag_prefix = <string>, "\
>>>                            "tag_handle = <string>})");
>>>

My diff, which reverts some changes and makes this patch one-liner:

diff --git a/third_party/lua-yaml/lyaml.cc b/third_party/lua-yaml/lyaml.cc
index 354cafe86..854794dd1 100644
--- a/third_party/lua-yaml/lyaml.cc
+++ b/third_party/lua-yaml/lyaml.cc
@@ -400,8 +400,7 @@ static void load(struct lua_yaml_loader *loader) {
   */
  static int l_load(lua_State *L) {
     struct lua_yaml_loader loader;
-   int top = lua_gettop(L);
-   if (!(top == 1 || top == 2) || !lua_isstring(L, 1)) {
+   if (! lua_isstring(L, 1)) {
  usage_error:
        return luaL_error(L, "Usage: yaml.decode(document, "\
                          "[{tag_only = boolean}])");
@@ -417,7 +416,7 @@ usage_error:
        return luaL_error(L, OOM_ERRMSG);
     yaml_parser_set_input_string(&loader.parser, (yaml_char_t *) document, len);
     bool tag_only;
-   if (lua_gettop(L) == 2 && ! lua_isnil(L, 2)) {
+   if (lua_gettop(L) > 1) {
        if (! lua_istable(L, 2))
           goto usage_error;
        lua_getfield(L, 2, "tag_only");