From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp57.i.mail.ru (smtp57.i.mail.ru [217.69.128.37]) (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 CE4AF445320 for ; Fri, 17 Jul 2020 11:35:31 +0300 (MSK) Date: Fri, 17 Jul 2020 11:35:21 +0300 From: Sergey Kaplun Message-ID: <20200717083521.GA3979@root> References: <9e6b2ec973a0dd0f2bb6da4915bf1f8f7ff58081.1592235478.git.skaplun@tarantool.org> <20200714154646.GL5559@tarantool.org> <20200715093215.GA947@root> <20200716205331.GP5559@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200716205331.GP5559@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH 2/2] msgpack: fix wrong mp_ext type in error message 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 Igor, On 16.07.20, Igor Munkin wrote: > Sergey, > > On 15.07.20, Sergey Kaplun wrote: > > > > 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. > > > 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. > > 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. > > > -- > 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