From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Wed, 6 Mar 2019 14:03:36 +0300 From: Vladimir Davydov Subject: Re: [tarantool-patches] Re: [PATCH v1 1/1] iproto: fix assertion failure on invalid msgpack Message-ID: <20190306110336.s3j5nkto5lsn52qp@esperanza> References: <8d2042cb6269662edfee66e9b3ef3f4b34175975.1551785193.git.kshcherbatov@tarantool.org> <20190305140013.6xyoniukextm42vw@esperanza> <035cb093-c2f2-6a74-400e-d20debba7590@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <035cb093-c2f2-6a74-400e-d20debba7590@tarantool.org> To: Kirill Shcherbatov Cc: tarantool-patches@freelists.org List-ID: On Tue, Mar 05, 2019 at 06:55:16PM +0300, Kirill Shcherbatov wrote: > Before the commit d9f82b176e24 "More than one row in fixheader. > Zstd compression", xrow_header_decode treated everything until > 'end' as the packet body while currently it allows a packet to > end before 'end'. The iproto_msg_decode may receive an invalid > msgpack but it still assumes that xrow_header_decode sets an > error in such case and use assert to test it, bit it is not so. > Introduced a new boolean flag to control routine behaviour. When > flag is set, xrow_header_decode should raise 'packet body' error > unless the packet ends exactly at 'end'. > > Closes #3900 > > https://github.com/tarantool/tarantool/tree/kshch/gh-2027-telnet-alternative-delimiter > https://github.com/tarantool/tarantool/issues/2027 Bad links. Please be more careful. > diff --git a/src/box/xrow.c b/src/box/xrow.c > index 107009c90..a984fa2c9 100644 > --- a/src/box/xrow.c > +++ b/src/box/xrow.c > @@ -90,7 +90,7 @@ mp_decode_vclock(const char **data, struct vclock *vclock) > > int > xrow_header_decode(struct xrow_header *header, const char **pos, > - const char *end) > + const char *end, bool ensure_package_read) > { > memset(header, 0, sizeof(struct xrow_header)); > const char *tmp = *pos; > @@ -170,6 +170,10 @@ error: > header->body[0].iov_base = (void *) body; > header->body[0].iov_len = *pos - body; > } > + if (ensure_package_read && *pos < end) { > + diag_set(ClientError, ER_INVALID_MSGPACK, "packet end"); It's an error in the packet body so I guess it should be "packet body". > + goto error; > + } > return 0; > } Don't like the variable name. Renamed it to end_is_exact and pushed to 2.1 and 1.10. The incremental diff is below: diff --git a/src/box/xrow.c b/src/box/xrow.c index a984fa2c..bddae1d5 100644 --- a/src/box/xrow.c +++ b/src/box/xrow.c @@ -90,7 +90,7 @@ mp_decode_vclock(const char **data, struct vclock *vclock) int xrow_header_decode(struct xrow_header *header, const char **pos, - const char *end, bool ensure_package_read) + const char *end, bool end_is_exact) { memset(header, 0, sizeof(struct xrow_header)); const char *tmp = *pos; @@ -170,9 +170,9 @@ error: header->body[0].iov_base = (void *) body; header->body[0].iov_len = *pos - body; } - if (ensure_package_read && *pos < end) { - diag_set(ClientError, ER_INVALID_MSGPACK, "packet end"); - goto error; + if (end_is_exact && *pos < end) { + diag_set(ClientError, ER_INVALID_MSGPACK, "packet body"); + return -1; } return 0; } diff --git a/src/box/xrow.h b/src/box/xrow.h index 9c7ca5c2..3a8cba19 100644 --- a/src/box/xrow.h +++ b/src/box/xrow.h @@ -128,16 +128,16 @@ xrow_header_encode(const struct xrow_header *header, uint64_t sync, * @param header[out] xrow to fill * @param pos[inout] the start of a packet * @param end the end of a packet - * @parma ensure_package_read True to set error if *pos < end - * on success. - * + * @param end_is_exact if set, raise an error in case the packet + * ends before @end * @retval 0 on success * @retval -1 on error (check diag) - * @post *pos == end on success when ensure_package_read == true. + * @post *pos <= end on success + * @post *pos == end on success if @end_is_exact is set */ int -xrow_header_decode(struct xrow_header *header, - const char **pos, const char *end, bool ensure_package_read); +xrow_header_decode(struct xrow_header *header, const char **pos, + const char *end, bool end_is_exact); /** * DML request. @@ -678,9 +678,9 @@ vclock_follow_xrow(struct vclock* vclock, const struct xrow_header *row) /** @copydoc xrow_header_decode. */ static inline void xrow_header_decode_xc(struct xrow_header *header, const char **pos, - const char *end, bool ensure_package_read) + const char *end, bool end_is_exact) { - if (xrow_header_decode(header, pos, end, ensure_package_read) < 0) + if (xrow_header_decode(header, pos, end, end_is_exact) < 0) diag_raise(); } diff --git a/test/box/net.box.result b/test/box/net.box.result index 9ce6117f..aecaf943 100644 --- a/test/box/net.box.result +++ b/test/box/net.box.result @@ -1382,7 +1382,7 @@ box.schema.user.revoke('guest', 'execute', 'universe') --- ... -- --- 3900: tarantool can be crashed by sending gibberish to a +-- gh-3900: tarantool can be crashed by sending gibberish to a -- binary socket -- socket = require("socket") @@ -1402,9 +1402,9 @@ sock:close() --- - true ... -test_run:grep_log('default', 'ER_INVALID_MSGPACK') ~= nil +test_run:grep_log('default', 'ER_INVALID_MSGPACK.*') --- -- true +- 'ER_INVALID_MSGPACK: Invalid MsgPack - packet body' ... -- gh-983 selecting a lot of data crashes the server or hangs the -- connection diff --git a/test/box/net.box.test.lua b/test/box/net.box.test.lua index 8d8257d0..04d6c190 100644 --- a/test/box/net.box.test.lua +++ b/test/box/net.box.test.lua @@ -541,7 +541,7 @@ test_run:grep_log("default", "ER_NO_SUCH_PROC") box.schema.user.revoke('guest', 'execute', 'universe') -- --- 3900: tarantool can be crashed by sending gibberish to a +-- gh-3900: tarantool can be crashed by sending gibberish to a -- binary socket -- socket = require("socket") @@ -549,7 +549,7 @@ sock = socket.tcp_connect(LISTEN.host, LISTEN.service) data = string.fromhex("6783000000000000000000000000000000000000000000800000C8000000000000000000000000000000000000000000FFFF210100373208000000FFFF000055AAEB66486472530D02000000000010A0350001008000001000000000000000000000000000D05700") sock:write(data) sock:close() -test_run:grep_log('default', 'ER_INVALID_MSGPACK') ~= nil +test_run:grep_log('default', 'ER_INVALID_MSGPACK.*') -- gh-983 selecting a lot of data crashes the server or hangs the -- connection