From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp29.i.mail.ru (smtp29.i.mail.ru [94.100.177.89]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 07EF8469710 for ; Tue, 24 Nov 2020 04:51:10 +0300 (MSK) References: <20201117164012.14652-1-roman.habibov@tarantool.org> <20201123202841.GC14086@tarantool.org> From: roman Message-ID: Date: Tue, 24 Nov 2020 04:51:09 +0300 MIME-Version: 1.0 In-Reply-To: <20201123202841.GC14086@tarantool.org> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit Content-Language: en-US Subject: Re: [Tarantool-patches] [PATCH] serializer: check for recursive serialization List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Munkin Cc: tarantool-patches@dev.tarantool.org, alexander.turenko@tarantool.org Hi! Thanks for the review. On 23.11.2020 23:28, Igor Munkin wrote: > Roma, > > Thanks for the patch! This version looks much better than the previous one, > but I still have a couple of nits. Otherwise LGTM. > > On 17.11.20, Roman Khabibov wrote: >> Print error if object after serialization is the same. > I believe we need a doc request to update __serialize description, since > its behaviour is restricted with the introduced constraint now. Done. >> Closes #3228 >> --- >> >> Branch: https://github.com/tarantool/tarantool/tree/romanhabibov/serialize-check >> Issue: https://github.com/tarantool/tarantool/issues/3228 >> >> @ChangeLog: >> * Fix bug with bus error when __serialize function generates infinite recursion (gh-3228). >> >> src/lua/utils.c | 5 +++++ >> ...-3228-serializer-look-for-recursion.result | 19 +++++++++++++++++++ >> ...228-serializer-look-for-recursion.test.lua | 8 ++++++++ >> 3 files changed, 32 insertions(+) >> create mode 100644 test/app/gh-3228-serializer-look-for-recursion.result >> create mode 100644 test/app/gh-3228-serializer-look-for-recursion.test.lua >> > > >> diff --git a/test/app/gh-3228-serializer-look-for-recursion.result b/test/app/gh-3228-serializer-look-for-recursion.result >> new file mode 100644 >> index 000000000..cd86ab06a >> --- /dev/null >> +++ b/test/app/gh-3228-serializer-look-for-recursion.result >> @@ -0,0 +1,19 @@ >> +-- test-run result file version 2 >> +test_run = require('test_run').new() >> + | --- >> + | ... >> + >> +-- >> +-- gh-3228: Check the error message in the case of a __serialize >> +-- function generating infinite recursion. >> +-- >> +setmetatable({}, {__serialize = function(a) return a end}) >> + | --- >> + | - error: 'console: an exception occurred when formatting the output: Bad __serialize >> + | function. It can''t return the same value.' >> + | ... >> +setmetatable({}, {__serialize = function(a, b, c) return a, b, c end}) >> + | --- >> + | - error: 'console: an exception occurred when formatting the output: Bad __serialize >> + | function. It can''t return the same value.' > Hm, AFAICS the custom serializer accepts a single argument (i.e. "self") > and a single return value is expected (considering the code you were > around to). Hence, the latter check is the same as the first one and > checks literally nothing. By the way, I guess it's worth to check that > __eq metamethod is ignored when the object itself is compared with its > "serialized" value. Just to be sure it won't be broken unintentionally > in future. Added. >> + | ... > > >> -- >> 2.24.3 (Apple Git-128) >> commit a40f5623b31e547cc0c273f2224484a591002e31 Author: Roman Khabibov Date:   Thu Oct 8 18:22:24 2020 +0300     serializer: check for recursive serialization     Print error if object after serialization is the same.     Closes #3228     @TarantoolBot documnet     Title: __serialize parameter     If __serialize parameter is function, then this function     can't return the value passed to it. Such functions     generates recursions, so this is forbidden.     Example:     ```     tarantool> setmetatable({},{__serialize = function(_) return _ end})     ---     - error: 'console: an exception occurred when formatting the output: Bad __serialize         function. It can''t return the same value.'     ...     ``` diff --git a/src/lua/utils.c b/src/lua/utils.c index 23fbdd4..d12f367 100644 --- a/src/lua/utils.c +++ b/src/lua/utils.c @@ -508,6 +508,11 @@ lua_field_try_serialize(struct lua_State *L, struct luaL_serializer *cfg,              diag_set(LuajitError, lua_tostring(L, -1));              return -1;          } +        if (lua_rawequal(L, -2, -1) == 1) { +            diag_set(LuajitError, "Bad __serialize function. It " +                 "can't return the same value."); +            return -1; +        }          if (luaL_tofield(L, cfg, NULL, -1, field) != 0)              return -1;          lua_replace(L, idx); diff --git a/test/app/gh-3228-serializer-look-for-recursion.result b/test/app/gh-3228-serializer-look-for-recursion.result new file mode 100644 index 0000000..aaa3a4f --- /dev/null +++ b/test/app/gh-3228-serializer-look-for-recursion.result @@ -0,0 +1,26 @@ +-- test-run result file version 2 +test_run = require('test_run').new() + | --- + | ... + +-- +-- gh-3228: Check the error message in the case of a __serialize +-- function generating infinite recursion. +-- +setmetatable({}, {__serialize = function(a) return a end}) + | --- + | - error: 'console: an exception occurred when formatting the output: Bad __serialize + |     function. It can''t return the same value.' + | ... + +-- +--Check that __eq metamethod is ignored. +-- +local table = setmetatable({}, {__eq = function(a, b) return a ~= b end}) + | --- + | ... +setmetatable(table, {__serialize = function(a) return a end}) + | --- + | - error: 'console: an exception occurred when formatting the output: Bad __serialize + |     function. It can''t return the same value.' + | ... diff --git a/test/app/gh-3228-serializer-look-for-recursion.test.lua b/test/app/gh-3228-serializer-look-for-recursion.test.lua new file mode 100644 index 0000000..2f757af --- /dev/null +++ b/test/app/gh-3228-serializer-look-for-recursion.test.lua @@ -0,0 +1,13 @@ +test_run = require('test_run').new() + +-- +-- gh-3228: Check the error message in the case of a __serialize +-- function generating infinite recursion. +-- +setmetatable({}, {__serialize = function(a) return a end}) + +-- +--Check that __eq metamethod is ignored. +-- +local table = setmetatable({}, {__eq = function(a, b) return a ~= b end}) +setmetatable(table, {__serialize = function(a) return a end})