Re: [Tarantool-patches] [PATCH 1/1] box: export box_session_push to the public C API

[Alexander Turenko <alexander.turenko@tarantool.org>]

Thanks for the patch!

I run the following code:

 | cd test/box
 | ../../src/tarantool
 | tarantool> box.cfg{}
 | tarantool> box.schema.func.create('function1.test_push', {language = 'C'})
 | tarantool> box.func['function1.test_push']:call({1,2,3,4,5,6})

And got segmentation fault:

#0  0x55fd20b2b789 in print_backtrace+9
#1  0x55fd209dd2c2 in _ZL12sig_fatal_cbiP9siginfo_tPv+1e7
#2  0x7efce59603a0 in funlockfile+80
#3  (nil) in +80
#4  0x55fd20ae6e54 in console_session_push+79
#5  0x55fd20aac23a in session_push+34
#6  0x55fd20ab1bdb in box_session_push+6e
#7  0x7efce7d941cf in test_push+27
#8  0x55fd20a84bdb in func_c_call+181
#9  0x55fd20a85011 in func_call+e9
#10 0x55fd20ae4c60 in lbox_func_call+20b
#11 0x55fd20b5b45b in lj_BC_FUNCC+34
#12 0x55fd20b7e23f in lua_pcall+18b
#13 0x55fd20b0de3a in luaT_call+29
#14 0x55fd20b04be2 in lua_main+b9
#15 0x55fd20b05214 in run_script_f+619
#16 0x55fd209dcbc9 in _ZL16fiber_cxx_invokePFiP13__va_list_tagES0_+1e
#17 0x55fd20b27bbf in fiber_loop+82
#18 0x55fd20d352ef in coro_init+4c

I would expect the same output as from `box.session.push(<...>)` call in
repl: a yaml document with a tag.

I see that box_return_tuple() receives (box_function_ctx_t *) as the
first argument and wonder why box_session_push() does not. If we able to
determine where to and how to send a push w/o the context, then the
context is not necessary in box_return_tuple() too? I don't very
familiar with this part of codebase, so it is just question for now.
I'll look around soon if time permits.

We also discussed with Vlad whether box_session_push() should validate
passed msgpack. I think that it is usual for a C API (in general) to
don't perform full scan of data from a user provided function (when it
is C function, not something from, say, socket). So in my understanding
passing of incorrect msgpack should be considered as the API misusage
and should not be checked.

A client may determine where the incorrect iproto packet ends, handle
incorrect msgpack somehow and continue reading from the connection. Or
expect that this situation will not occur due to performance reasons
(which may be acceptable, say, when both client and server procedures
are developed in tight).

In this case incorrect iproto packet will be sent to a client, but it is
possible already with box_return_tuple() in RelWithDebInfo build (Debug
build however will fail on mp_tuple_assert(), called from
runtime_tuple_new()).

I checked it in this way:

 | const char *data = "\x93\x04\x05\x06\x07";
 | box_tuple_format_t *fmt = box_tuple_format_default();
 | box_tuple_t *tuple = box_tuple_new(fmt, data, data + 5);
 | return box_return_tuple(ctx, tuple);

Moreover, our C API is used for cases where performance is vital. So
extra traversal over msgpack data looks inappropriate here.

WBR, Alexander Turenko.

On Mon, Jan 20, 2020 at 11:16:05PM +0100, Vladislav Shpilevoy wrote:
> It was a customer request. Nothing special here, the patch is
> trivial, with a couple of short notes:
> 
> 1) Sync argument was removed. It will disappear from Lua API as
>   well, according to #4689. Port is replaced with raw MessagePack,
>   because port is not a part of public API. Session is omitted as
>   well. Indeed, a user can't push to a foreign session, and the
>   current session can be obtained inside box_session_push(). And
>   anyway session is not in the public C API.
> 
> 2) Dump is done using obuf_dup(), just like tuple_to_obuf() does.
>   obuf_alloc() would be a bad call here, because it wouldn't be
>   able to split the pushed data into several obuf chunks, and
>   would cause obuf fragmentation.
> 
> Closes #4734
> ---
> Issue: https://github.com/tarantool/tarantool/issues/4734
> Branch: https://github.com/tarantool/tarantool/tree/gerold103/gh-4734-export-box-session-push
> 
>  extra/exports               |  1 +
>  src/box/box.cc              | 12 +++++++++++
>  src/box/box.h               | 14 +++++++++++++
>  src/box/call.c              | 14 ++++++++++++-
>  test/box/function1.c        |  7 +++++++
>  test/box/function1.result   | 41 +++++++++++++++++++++++++++++++++++++
>  test/box/function1.test.lua | 19 +++++++++++++++++
>  7 files changed, 107 insertions(+), 1 deletion(-)