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

[Sergey Kaplun <skaplun@tarantool.org>]

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