From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Tue, 5 Mar 2019 17:00:13 +0300 From: Vladimir Davydov Subject: Re: [PATCH v1 1/1] iproto: fix assertion failure on invalid msgpack Message-ID: <20190305140013.6xyoniukextm42vw@esperanza> References: <8d2042cb6269662edfee66e9b3ef3f4b34175975.1551785193.git.kshcherbatov@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <8d2042cb6269662edfee66e9b3ef3f4b34175975.1551785193.git.kshcherbatov@tarantool.org> To: Kirill Shcherbatov Cc: tarantool-patches@freelists.org List-ID: 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; >