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

[Kirill Shcherbatov <kshcherbatov@tarantool.org>]

> 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'.
Hi! Thank you for review.

===================================================

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

---
 src/box/iproto.cc         |  2 +-
 src/box/vy_run.c          |  4 ++--
 src/box/xlog.c            |  2 +-
 src/box/xrow.c            |  6 +++++-
 src/box/xrow.h            | 10 ++++++----
 src/box/xrow_io.cc        |  6 ++++--
 test/box/net.box.result   | 25 +++++++++++++++++++++++++
 test/box/net.box.test.lua | 11 +++++++++++
 test/unit/xrow.cc         |  4 ++--
 9 files changed, 57 insertions(+), 13 deletions(-)

diff --git a/src/box/iproto.cc b/src/box/iproto.cc
index c8b83b16b..3b0ba6234 100644
--- a/src/box/iproto.cc
+++ b/src/box/iproto.cc
@@ -1157,7 +1157,7 @@ iproto_msg_decode(struct iproto_msg *msg, const char **pos, const char *reqend,
 {
 	uint8_t type;
 
-	if (xrow_header_decode(&msg->header, pos, reqend))
+	if (xrow_header_decode(&msg->header, pos, reqend, true))
 		goto error;
 	assert(*pos == reqend);
 
diff --git a/src/box/vy_run.c b/src/box/vy_run.c
index ff39c133d..fae123d8d 100644
--- a/src/box/vy_run.c
+++ b/src/box/vy_run.c
@@ -698,7 +698,7 @@ vy_page_xrow(struct vy_page *page, uint32_t stmt_no,
 	const char *data_end = stmt_no + 1 < page->row_count ?
 			       page->data + page->row_index[stmt_no + 1] :
 			       page->data + page->unpacked_size;
-	return xrow_header_decode(xrow, &data, data_end);
+	return xrow_header_decode(xrow, &data, data_end, false);
 }
 
 /* {{{ vy_run_iterator vy_run_iterator support functions */
@@ -832,7 +832,7 @@ vy_page_read(struct vy_page *page, const struct vy_page_info *page_info,
 	struct xrow_header xrow;
 	data_pos = page->data + page_info->row_index_offset;
 	data_end = page->data + page_info->unpacked_size;
-	if (xrow_header_decode(&xrow, &data_pos, data_end) == -1)
+	if (xrow_header_decode(&xrow, &data_pos, data_end, true) == -1)
 		goto error;
 	if (xrow.type != VY_RUN_ROW_INDEX) {
 		diag_set(ClientError, ER_INVALID_RUN_FILE,
diff --git a/src/box/xlog.c b/src/box/xlog.c
index d5de8e6d8..692b5f5ed 100644
--- a/src/box/xlog.c
+++ b/src/box/xlog.c
@@ -1801,7 +1801,7 @@ xlog_tx_cursor_next_row(struct xlog_tx_cursor *tx_cursor,
 	/* Return row from xlog tx buffer */
 	int rc = xrow_header_decode(xrow,
 				    (const char **)&tx_cursor->rows.rpos,
-				    (const char *)tx_cursor->rows.wpos);
+				    (const char *)tx_cursor->rows.wpos, false);
 	if (rc != 0) {
 		diag_set(XlogError, "can't parse row");
 		/* Discard remaining row data */
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");
+		goto error;
+	}
 	return 0;
 }
 
diff --git a/src/box/xrow.h b/src/box/xrow.h
index 5317ae22d..9c7ca5c2e 100644
--- a/src/box/xrow.h
+++ b/src/box/xrow.h
@@ -128,14 +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.
  *
  * @retval 0 on success
  * @retval -1 on error (check diag)
- * @post *pos == end on success
+ * @post *pos == end on success when ensure_package_read == true.
  */
 int
 xrow_header_decode(struct xrow_header *header,
-		   const char **pos, const char *end);
+		   const char **pos, const char *end, bool ensure_package_read);
 
 /**
  * DML request.
@@ -676,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)
+		      const char *end, bool ensure_package_read)
 {
-	if (xrow_header_decode(header, pos, end) < 0)
+	if (xrow_header_decode(header, pos, end, ensure_package_read) < 0)
 		diag_raise();
 }
 
diff --git a/src/box/xrow_io.cc b/src/box/xrow_io.cc
index e9ee6b0c8..4b6e68bc1 100644
--- a/src/box/xrow_io.cc
+++ b/src/box/xrow_io.cc
@@ -58,7 +58,8 @@ coio_read_xrow(struct ev_io *coio, struct ibuf *in, struct xrow_header *row)
 	if (to_read > 0)
 		coio_breadn(coio, in, to_read);
 
-	xrow_header_decode_xc(row, (const char **) &in->rpos, in->rpos + len);
+	xrow_header_decode_xc(row, (const char **) &in->rpos, in->rpos + len,
+			      true);
 }
 
 void
@@ -89,7 +90,8 @@ coio_read_xrow_timeout_xc(struct ev_io *coio, struct ibuf *in,
 	if (to_read > 0)
 		coio_breadn_timeout(coio, in, to_read, delay);
 
-	xrow_header_decode_xc(row, (const char **) &in->rpos, in->rpos + len);
+	xrow_header_decode_xc(row, (const char **) &in->rpos, in->rpos + len,
+			      true);
 }
 
 
diff --git a/test/box/net.box.result b/test/box/net.box.result
index b800531b4..9ce6117fc 100644
--- a/test/box/net.box.result
+++ b/test/box/net.box.result
@@ -1381,6 +1381,31 @@ 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
+-- binary socket
+--
+socket = require("socket")
+---
+...
+sock = socket.tcp_connect(LISTEN.host, LISTEN.service)
+---
+...
+data = string.fromhex("6783000000000000000000000000000000000000000000800000C8000000000000000000000000000000000000000000FFFF210100373208000000FFFF000055AAEB66486472530D02000000000010A0350001008000001000000000000000000000000000D05700")
+---
+...
+sock:write(data)
+---
+- 104
+...
+sock:close()
+---
+- true
+...
+test_run:grep_log('default', 'ER_INVALID_MSGPACK') ~= nil
+---
+- true
+...
 -- gh-983 selecting a lot of data crashes the server or hangs the
 -- connection
 -- gh-983 test case: iproto connection selecting a lot of data
diff --git a/test/box/net.box.test.lua b/test/box/net.box.test.lua
index 9e5ecfa0d..8d8257d08 100644
--- a/test/box/net.box.test.lua
+++ b/test/box/net.box.test.lua
@@ -540,6 +540,17 @@ test_run:cmd("setopt delimiter ''");
 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
+-- binary socket
+--
+socket = require("socket")
+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
+
 -- gh-983 selecting a lot of data crashes the server or hangs the
 -- connection
 
diff --git a/test/unit/xrow.cc b/test/unit/xrow.cc
index d7c172955..68a334239 100644
--- a/test/unit/xrow.cc
+++ b/test/unit/xrow.cc
@@ -208,7 +208,7 @@ test_xrow_header_encode_decode()
 	char buffer[2048];
 	char *pos = mp_encode_uint(buffer, 300);
 	is(xrow_header_decode(&header, (const char **) &pos,
-			      buffer + 100), -1, "bad msgpack end");
+			      buffer + 100, true), -1, "bad msgpack end");
 
 	header.type = 100;
 	header.replica_id = 200;
@@ -229,7 +229,7 @@ test_xrow_header_encode_decode()
 	begin += fixheader_len;
 	const char *end = (const char *)vec[0].iov_base;
 	end += vec[0].iov_len;
-	is(xrow_header_decode(&decoded_header, &begin, end), 0,
+	is(xrow_header_decode(&decoded_header, &begin, end, true), 0,
 	   "header decode");
 	is(header.type, decoded_header.type, "decoded type");
 	is(header.replica_id, decoded_header.replica_id, "decoded replica_id");
-- 
2.21.0