[tarantool-patches] Re: [PATCH v1 1/1] iproto: fix assertion failure on invalid msgpack
Vladimir Davydov
vdavydov.dev at gmail.com
Wed Mar 6 14:03:36 MSK 2019
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
More information about the Tarantool-patches
mailing list