[tarantool-patches] Re: [PATCH v9 4/7] lua: remove exceptions from function luaL_tofield()

[Vladislav Shpilevoy]

Thanks for the patch!

>> 2. Looking at lua_field_inspect_table I found that if a table has __serialize
>> metamethod, it is called without a protection (utils.c:409). __serialize is an
>> arbitrary unprotected user code, that can throw an error deliberately. What are
>> we gonna do with that? Personally I've already faced with some user code, throwing
>> an error from __serialize, deliberately. On not critical errors not meaning panic.
>>
>> As a solution we could 1) do not care, again; 2) finally accept the fact that
>> wrap into a pcall was not so bad and use it; 3) use lua_pcall in that
>> particular place. Please, consult Kostja, what does he choose.
> 
> I checked it this way:
> 
> diff --git a/src/lua/utils.c b/src/lua/utils.c
> index bbfc549..eae3b5f 100644
> --- a/src/lua/utils.c
> +++ b/src/lua/utils.c
> @@ -398,6 +398,20 @@ lua_field_inspect_ucdata(struct lua_State *L, struct luaL_serializer *cfg,
>  	lua_settop(L, top); /* remove temporary objects */
>  }
>  
> +/**
> + * Check that the field LUAL_SERIALIZE of the metatable is
> + * available.
> + *
> + * @param L Lua State.
> + */
> +static int
> +get_metafield_serialize(struct lua_State *L)
> +{
> +	int idx = *(int *)lua_topointer(L, -1);
> +	luaL_getmetafield(L, idx, LUAL_SERIALIZE);
> +	return 0;
> +}
> +
>  static int
>  lua_field_inspect_table(struct lua_State *L, struct luaL_serializer *cfg,
>  			int idx, struct luaL_field *field)
> @@ -408,6 +422,9 @@ lua_field_inspect_table(struct lua_State *L, struct luaL_serializer *cfg,
>  	uint32_t max = 0;
>  
>  	/* Try to get field LUAL_SERIALIZER_TYPE from metatable */
> +	if (!cfg->encode_load_metatables &&
> +	    luaT_cpcall(L, get_metafield_serialize, &idx) != 0)
> +		return -1;
>  	if (!cfg->encode_load_metatables ||
>  	    !luaL_getmetafield(L, idx, LUAL_SERIALIZE))
>  		goto skip;
> 

Unfortunately, it was not the place I told about. You
wrapped just a fetch of __serialize function, but not its
call. It is still unsafe on line 435. In other words it was:

    1 func = obj.__serialize
    2 func(obj)

You made it:

    1 func = pcall(function() obj.__serialize end)
    2 func(obj)

The second line is still unsafe. And much more unsafe than
the first one.

But it is a good catch too, because a user could define
such an __index for his metatable, that it throws an error.
You should wrap both __serialize fetch and call into a single
lua_cpcall.