Re: [Tarantool-patches] [PATCH 2/2] msgpack: fix wrong mp_ext type in error message

["sergos@tarantool.org" <sergos@tarantool.org>]

Hi!

Thanks for the new patch, LGTM.

I support Alexander Turenko in his contribution path: master + all supported
versions.

Sergos


> On 17 Jul 2020, at 11:35, Sergey Kaplun <skaplun@tarantool.org> wrote:
> 
> Igor,
> 
> On 16.07.20, Igor Munkin wrote:
>> Sergey,
>> 
>> On 15.07.20, Sergey Kaplun wrote:
> 
> <snipped>
> 
>>> 
>>> This bug is hidden at master because since 2.4.1 (commit 345877) we have
>>> additional error handler (luamp_decode_extension_box) that correctly decode
>>> extension type.
>> 
>> So it's not hidden, but handled and the issue doesn't occur since 2.4.1.
> 
> AFAIK, yes.
> 
>> 
>>> 
>>> The main idea of the patch is to cherry-pick commits e476ad1 and 8ae606a
>>> to separate msgpack branch for tarantool-1.10 (only them because we
>>> don't want to add a lot of commits to LTS version). Also we should
>>> cherry-pick commit 00d770 to latest tarantool 1.10 and after that add
>>> corresponding decoding of extension type. For versions higher than 1.10
>>> (IIRC) we can just cherry-pick this patch and a test.
>> 
>> As you mentioned above, the issue is gone (or masked, whatever) in all
>> versions above 2.4.1. AFAIR everything except these versions and LTS
>> will be deprecated in a few days. So you need to fix the issue only for
>> 1.10 and nothing to be cherry-picked, right?
> 
> Hmm, I suppose that it will be enough to apply this patch only to 1.10.
> In that way we don't need C-implemented test.
> 
>> 
> <snipped>
>> 
>> My bad. I tried to reduce the amount of the required libs myself and get
>> the list quite similar to the one you've mentioned within your next
>> reply. Does it successfully work for all build modes?
> 
> Yes, it does.
> 
> <snipped>
> 
>> 
>> Well, I still don't get the reason for it. You can simply check your
>> patch via a tiny Lua chunk for 1.10, otherwise the patch and the test
>> are not necessary for the reasons I mentioned above.
> 
> I rewrote test to simple lua chunk (see it below).
> 
>> 
>> However, if other reviewers insist on the test implemented in C, please
>> adjust it considering the comments I left in my previous reply.
>> 
> 
> <snipped>
> 
>> 
>> -- 
>> Best regards,
>> IM
> 
> msgpack: fix wrong mp_ext type in an error message
> 
> While decoding msgpack's extension tarantool raises error as far as it
> doesn't support extension types. Error contains message about wrong
> extension type with wrong byte (the first instead of the second).
> 
> This patch adds decoding of mp_ext type and inserts it into error
> message when error is raised.
> 
> Closes #5017
> ---
> src/lua/msgpack.c             | 5 +++--
> test/app-tap/msgpack.test.lua | 5 ++++-
> 2 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/src/lua/msgpack.c b/src/lua/msgpack.c
> index b7a2955fe..e3935d383 100644
> --- a/src/lua/msgpack.c
> +++ b/src/lua/msgpack.c
> @@ -224,8 +224,9 @@ luamp_set_encode_extension(luamp_encode_extension_f handler)
> static void
> luamp_decode_extension_default(struct lua_State *L, const char **data)
> {
> -	luaL_error(L, "msgpack.decode: unsupported extension: %u",
> -		   (unsigned char) **data);
> +	int8_t ext_type;
> +	mp_decode_extl(data, &ext_type);
> +	luaL_error(L, "msgpack.decode: unsupported extension: %u", ext_type);
> }
> 
> 
> diff --git a/test/app-tap/msgpack.test.lua b/test/app-tap/msgpack.test.lua
> index 6f7b46a15..1f3904837 100755
> --- a/test/app-tap/msgpack.test.lua
> +++ b/test/app-tap/msgpack.test.lua
> @@ -36,7 +36,7 @@ local function test_offsets(test, s)
> end
> 
> local function test_misc(test, s)
> -    test:plan(4)
> +    test:plan(5)
>     local ffi = require('ffi')
>     local buffer = require('buffer')
>     local buf = ffi.cast("const char *", "\x91\x01")
> @@ -47,6 +47,9 @@ local function test_misc(test, s)
>     test:is(buf + 2, bufend, 'ibuf_decode position')
>     test:is_deeply(result, {1}, "ibuf_decode result")
>     test:ok(not st and e:match("null"), "null ibuf")
> +    st, e = pcall(s.decode, "\xd4\x0f\x00")
> +    test:ok(not st and e:match("unsupported extension: 15"),
> +                               "unsupported extension decode")
> end
> 
> tap.test("msgpack", function(test)
> -- 
> 2.24.1
> 
> -- 
> Best regards,
> Sergey Kaplun