From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
To: tarantool-patches@freelists.org, imeevma@tarantool.org
Subject: [tarantool-patches] Re: [PATCH v9 4/7] lua: remove exceptions from function luaL_tofield()
Date: Wed, 27 Mar 2019 00:48:32 +0300 [thread overview]
Message-ID: <c8985ddc-53d6-f824-db03-b8fa5f0f711c@tarantool.org> (raw)
In-Reply-To: <e3395eb44aee7ce47c0191291708c585de596c6a.1553251042.git.imeevma@gmail.com>
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.
next prev parent reply other threads:[~2019-03-26 21:48 UTC|newest]
Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-03-22 10:50 [tarantool-patches] [PATCH v9 0/7] sql: remove box.sql.execute imeevma
2019-03-22 10:50 ` [tarantool-patches] [PATCH v9 1/7] sql: add column name to SQL change counter imeevma
2019-03-22 15:42 ` [tarantool-patches] " Konstantin Osipov
2019-03-25 19:34 ` Mergen Imeev
2019-03-29 12:00 ` Kirill Yukhin
2019-03-22 10:50 ` [tarantool-patches] [PATCH v9 2/7] sql: fix error code for SQL errors in execute.c imeevma
2019-03-22 15:45 ` [tarantool-patches] " Konstantin Osipov
2019-03-26 21:48 ` Vladislav Shpilevoy
2019-03-27 11:43 ` Konstantin Osipov
2019-03-28 17:46 ` Mergen Imeev
2019-03-29 12:01 ` Kirill Yukhin
2019-03-22 10:50 ` [tarantool-patches] [PATCH v9 3/7] sql: remove box.sql.debug() imeevma
2019-03-22 15:46 ` [tarantool-patches] " Konstantin Osipov
2019-03-25 19:39 ` Mergen Imeev
2019-03-26 21:48 ` Vladislav Shpilevoy
2019-03-28 17:48 ` Mergen Imeev
2019-03-28 18:01 ` Vladislav Shpilevoy
2019-03-29 12:02 ` Kirill Yukhin
2019-03-22 10:50 ` [tarantool-patches] [PATCH v9 4/7] lua: remove exceptions from function luaL_tofield() imeevma
2019-03-22 15:53 ` [tarantool-patches] " Konstantin Osipov
2019-03-29 19:26 ` Vladislav Shpilevoy
2019-03-26 21:48 ` Vladislav Shpilevoy [this message]
2019-03-28 17:54 ` Mergen Imeev
2019-03-28 18:40 ` Vladislav Shpilevoy
2019-03-28 19:56 ` Mergen Imeev
2019-03-28 21:41 ` Mergen Imeev
2019-03-29 21:06 ` Vladislav Shpilevoy
2019-03-22 10:50 ` [tarantool-patches] [PATCH v9 5/7] iproto: create port_sql imeevma
2019-03-22 15:55 ` [tarantool-patches] " Konstantin Osipov
2019-03-22 10:50 ` [tarantool-patches] [PATCH v9 6/7] sql: create box.execute() imeevma
2019-03-22 15:57 ` [tarantool-patches] " Konstantin Osipov
2019-03-22 10:50 ` [tarantool-patches] [PATCH v9 7/7] sql: remove box.sql.execute() imeevma
2019-03-26 21:48 ` [tarantool-patches] " Vladislav Shpilevoy
2019-03-28 20:13 ` Mergen Imeev
2019-03-29 21:06 ` Vladislav Shpilevoy
2019-03-29 21:07 ` [tarantool-patches] Re: [PATCH v9 0/7] sql: remove box.sql.execute Vladislav Shpilevoy
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=c8985ddc-53d6-f824-db03-b8fa5f0f711c@tarantool.org \
--to=v.shpilevoy@tarantool.org \
--cc=imeevma@tarantool.org \
--cc=tarantool-patches@freelists.org \
--subject='[tarantool-patches] Re: [PATCH v9 4/7] lua: remove exceptions from function luaL_tofield()' \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox