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

[Sergey Kaplun <skaplun@tarantool.org>]

Igor,

On 17.07.20, Igor Munkin wrote:
> Sergey,
> 
> Thanks for the fixes you made! The patch LGTM now, except a single nit
> regarding the commit message.
> 
> On 17.07.20, Sergey Kaplun wrote:
> > Igor,
> > 
> > On 16.07.20, Igor Munkin wrote:
> > > Sergey,
> > > 
> > > On 15.07.20, Sergey Kaplun wrote:
> 
> <snipped>
> 
> > > > 
> > > > 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.
> 
> I see other reviewers have mentioned in their replies that it's worth to
> apply the patch for all supported versions. Since the output differs,
> please create two different branches to ease the merge procedure: one to
> be applied to LTS branch and another one -- for bleeding and active 2.x
> stable branches. AFAICS, only the error message need to be adjusted.
> 

I added additional branch (see corresponding diff with ChangeLog below).

> > 
> 
> <snipped>
> 
> > 
> > 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.
> 
> I reworded the commit message to make it a bit clearer:
> | Since Tarantool doesn't support msgpack extension types, the error
> | occurs while its decoding. The error message is misleading, since
> | the wrong byte is used for representing the extension type.
> |
> | The patch adds mp_ext type decoding and the correct type value is used
> | in the message when the error is raised.
> 
> Feel free to change it on your own.

Thanks for your comment! I've changed the commit message for both
branches (see it below).

> 
> > 
> > Closes #5017
> > ---
> >  src/lua/msgpack.c             | 5 +++--
> >  test/app-tap/msgpack.test.lua | 5 ++++-
> >  2 files changed, 7 insertions(+), 3 deletions(-)
> > 
> 
> <snipped>
> 
> > -- 
> > 2.24.1
> > 
> > -- 
> > Best regards,
> > Sergey Kaplun
> 
> -- 
> Best regards,
> IM

msgpack: fix wrong mp_ext type in an error message

Since Tarantool doesn't support msgpack extension types, the error
occurs while its decoding by default. The error message is misleading,
since the wrong byte is used for representing the extension type.

The patch adds mp_ext type decoding and the correct type value is used
in the message when the error is raised.

Closes #5017
---

Branch: https://github.com/tarantool/tarantool/tree/skaplun/gh-5017-msgpack-wrong-ext-type-master

@ChangeLog:
 * Fix wrong mpsgpack extension type in an error message at decoding
 (gh-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 d482c9b9a..128cc5d2f 100644
--- a/src/lua/msgpack.c
+++ b/src/lua/msgpack.c
@@ -92,8 +92,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 1df9d2372..2d62850fb 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("Unsuported MsgPack extension type: 15"),
+                               "unsupported extension decode")
 end
 
 local function test_decode_array_map_header(test, s)
-- 
2.24.1

-- 
Best regards,
Sergey Kaplun