[tarantool-patches] Re: [PATCH v2 1/3] lua-yaml: verify arguments count
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Tue Feb 5 22:36:41 MSK 2019
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");
More information about the Tarantool-patches
mailing list