From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id C53B425896 for ; Tue, 29 Jan 2019 15:42:30 -0500 (EST) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id a-ps0dSXrSvA for ; Tue, 29 Jan 2019 15:42:30 -0500 (EST) Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 7C6EA255B1 for ; Tue, 29 Jan 2019 15:42:30 -0500 (EST) Subject: [tarantool-patches] Re: [PATCH v8 1/6] lua: remove exceptions from function luaL_tofield() References: From: Vladislav Shpilevoy Message-ID: Date: Tue, 29 Jan 2019 23:42:28 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org, imeevma@tarantool.org Hi! Thanks for the patch! See 2 comments below. > @@ -525,94 +533,95 @@ luaL_tofield(struct lua_State *L, struct luaL_serializer *cfg, int index, > case LUA_TBOOLEAN: > field->type = MP_BOOL; > field->bval = lua_toboolean(L, index); > - return; > + return 0; > case LUA_TNIL: > field->type = MP_NIL; > - return; > + return 0; > case LUA_TSTRING: > field->sval.data = lua_tolstring(L, index, &size); > field->sval.len = (uint32_t) size; > field->type = MP_STR; > - return; > + return 0; > case LUA_TTABLE: > { > field->compact = false; > - lua_field_inspect_table(L, cfg, index, field); > - return; > + if (lua_field_inspect_table(L, cfg, index, field) < 0) > + return -1; > + return 0; 1. Why not simply 'return lua_field_inspect_table' ? > } > case LUA_TLIGHTUSERDATA: > case LUA_TUSERDATA: > diff --git a/src/lua/utils.h b/src/lua/utils.h > index a47e3d2..fde3514 100644 > --- a/src/lua/utils.h > +++ b/src/lua/utils.h > @@ -345,7 +348,8 @@ static inline void > luaL_checkfield(struct lua_State *L, struct luaL_serializer *cfg, int idx, > struct luaL_field *field) > { > - luaL_tofield(L, cfg, idx, field); > + if (luaL_tofield(L, cfg, idx, field) < 0) > + luaT_error(L); > if (field->type != MP_EXT) > return; > luaL_convertfield(L, cfg, idx, field); > -- > 2.7.4 > > 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.