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

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

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