From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 5E156445320 for ; Wed, 8 Jul 2020 18:14:25 +0300 (MSK) From: Aleksandr Lyapunov Date: Wed, 8 Jul 2020 18:14:09 +0300 Message-Id: <1594221263-6228-3-git-send-email-alyapunov@tarantool.org> In-Reply-To: <1594221263-6228-1-git-send-email-alyapunov@tarantool.org> References: <1594221263-6228-1-git-send-email-alyapunov@tarantool.org> Subject: [Tarantool-patches] [PATCH 02/16] Check data_offset overflow in struct tuple List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: tarantool-patches@dev.tarantool.org Cc: v.shpilevoy@tarantool.org data_offset member of tuple is uint16_t now. At the same time this field is calculated from field_map_size which is uint32_t. That could lead to overflows and crashes. Fixes #5084 --- src/box/errcode.h | 1 + src/box/memtx_engine.c | 19 ++++++++----- src/box/tuple.c | 11 ++++++-- src/box/vy_stmt.c | 8 ++++++ test/box/error.result | 1 + test/box/huge_field_map.result | 49 +++++++++++++++++++++++++++++++++ test/box/huge_field_map.test.lua | 22 +++++++++++++++ test/box/huge_field_map_long.result | 51 +++++++++++++++++++++++++++++++++++ test/box/huge_field_map_long.test.lua | 28 +++++++++++++++++++ test/box/suite.ini | 1 + 10 files changed, 183 insertions(+), 8 deletions(-) create mode 100644 test/box/huge_field_map.result create mode 100644 test/box/huge_field_map.test.lua create mode 100644 test/box/huge_field_map_long.result create mode 100644 test/box/huge_field_map_long.test.lua diff --git a/src/box/errcode.h b/src/box/errcode.h index d1e4d02..938d411 100644 --- a/src/box/errcode.h +++ b/src/box/errcode.h @@ -266,6 +266,7 @@ struct errcode_record { /*211 */_(ER_WRONG_QUERY_ID, "Prepared statement with id %u does not exist") \ /*212 */_(ER_SEQUENCE_NOT_STARTED, "Sequence '%s' is not started") \ /*213 */_(ER_NO_SUCH_SESSION_SETTING, "Session setting %s doesn't exist") \ + /*214 */_(ER_TUPLE_METADATA_IS_TOO_BIG, "Can't create tuple: metadata size %u is too big") \ /* * !IMPORTANT! Please follow instructions at start of the file diff --git a/src/box/memtx_engine.c b/src/box/memtx_engine.c index 6ce8cac..b5b6b14 100644 --- a/src/box/memtx_engine.c +++ b/src/box/memtx_engine.c @@ -1125,6 +1125,18 @@ memtx_tuple_new(struct tuple_format *format, const char *data, const char *end) if (tuple_field_map_create(format, data, true, &builder) != 0) goto end; uint32_t field_map_size = field_map_build_size(&builder); + /* + * Data offset is calculated from the begin of the struct + * tuple base, not from memtx_tuple, because the struct + * tuple is not the first field of the memtx_tuple. + */ + uint32_t data_offset = sizeof(struct tuple) + field_map_size; + if (data_offset > UINT16_MAX) { + /** tuple->data_offset is 16 bits */ + diag_set(ClientError, ER_TUPLE_METADATA_IS_TOO_BIG, + data_offset); + goto end; + } size_t tuple_len = end - data; size_t total = sizeof(struct memtx_tuple) + field_map_size + tuple_len; @@ -1157,12 +1169,7 @@ memtx_tuple_new(struct tuple_format *format, const char *data, const char *end) tuple->bsize = tuple_len; tuple->format_id = tuple_format_id(format); tuple_format_ref(format); - /* - * Data offset is calculated from the begin of the struct - * tuple base, not from memtx_tuple, because the struct - * tuple is not the first field of the memtx_tuple. - */ - tuple->data_offset = sizeof(struct tuple) + field_map_size; + tuple->data_offset = data_offset; char *raw = (char *) tuple + tuple->data_offset; field_map_build(&builder, raw - field_map_size); memcpy(raw, data, tuple_len); diff --git a/src/box/tuple.c b/src/box/tuple.c index 1f52a8c..e48ee08 100644 --- a/src/box/tuple.c +++ b/src/box/tuple.c @@ -83,6 +83,13 @@ runtime_tuple_new(struct tuple_format *format, const char *data, const char *end if (tuple_field_map_create(format, data, true, &builder) != 0) goto end; uint32_t field_map_size = field_map_build_size(&builder); + uint32_t data_offset = sizeof(struct tuple) + field_map_size; + if (data_offset > UINT16_MAX) { + /** tuple->data_offset is 16 bits */ + diag_set(ClientError, ER_TUPLE_METADATA_IS_TOO_BIG, + data_offset); + goto end; + } size_t data_len = end - data; size_t total = sizeof(struct tuple) + field_map_size + data_len; @@ -97,8 +104,8 @@ runtime_tuple_new(struct tuple_format *format, const char *data, const char *end tuple->bsize = data_len; tuple->format_id = tuple_format_id(format); tuple_format_ref(format); - tuple->data_offset = sizeof(struct tuple) + field_map_size; - char *raw = (char *) tuple + tuple->data_offset; + tuple->data_offset = data_offset; + char *raw = (char *) tuple + data_offset; field_map_build(&builder, raw - field_map_size); memcpy(raw, data, data_len); say_debug("%s(%zu) = %p", __func__, data_len, tuple); diff --git a/src/box/vy_stmt.c b/src/box/vy_stmt.c index 392f3da..f59c418 100644 --- a/src/box/vy_stmt.c +++ b/src/box/vy_stmt.c @@ -159,6 +159,14 @@ static struct tuple * vy_stmt_alloc(struct tuple_format *format, uint32_t data_offset, uint32_t bsize) { assert(data_offset >= sizeof(struct vy_stmt) + format->field_map_size); + + if (data_offset > UINT16_MAX) { + /** tuple->data_offset is 16 bits */ + diag_set(ClientError, ER_TUPLE_METADATA_IS_TOO_BIG, + data_offset); + return NULL; + } + struct vy_stmt_env *env = format->engine; uint32_t total_size = data_offset + bsize; if (unlikely(total_size > env->max_tuple_size)) { diff --git a/test/box/error.result b/test/box/error.result index 2196fa5..a166824 100644 --- a/test/box/error.result +++ b/test/box/error.result @@ -432,6 +432,7 @@ t; | 211: box.error.WRONG_QUERY_ID | 212: box.error.SEQUENCE_NOT_STARTED | 213: box.error.NO_SUCH_SESSION_SETTING + | 214: box.error.TUPLE_METADATA_IS_TOO_BIG | ... test_run:cmd("setopt delimiter ''"); diff --git a/test/box/huge_field_map.result b/test/box/huge_field_map.result new file mode 100644 index 0000000..11b4da3 --- /dev/null +++ b/test/box/huge_field_map.result @@ -0,0 +1,49 @@ +-- test-run result file version 2 +env = require('test_run') + | --- + | ... +test_run = env.new() + | --- + | ... + +s = box.schema.space.create('test', {engine = 'memtx'}) + | --- + | ... +i1 = s:create_index('pk') + | --- + | ... +i2 = s:create_index('mk', {parts={{'[2][*]', 'uint'}}}) + | --- + | ... +test_run:cmd("setopt delimiter ';'") + | --- + | - true + | ... +function test() + local t = {1, {}} + for i = 1,65536 do + table.insert(t[2], i) + if (i % 4096 == 0) then + s:replace(t) + end + end +end; + | --- + | ... +test_run:cmd("setopt delimiter ''"); + | --- + | - true + | ... + +pcall(test) -- must fail but not crash + | --- + | - false + | - 'Can''t create tuple: metadata size 65558 is too big' + | ... + +test = nil + | --- + | ... +s:drop() + | --- + | ... diff --git a/test/box/huge_field_map.test.lua b/test/box/huge_field_map.test.lua new file mode 100644 index 0000000..9042751 --- /dev/null +++ b/test/box/huge_field_map.test.lua @@ -0,0 +1,22 @@ +env = require('test_run') +test_run = env.new() + +s = box.schema.space.create('test', {engine = 'memtx'}) +i1 = s:create_index('pk') +i2 = s:create_index('mk', {parts={{'[2][*]', 'uint'}}}) +test_run:cmd("setopt delimiter ';'") +function test() + local t = {1, {}} + for i = 1,65536 do + table.insert(t[2], i) + if (i % 4096 == 0) then + s:replace(t) + end + end +end; +test_run:cmd("setopt delimiter ''"); + +pcall(test) -- must fail but not crash + +test = nil +s:drop() \ No newline at end of file diff --git a/test/box/huge_field_map_long.result b/test/box/huge_field_map_long.result new file mode 100644 index 0000000..d7971ae --- /dev/null +++ b/test/box/huge_field_map_long.result @@ -0,0 +1,51 @@ +-- test-run result file version 2 +env = require('test_run') + | --- + | ... +test_run = env.new() + | --- + | ... + +s = box.schema.space.create('test', {engine = 'memtx'}) + | --- + | ... +test_run:cmd("setopt delimiter ';'") + | --- + | - true + | ... +function test() + local t = {} + local k = {} + for i = 1,128 do + local parts = {} + for j = 0,127 do + table.insert(parts, {i * 128 - j, 'uint'}) + table.insert(t, 1) + end + if i == 1 then k = table.deepcopy(t) end + s:create_index('test'..i, {parts = parts}) + if i % 16 == 0 then + s:replace(t) + s:delete(k) + end + end +end; + | --- + | ... +test_run:cmd("setopt delimiter ''"); + | --- + | - true + | ... + +pcall(test) -- must fail but not crash + | --- + | - false + | - 'Can''t create tuple: metadata size 65542 is too big' + | ... + +test = nil + | --- + | ... +s:drop() + | --- + | ... diff --git a/test/box/huge_field_map_long.test.lua b/test/box/huge_field_map_long.test.lua new file mode 100644 index 0000000..6415615 --- /dev/null +++ b/test/box/huge_field_map_long.test.lua @@ -0,0 +1,28 @@ +env = require('test_run') +test_run = env.new() + +s = box.schema.space.create('test', {engine = 'memtx'}) +test_run:cmd("setopt delimiter ';'") +function test() + local t = {} + local k = {} + for i = 1,128 do + local parts = {} + for j = 0,127 do + table.insert(parts, {i * 128 - j, 'uint'}) + table.insert(t, 1) + end + if i == 1 then k = table.deepcopy(t) end + s:create_index('test'..i, {parts = parts}) + if i % 16 == 0 then + s:replace(t) + s:delete(k) + end + end +end; +test_run:cmd("setopt delimiter ''"); + +pcall(test) -- must fail but not crash + +test = nil +s:drop() \ No newline at end of file diff --git a/test/box/suite.ini b/test/box/suite.ini index de8f5a7..801a91e 100644 --- a/test/box/suite.ini +++ b/test/box/suite.ini @@ -3,6 +3,7 @@ core = tarantool description = Database tests script = box.lua disabled = rtree_errinj.test.lua tuple_bench.test.lua +long_run = huge_field_map_long.test.lua config = engine.cfg release_disabled = errinj.test.lua errinj_index.test.lua rtree_errinj.test.lua upsert_errinj.test.lua iproto_stress.test.lua gh-4648-func-load-unload.test.lua lua_libs = lua/fifo.lua lua/utils.lua lua/bitset.lua lua/index_random_test.lua lua/push.lua lua/identifier.lua -- 2.7.4