Tarantool development patches archive
 help / color / mirror / Atom feed
From: "sergos@tarantool.org" <sergos@tarantool.org>
To: Sergey Kaplun <skaplun@tarantool.org>
Cc: Alexander Turenko <alexander.turenko@tarantool.org>,
	tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH 2/2] msgpack: fix wrong mp_ext type in error message
Date: Fri, 17 Jul 2020 12:23:53 +0300	[thread overview]
Message-ID: <232C7A81-4104-4DBE-8985-3CBBDB007CC3@tarantool.org> (raw)
In-Reply-To: <20200717083521.GA3979@root>

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

  parent reply	other threads:[~2020-07-17  9:23 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-15 15:56 [Tarantool-patches] [PATCH 0/2] Msgpack wrong extension " Sergey Kaplun
2020-06-15 15:56 ` [Tarantool-patches] [PATCH 1/2] lib: update msgpuck library Sergey Kaplun
2020-06-15 15:56 ` [Tarantool-patches] [PATCH 2/2] msgpack: fix wrong mp_ext type in error message Sergey Kaplun
2020-07-14 15:46   ` Igor Munkin
2020-07-15  9:32     ` Sergey Kaplun
2020-07-15 15:21       ` Sergey Kaplun
2020-07-16 20:53       ` Igor Munkin
2020-07-17  8:35         ` Sergey Kaplun
2020-07-17  9:09           ` Alexander Turenko
2020-07-17  9:54             ` Sergey Kaplun
2020-07-17  9:23           ` sergos [this message]
2020-07-17 10:12           ` Igor Munkin
2020-07-17 11:02             ` Sergey Kaplun
2020-07-17 11:27 ` [Tarantool-patches] [PATCH 0/2] Msgpack wrong extension " Kirill Yukhin

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=232C7A81-4104-4DBE-8985-3CBBDB007CC3@tarantool.org \
    --to=sergos@tarantool.org \
    --cc=alexander.turenko@tarantool.org \
    --cc=skaplun@tarantool.org \
    --cc=tarantool-patches@dev.tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH 2/2] msgpack: fix wrong mp_ext type in error message' \
    /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