Re: [Tarantool-patches] [PATCH] serializer: check for recursive serialization

[Igor Munkin <imun@tarantool.org>]

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.

> 
> 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
> 

<snipped>

> 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.

> + | ...

<snipped>

> -- 
> 2.24.3 (Apple Git-128)
> 

-- 
Best regards,
IM