Re: [PATCH v1 1/1] iproto: fix assertion failure on invalid msgpack

[Vladimir Davydov <vdavydov.dev@gmail.com>]

On Tue, Mar 05, 2019 at 02:29:05PM +0300, Kirill Shcherbatov wrote:
> https://github.com/tarantool/tarantool/tree/kshch/gh-3900-binary-socket-gibberish-crash
> https://github.com/tarantool/tarantool/issues/3900
> 
> In some cases, only the initial portion of the transmitted client
> request buffer is a valid msgpack. Function row_header_decode
> admits such scenario, however, proto_msg_decode used
> assert(*pos == reqend) to ensure that the data has been
> fully processed. It is wrong.
> Based on the error handling policy in this module, let's set diag
> message and continue processing the buffer.
> 
> Closes #3900

Looks like the issue was introduced by commit d9f82b176e24 ("More than
one row in fixheader. Zstd compression. Issue #1656"):

| @@ -48,8 +48,8 @@ xrow_header_decode(struct xrow_header *header, const char **pos,
|  		   const char *end)
|  {
|  	memset(header, 0, sizeof(struct xrow_header));
| -	const char *pos2 = *pos;
| -	if (mp_check(&pos2, end) != 0) {
| +	const char *tmp = *pos;
| +	if (mp_check(&tmp, end) != 0) {
|  error:
|  		tnt_raise(ClientError, ER_INVALID_MSGPACK, "packet header");
|  	}
| @@ -90,10 +90,13 @@ error:
|  	}
|  	assert(*pos <= end);
|  	if (*pos < end) {
| +		const char *body = *pos;
| +		if (mp_check(pos, end)) {
| +			tnt_raise(ClientError, ER_INVALID_MSGPACK, "packet body");
| +		}
|  		header->bodycnt = 1;
| -		header->body[0].iov_base = (void *) *pos;
| -		header->body[0].iov_len = (end - *pos);
| -		*pos = end;
| +		header->body[0].iov_base = (void *) body;
| +		header->body[0].iov_len = *pos - body;
|  	}
|  }

Before the commit, xrow_header_decode treated everything until 'end' as
the packet body while currently it allows a packet to end before 'end'.

Actually, most callers of xrow_header_decode implicitly assume that this
function still acts like that. So I think that rather than replacing the
assertion in iproto with an error, we should add a bool argument to
xrow_header_decode that would specify whether 'end' is an exact expected
position of the packet end or there may be data beyond it. If the flag
is set, it should raise 'packet body' error unless the packet ends
exactly at 'end'.

> ---
>  src/box/iproto.cc         |  5 ++++-
>  test/box/net.box.result   | 25 +++++++++++++++++++++++++
>  test/box/net.box.test.lua | 11 +++++++++++
>  3 files changed, 40 insertions(+), 1 deletion(-)
> 
> diff --git a/src/box/iproto.cc b/src/box/iproto.cc
> index c8b83b16b..4e08c8e0b 100644
> --- a/src/box/iproto.cc
> +++ b/src/box/iproto.cc
> @@ -1159,7 +1159,10 @@ iproto_msg_decode(struct iproto_msg *msg, const char **pos, const char *reqend,
>  
>  	if (xrow_header_decode(&msg->header, pos, reqend))
>  		goto error;
> -	assert(*pos == reqend);
> +	if (unlikely(*pos != reqend)) {
> +		diag_set(ClientError, ER_INVALID_MSGPACK, "packet end");
> +		goto error;
> +	}
>  
>  	type = msg->header.type;
>