Re: [Tarantool-patches] [PATCH] lua: show error on attempt to access to tuple of unsupported type

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

It is still not fully clear for me how it should be fixed, but at least
it becomes more clear what is going on.

I commented every place, where I have thoughts. Sorry, my answer is not
very structured, because I found new things while looking into this and
experimenting around. (And also I want to give some feedback sooner.)

WBR, Alexander Turenko.

On Mon, May 18, 2020 at 05:27:46PM +0300, sergeyb@tarantool.org wrote:
> From: Sergey Bronnikov <sergeyb@tarantool.org>
> 
> There are cases when tarantool may get access to tuples of unsupported type
> added in next versions:

Nit: Strictly speaking, a value in a tuple has an unsupported type, not
the tuple itself.

> - new client inserts decimal value in a space and server unable to decode it.
> - old client select decimal value
> - old client recieve decimal value from a function

Typo: recieve -> receive.

Nit: I would say that in this context 'old client' means net.box in old
tarantool version or just write it as 'net.box' instead of 'old client'.
The problem is common for clients, but here we'll fix it for net.box.

> 
> Patch adds error message shown when cases above are happen.
> 
> Fixes #4632

We discussed the problem in the issue comments, where I propose to hide
unsupported spaces. Then there were private discussions, but we don't
hold reasons to don't hide spaces anywhere: nor in the issue, neither in
the commit message. It will be hard to understand in the future, why one
resolution way was proposed in the issue, but another one is choosen
finally.

I made attempt to clarify my thoughts in [1].

[1]: https://github.com/tarantool/tarantool/issues/4632#issuecomment-632952264

It states that we should fix the problem with creating tuple formats,
otherwise we cannot say that the issue is fixed.

> ---
> GH issue: https://github.com/tarantool/tarantool/issues/4632
> GH branch: https://github.com/tarantool/tarantool/tree/ligurio/gh-4632-warn-unsupported-types
> 
> Note: patch is applicable to 1.10 branch only.
> 
>  src/lua/msgpackffi.lua | 2 ++
>  1 file changed, 2 insertions(+)

I would add unit-like test cases for msgpack/msgpackffi and also (that
looks more important for me) scenarious for box / net.box that we're
interested. You may construct msgpack manually and write it to a space
via binary protocol (see
test/sql-tap/gh-4077-iproto-execute-no-bind.test.lua for example). Then
just use box / net.box and show its behaviour.

But, yep, I don't know how to imitate decimal return value from a
function in such simple way. Maybe it is the time to write a simple mock
for iproto to use in tests? I tried to do it and it is 77 lines on Lua
(see below the email).

(More on this: Such scenarious would greatly simplify review and
understanding your commit in the future.)

My doubts about just updating 'msgpackffi' module (except tuple format
problem that is mentioned above and except details about the
implementation that are described below):

* net.box use 'msgpack' module, not 'msgpackffi' to decode, say, call
  response.
* net.box use mp_next() directly to cut msgpack data for tuples. It does
  not verify mp_ext types and successfully creates a tuple with a value
  of unknown mp_ext type. (Mentioned in [2].)

[2]: https://github.com/tarantool/tarantool/issues/4632#issuecomment-632974789

The latter bullet disquiets me:

1. It is inconsistent with other responses (say, call).
2. But it is consistent with box (it allows to store unknown mp_ext
   values).
3. Accessing to a field from Lua is performed using 'msgpackffi'. But
   other code may expect that msgpack in a tuple is not only
   well-formed, but also perfectly valid. I don't know, to be honest,
   how to better verify it.
4. Even accessing to a field after unknown mp_ext may give an error
   (depends on offset map).

3rd point is important, others are not much. I don't sure whether it is
correct to have such tuples and how it breaks things. What should be
verified? Comparators / key_def module? Anything that touch msgpack
somehow?

It is the question about `struct tuple` contract: should in be
well-formed or valid?

(More on this: It is not obvious from the commit message that an error
is reported on a tuple field access, not at time of parsing a select
response.)

> 
> diff --git a/src/lua/msgpackffi.lua b/src/lua/msgpackffi.lua
> index abcbd54fa..4c60523e6 100644
> --- a/src/lua/msgpackffi.lua
> +++ b/src/lua/msgpackffi.lua
> @@ -541,6 +541,8 @@ decode_r = function(data)
>          return false
>      elseif c == 0xc3 then
>          return true
> +    elseif c >= 0xd4 and c <= 0xd8 or c >= 0xc7 and c <= 0xc9 then
> +        error(string.format("unsupported tuple type"))

1. It adds several mp_exp codes, but not all.
2. It does not give info about mp_ext type.
3. msgpack module (Lua-C implementation) is not updated in the same way.
4. 'tuple' is not related here, the module is about msgpack.

Now Vlad improved msgpack extension printing for unknown extensions in
msgpuck for mp_*print*() functions (it was just 'unknown' and becomes
'(extension: type 10, len 35)'). I think that it would be good to report
decoding errors from the Lua modules in the same way.

I verified error messages on 1.10 and 2.5:

1.10:

 | Tarantool 1.10.6-28-g8a457324b
 | type 'help' for interactive help
 | tarantool> require('msgpack').decode('\xd4\xfe\x00')
 | ---
 | - error: 'msgpack.decode: unsupported extension: 212'
 | ...
 |
 | tarantool> require('msgpackffi').decode('\xd4\xfe\x00')
 | ---
 | - error: 'builtin/msgpackffi.lua:546: assertion failed!'
 | ...

