[Tarantool-patches] [PATCH 2/2] msgpack: fix wrong mp_ext type in error message
Sergey Kaplun
skaplun at tarantool.org
Wed Jul 15 12:32:15 MSK 2020
Hi Igor! Thanks for the review!
On 14.07.20, Igor Munkin wrote:
> Sergey,
>
> Thanks for the patch! Please consider my comments below.
>
> On 15.06.20, Sergey Kaplun wrote:
> > This patch adds decoding of mp_ext type and inserts it into error
> > message when an error is raised
>
> But what was the problem? I can read various binary sequences, but I'm
> still not good at MessagePack. Could you please describe what exactly
> was wrong prior to your fix?
Well, I had to provide more detailed desciption in commit message.
Our msgpack library doesn't support deconding/encoding of ext types
until commit 8ae606 (00d770 commit adds this functionality in
tarantool). As you can see from issue [1] \xd4 or \xd5 are leading to
the error at decoding. Nevertheless they are valid msgpack keys that
mean extension format family [2]. User should see a type of
unsupported extension as 15 (\xf0).
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.
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.
> >
> > Closes #5017
> > ---
> > src/lua/msgpack.c | 5 +++--
> > test/unit/CMakeLists.txt | 5 +++++
> > test/unit/mplua.c | 47 ++++++++++++++++++++++++++++++++++++++++
> > test/unit/mplua.result | 5 +++++
> > 4 files changed, 60 insertions(+), 2 deletions(-)
> > create mode 100644 test/unit/mplua.c
> > create mode 100644 test/unit/mplua.result
> >
>
> <snipped>
>
> > diff --git a/test/unit/CMakeLists.txt b/test/unit/CMakeLists.txt
> > index 6dfb10f46..906422e82 100644
> > --- a/test/unit/CMakeLists.txt
> > +++ b/test/unit/CMakeLists.txt
> > @@ -205,3 +205,8 @@ target_link_libraries(coll.test core unit ${ICU_LIBRARIES} misc)
> >
> > add_executable(tuple_bigref.test tuple_bigref.c)
> > target_link_libraries(tuple_bigref.test tuple unit)
> > +
> > +add_executable(mplua.test mplua.c)
> > +target_link_libraries(mplua.test unit box server core
> > + ${CURL_LIBRARIES} ${LIBYAML_LIBRARIES} ${READLINE_LIBRARIES}
> > + ${ICU_LIBRARIES} ${LUAJIT_LIBRARIES})
>
> Why should this be done in such complex way? Do your test binary need
> *all* mentioned libs? I see no usage for libcurl, libreadline, libicu,
> libyaml in the test. Furthermore, IIRC LuaJIT symbols are provided by
> libserver archive.
When I had tried to build my test I found out that lua/msgpack.c requires build
with tarantool_lua_ibuf (from lua/utils.c).
| /usr/lib/gcc/x86_64-pc-linux-gnu/9.2.0/../../../../x86_64-pc-linux-gnu/bin/ld:
| CMakeFiles/mplua.test.dir/__/__/src/lua/msgpack.c.o: in function
| `lua_msgpack_encode': msgpack.c:(.text+0x1673): undefined reference to
| `tarantool_lua_ibuf'
Building utils requires build with httpc_lua (from lua/httpc.c). And it
requires curl library.
| /usr/lib/gcc/x86_64-pc-linux-gnu/9.2.0/../../../../x86_64-pc-linux-gnu/bin/ld:
| CMakeFiles/mplua.test.dir/__/__/src/lua/init.c.o:(.data.rel+0x1b8): undefined
| reference to `httpc_lua'
And so on. I suppose that building with libserver and its linkage with
necessary libraries would be less bulky than list all necessary sources
inside add_executable. It would be nice if you will offer a prettier
approach.
Unfortunately libserver doesn't provides necessary symbols, but requires it to
be built.
| nm -g src/libserver.a | grep lua_type | sort | uniq
| U lua_type
| U lua_typename
>
> Well, I faced the following linkage error when the test is build in
> Release mode:
> | [ 87%] Linking CXX executable mplua.test
> | /usr/lib/gcc/x86_64-pc-linux-gnu/8.3.0/../../../../x86_64-pc-linux-gnu/bin/ld: ../../src/libcore.a(evio.cc.o): in function `evio_service_bind(evio_service*, char const*)':
> | evio.cc:(.text+0x9e2): undefined reference to `uri_parse'
> | collect2: error: ld returned 1 exit status
Hmm, I build successfully with Debug/RelWithDebInfo/Release mode
locally. How did you get this error? Did you build from my branch or
applied diff to master?
> > diff --git a/test/unit/mplua.c b/test/unit/mplua.c
> > new file mode 100644
> > index 000000000..3b7f4de76
> > --- /dev/null
> > +++ b/test/unit/mplua.c
> > @@ -0,0 +1,47 @@
> > +#include <lua.h> /* lua_*() */
> > +#include <lauxlib.h> /* luaL_*() */
>
> Minor: Since we use bundled LuaJIT you need to lookup "local" headers
> prior to the system-wide ones[1]. But I've run make with more verbose
> output and seen we don't use "-I-" in preprocessor flags[2], so I see no
> difference how to specify such includes (IMHO it's not the proper way):
> | [100%] Building C object test/unit/CMakeFiles/mplua.test.dir/mplua.c.o
> | cd /tarantool/test/unit && /usr/bin/cc -DCORO_ASM
> | -DLUAJIT_SMART_STRINGS=1 -DLUA_USE_APICHECK=1 -DLUA_USE_ASSERT=1
> | -DNVALGRIND=1 -DYAML_DECLARE_STATIC -D_FILE_OFFSET_BITS=64 -D_GNU_SOURCE
> | -D__STDC_CONSTANT_MACROS=1 -D__STDC_FORMAT_MACROS=1
> | -D__STDC_LIMIT_MACROS=1 -I/tarantool/src -I/tarantool/src/lib
> | -I/tarantool/src/lib/small -I/tarantool/src/lib/small/third_party
> | -I/tarantool -I/tarantool/third_party/zstd/lib
> | -I/tarantool/third_party/zstd/lib/common
> | -I/tarantool/third_party/luajit/src -I/tarantool/src/lib/msgpuck
> | -I/tarantool/src/box -I/tarantool/third_party
> | -I/tarantool/build/curl/dest/include
> | -I/tarantool/third_party/libyaml/include -fexceptions -funwind-tables
> | -fno-omit-frame-pointer -fno-stack-protector -fno-common -fopenmp -msse2
> | -std=c11 -Wall -Wextra -Wno-strict-aliasing -Wno-char-subscripts
> | -Wno-format-truncation -fno-gnu89-inline -Wno-cast-function-type -Werror
> | -Wno-unused-parameter -Wno-unused -Wno-unused-result
> | -Wno-tautological-compare -g -ggdb -O0 -o
> | CMakeFiles/mplua.test.dir/mplua.c.o -c /tarantool/test/unit/mplua.c
It's very important comment! Of course I should use "" here. Thanks!
> > +#include <lualib.h> /* luaL_openlibs() */
>
> This include is excess.
Thanks! Nice catch!
> > +#include <string.h> /* memcmp() */
<snipped>
> > + const char test_str[] = "\xd4\x0f\x00";
> > + const char *data = (char *)test_str;
>
> Why do you create a distinct pointer for to be passed to <luamp_decode>?
> You can just properly declare <test_str>:
> | const char *test_str = "\xd4\x0f\00";
>
Ok! Sure!
> > + luamp_decode(L, luaL_msgpack_default, &data);
> > + return 0;
> > +}
> > +
> > +
> > +static void
> > +test_luamp_encode_extension_default(struct lua_State *L)
> > +{
> > + plan(2);
> > + lua_pushcfunction(L, lua_encode_unknown_mp);
> > + ok(lua_pcall(L, 0, 0, 0) != 0,
>
> You can check that exactly <LUA_ERRRUN> is yielded here.
Thanks! Good idea!
> > + "mplua_decode: unsupported extension raise error");
>
> You can simply use <lua_cpcall> here.
Ok! Thanks!
> > + const char err_msg[] = "msgpack.decode: unsupported extension: 15";
> > + ok(memcmp(lua_tostring(L, -1), err_msg, sizeof(err_msg)) == 0,
> > + "mplua_decode: unsupported extension correct type");
> > +}
> > +
> > +
>
> Well, it looks like you've mixed several approaches and it led to the
> following mess:
> * You set test plan right in the test case, but check it in the <main>
> routine. I guess these calls should be placed within the same
> function, otherwise they are hard maintained.
> * There are lots of tests using header/footer but no one except
> <base64.c> uses it right in <main>. I don't know, what these macros
> are for, but most of the tests use them in the test case.
>
> The test case is really small and I guess you can move everything right
> to the <main> as it's done in rlist.c
Ok, I will move body of the test to the <main> directly.
> Besides, the main question is why do you use C test instead of writing a
> small Lua test (similar to the one in the issue)? At least you can avoid
> all linkage issues.
As I say above this error is hidden from lua at tarantool >= 2.4.1. So
I suppose it would be nice to have one test to 1.10 and 2.4.1.
> > +int
> > +main(void)
> > +{
> > + header();
> > +
> > + struct lua_State *L = luaL_newstate();
> > + test_luamp_encode_extension_default(L);
> > +
> > + footer();
> > + check_plan();
>
> The function should return <int> but returns nothing. All other tests
> have the following idiom:
> | return check_plan();
>
It is correct if we are talking about master. But at 1.10 AFAIS test
adhere to other way (I use for example test/unit/tuple_bigref.c as one
of the oldest). Should I use approach from master here or I should use
old code style here?
> > +}
>
> <snipped>
>
> > --
> > 2.24.1
> >
>
> [1]: https://gcc.gnu.org/onlinedocs/gcc-10.1.0/cpp/Include-Syntax.html#Include-Syntax
> [2]: https://gcc.gnu.org/onlinedocs/gcc-10.1.0/cpp/Invocation.html#Invocation
>
> --
> Best regards,
> IM
[1]: https://github.com/tarantool/tarantool/issues/5017
[2]: https://github.com/msgpack/msgpack/blob/master/spec.md#ext-format-family
--
Best regards,
Sergey Kaplun
More information about the Tarantool-patches
mailing list