[tarantool-patches] Re: [PATCH v1 1/1] iproto: fix assertion failure on invalid msgpack
Kirill Shcherbatov
kshcherbatov at tarantool.org
Tue Mar 5 18:55:16 MSK 2019
> 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
More information about the Tarantool-patches
mailing list