(Filed #5017 re 212.)

2.5:

 | Tarantool 2.5.0-62-gbb7c3d167
 | type 'help' for interactive help
 | tarantool> require('msgpackffi').decode('\xd4\xfe\x00')
 | ---
 | - error: 'builtin/msgpackffi.lua:525: Unsupported extension type'
 | ...
 |
 | tarantool> require('msgpack').decode('\xd4\xfe\x00')
 | ---
 | - error: 'Unsuported MsgPack extension type: 4294967294'
 | ...

The latter error message looks as the best one (except that mp_ext type
should be signed and misspelled 'unsuported' -- filed #5016; also filed
#5014 for mp_error decoding).

I think that most of the problem will gone if we'll report the following
information (just like in the latter error message):

1. We meet msgpack extension.
2. It is of type N.
3. Which is unsupported.

Can we report it this way for msgpack and msgpackffi both and for all
tarantool versions?

I would also look, whether we can give a text type name for known, but
unsupported extensions. I filed [3] to organize specifications for them
into its own section or page. When there is a document that lists
supported msgpack extensions, it would look more natural to give them
names in tarantool-1.10 and so. (I also filed #1331 and #1332, because
now 'box protocol' page looks confusing.)

[3]: https://github.com/tarantool/doc/issues/1333

I propose the following error format: 'Unsupported MsgPack extension
type: 1 (tarantool decimal)'.

NB: Can we hold mp_ext types for datetime and other types we want to
encode / decode in the future? (BTW, there is timestamp with -1 type in
the msgpack spec, but I don't know whether our future datetime will fit
to it.) It seems, no, need to investigate first.

WBR, Alexander Turenko.

----

iproto_mock.lua (heavily based on
test/sql-tap/gh-4077-iproto-execute-no-bind.test.lua):

It responds with `decimal.new(1)` to any call request and works on 1.10
as well as newer tarantool.

 | #!/usr/bin/env tarantool
 | 
 | local socket = require('socket')
 | local msgpack = require('msgpack')
 | 
 | local IPROTO_GREETING_SIZE = 128
 | 
 | local IPROTO_REQUEST_TYPE   = 0x00
 | local IPROTO_SCHEMA_VERSION = 0x05
 | local IPROTO_SELECT         = 0x01
 | local IPROTO_CALL           = 0x0a
 | local IPROTO_SYNC           = 0x01
 | local IPROTO_DATA           = 0x30
 | local IPROTO_STATUS_KEY     = 0x00
 | local IPROTO_OK             = 0x00
 | local IPROTO_SPACE_ID       = 0x10
 | 
 | -- Use box to simplify _vspace, _vindex, vcollation responses.
 | -- Also it is useful to obtain greeting and don't hardcode it.
 | local listen = os.getenv('LISTEN') or 3301
 | box.cfg({listen = listen})
 | local GREETING = socket.tcp_connect(nil, listen):read(IPROTO_GREETING_SIZE)
 | assert(GREETING:len() == IPROTO_GREETING_SIZE)
 | box.cfg({listen = box.NULL})
 | 
 | local function accept_handler(s)
 |     s:write(GREETING)
 |     while true do
 |         -- Read request.
 |         local encoded_size = s:read(5)
 |         if encoded_size == nil or encoded_size == '' then
 |             break -- error or EOF
 |         end
 |         local size = msgpack.decode(encoded_size)
 |         local header_body = s:read(size)
 |         local header, header_len = msgpack.decode(header_body)
 |         local body = msgpack.decode(header_body:sub(header_len))
 |         local request_id = header[IPROTO_SYNC]
 |         local request_type = header[IPROTO_REQUEST_TYPE]
 | 
 |         local encoded_response_data
 | 
 |         -- Fill encoded_response_data.
 |         if request_type == IPROTO_SELECT then
 |             -- We need it at least for _vspace, _vindex and
 |             -- _vcollation.
 |             local space_id = body[IPROTO_SPACE_ID]
 |             if box.space[space_id] ~= nil then
 |                 encoded_response_data =
 |                     msgpack.encode(box.space[space_id]:select())
 |             else
 |                 assert(false)
 |             end
 |         elseif request_type == IPROTO_CALL then
 |             -- fixarray 1:     91
 |             -- decimal.new(1): d501001c
 |             encoded_response_data = ('91d501001c'):fromhex()
 |         else
 |             assert(false)
 |         end
 | 
 |         -- Write answer.
 |         local schema_version = 1
 |         local header = msgpack.encode({
 |             [IPROTO_STATUS_KEY]     = IPROTO_OK,
 |             [IPROTO_SYNC]           = request_id,
 |             [IPROTO_SCHEMA_VERSION] = schema_version,
 |         })
 |         -- fixmap 1: 81
 |         local body = '\x81' .. msgpack.encode(IPROTO_DATA) ..
 |             encoded_response_data
 |         local size = msgpack.encode(header:len() + body:len())
 |         s:write(size .. header .. body)
 |     end
 | end
 | 
 | socket.tcp_server('0.0.0.0', listen, {handler = accept_handler})