From: imeevma@tarantool.org To: korablev@tarantool.org Cc: tarantool-patches@dev.tarantool.org Subject: [Tarantool-patches] [PATCH v1 1/1] sql: do not reset region on select Date: Wed, 23 Dec 2020 22:15:52 +0300 [thread overview] Message-ID: <265e8f7a40dcedbfb67813643ef053cc76047e0c.1608750877.git.imeevma@gmail.com> (raw) Prior to this patch, region on fiber was reset during select(), get(), count(), max(), or min(). This would result in an error if one of these operations was used in a user-defined function in SQL. After this patch, these functions truncate region instead of resetting it. Closes #5427 --- https://github.com/tarantool/tarantool/issues/5427 https://github.com/tarantool/tarantool/tree/imeevma/gh-5427-lua-func-changes-result @ChangeLog - Fixed memory corruption when SQL called Lua functions with box calls inside (gh-5427). src/box/box.cc | 5 +- src/box/index.cc | 25 +-- src/box/sql.c | 5 +- src/box/txn.h | 23 ++- .../gh-5427-lua-func-changes-result.result | 149 ++++++++++++++++++ .../gh-5427-lua-func-changes-result.test.lua | 68 ++++++++ 6 files changed, 258 insertions(+), 17 deletions(-) create mode 100644 test/sql/gh-5427-lua-func-changes-result.result create mode 100644 test/sql/gh-5427-lua-func-changes-result.test.lua diff --git a/src/box/box.cc b/src/box/box.cc index 306db7c37..7f23487ed 100644 --- a/src/box/box.cc +++ b/src/box/box.cc @@ -1413,7 +1413,8 @@ box_select(uint32_t space_id, uint32_t index_id, }); struct txn *txn; - if (txn_begin_ro_stmt(space, &txn) != 0) + struct txn_ro_savepoint svp; + if (txn_begin_ro_stmt(space, &txn, &svp) != 0) return -1; struct iterator *it = index_create_iterator(index, type, @@ -1447,7 +1448,7 @@ box_select(uint32_t space_id, uint32_t index_id, txn_rollback_stmt(txn); return -1; } - txn_commit_ro_stmt(txn); + txn_commit_ro_stmt(txn, &svp); return 0; } diff --git a/src/box/index.cc b/src/box/index.cc index c2fc00867..179cc190f 100644 --- a/src/box/index.cc +++ b/src/box/index.cc @@ -239,13 +239,14 @@ box_index_get(uint32_t space_id, uint32_t index_id, const char *key, return -1; /* Start transaction in the engine. */ struct txn *txn; - if (txn_begin_ro_stmt(space, &txn) != 0) + struct txn_ro_savepoint svp; + if (txn_begin_ro_stmt(space, &txn, &svp) != 0) return -1; if (index_get(index, key, part_count, result) != 0) { txn_rollback_stmt(txn); return -1; } - txn_commit_ro_stmt(txn); + txn_commit_ro_stmt(txn, &svp); /* Count statistics. */ rmean_collect(rmean_box, IPROTO_SELECT, 1); if (*result != NULL) @@ -273,13 +274,14 @@ box_index_min(uint32_t space_id, uint32_t index_id, const char *key, return -1; /* Start transaction in the engine. */ struct txn *txn; - if (txn_begin_ro_stmt(space, &txn) != 0) + struct txn_ro_savepoint svp; + if (txn_begin_ro_stmt(space, &txn, &svp) != 0) return -1; if (index_min(index, key, part_count, result) != 0) { txn_rollback_stmt(txn); return -1; } - txn_commit_ro_stmt(txn); + txn_commit_ro_stmt(txn, &svp); if (*result != NULL) tuple_bless(*result); return 0; @@ -305,13 +307,14 @@ box_index_max(uint32_t space_id, uint32_t index_id, const char *key, return -1; /* Start transaction in the engine. */ struct txn *txn; - if (txn_begin_ro_stmt(space, &txn) != 0) + struct txn_ro_savepoint svp; + if (txn_begin_ro_stmt(space, &txn, &svp) != 0) return -1; if (index_max(index, key, part_count, result) != 0) { txn_rollback_stmt(txn); return -1; } - txn_commit_ro_stmt(txn); + txn_commit_ro_stmt(txn, &svp); if (*result != NULL) tuple_bless(*result); return 0; @@ -338,14 +341,15 @@ box_index_count(uint32_t space_id, uint32_t index_id, int type, return -1; /* Start transaction in the engine. */ struct txn *txn; - if (txn_begin_ro_stmt(space, &txn) != 0) + struct txn_ro_savepoint svp; + if (txn_begin_ro_stmt(space, &txn, &svp) != 0) return -1; ssize_t count = index_count(index, itype, key, part_count); if (count < 0) { txn_rollback_stmt(txn); return -1; } - txn_commit_ro_stmt(txn); + txn_commit_ro_stmt(txn, &svp); return count; } @@ -374,7 +378,8 @@ box_index_iterator(uint32_t space_id, uint32_t index_id, int type, if (key_validate(index->def, itype, key, part_count)) return NULL; struct txn *txn; - if (txn_begin_ro_stmt(space, &txn) != 0) + struct txn_ro_savepoint svp; + if (txn_begin_ro_stmt(space, &txn, &svp) != 0) return NULL; struct iterator *it = index_create_iterator(index, itype, key, part_count); @@ -382,7 +387,7 @@ box_index_iterator(uint32_t space_id, uint32_t index_id, int type, txn_rollback_stmt(txn); return NULL; } - txn_commit_ro_stmt(txn); + txn_commit_ro_stmt(txn, &svp); rmean_collect(rmean_box, IPROTO_SELECT, 1); return it; } diff --git a/src/box/sql.c b/src/box/sql.c index c55d0f4e5..3d968e56a 100644 --- a/src/box/sql.c +++ b/src/box/sql.c @@ -901,7 +901,8 @@ cursor_seek(BtCursor *pCur, int *pRes) struct space *space = pCur->space; struct txn *txn = NULL; - if (space->def->id != 0 && txn_begin_ro_stmt(space, &txn) != 0) + struct txn_ro_savepoint svp; + if (space->def->id != 0 && txn_begin_ro_stmt(space, &txn, &svp) != 0) return -1; struct iterator *it = index_create_iterator(pCur->index, pCur->iter_type, key, @@ -913,7 +914,7 @@ cursor_seek(BtCursor *pCur, int *pRes) return -1; } if (txn != NULL) - txn_commit_ro_stmt(txn); + txn_commit_ro_stmt(txn, &svp); pCur->iter = it; pCur->eState = CURSOR_VALID; diff --git a/src/box/txn.h b/src/box/txn.h index dfa7c0ee4..fca9bc1d0 100644 --- a/src/box/txn.h +++ b/src/box/txn.h @@ -258,6 +258,20 @@ struct txn_savepoint { char name[1]; }; +/** + * Read-only transaction savepoint object. After completing a read-only + * transaction, we must clear the region. However, if we just reset the region, + * we may corrupt the data that was placed in the region before the read-only + * transaction began. To avoid this, we should use truncation. This structure + * contains the information required for truncation. + */ +struct txn_ro_savepoint { + /** Region used during this transaction. */ + struct region *region; + /** Savepoint for region. */ + size_t region_used; +}; + extern double too_long_threshold; /** @@ -557,24 +571,27 @@ txn_begin_in_engine(struct engine *engine, struct txn *txn); * select. */ static inline int -txn_begin_ro_stmt(struct space *space, struct txn **txn) +txn_begin_ro_stmt(struct space *space, struct txn **txn, + struct txn_ro_savepoint *svp) { *txn = in_txn(); if (*txn != NULL) { struct engine *engine = space->engine; return txn_begin_in_engine(engine, *txn); } + svp->region = &fiber()->gc; + svp->region_used = region_used(svp->region); return 0; } static inline void -txn_commit_ro_stmt(struct txn *txn) +txn_commit_ro_stmt(struct txn *txn, struct txn_ro_savepoint *svp) { assert(txn == in_txn()); if (txn) { /* nothing to do */ } else { - fiber_gc(); + region_truncate(svp->region, svp->region_used); } } diff --git a/test/sql/gh-5427-lua-func-changes-result.result b/test/sql/gh-5427-lua-func-changes-result.result new file mode 100644 index 000000000..7175ea78a --- /dev/null +++ b/test/sql/gh-5427-lua-func-changes-result.result @@ -0,0 +1,149 @@ +-- test-run result file version 2 +test_run = require('test_run').new() + | --- + | ... +engine = test_run:get_cfg('engine') + | --- + | ... +_ = box.space._session_settings:update('sql_default_engine', {{'=', 2, engine}}) + | --- + | ... + +box.execute([[CREATE TABLE t (b STRING PRIMARY KEY);]]) + | --- + | - row_count: 1 + | ... +box.execute([[INSERT INTO t VALUES ('aaaaaaaaaaaa');]]) + | --- + | - row_count: 1 + | ... +test_run:cmd("setopt delimiter ';'"); + | --- + | - true + | ... +box.schema.func.create('CORRUPT_SELECT', { + returns = 'integer', + body = [[ + function() + box.space.T:select() + return 1 + end]], + exports = {'LUA', 'SQL'}, +}); + | --- + | ... +box.schema.func.create('CORRUPT_GET', { + returns = 'integer', + body = [[ + function() + box.space.T:get('aaaaaaaaaaaa') + return 1 + end]], + exports = {'LUA', 'SQL'}, +}); + | --- + | ... +box.schema.func.create('CORRUPT_COUNT', { + returns = 'integer', + body = [[ + function() + box.space.T:count() + return 1 + end]], + exports = {'LUA', 'SQL'}, +}); + | --- + | ... +box.schema.func.create('CORRUPT_MAX', { + returns = 'integer', + body = [[ + function() + box.space.T.index[0]:max() + return 1 + end]], + exports = {'LUA', 'SQL'}, +}); + | --- + | ... +box.schema.func.create('CORRUPT_MIN', { + returns = 'integer', + body = [[ + function() + box.space.T.index[0]:min() + return 1 + end]], + exports = {'LUA', 'SQL'}, +}); + | --- + | ... +test_run:cmd("setopt delimiter ''"); + | --- + | - true + | ... + +values = {"aaaaaaaaaaaa", "aaaaaaaaaaaa"} + | --- + | ... +query = [[select %s() from t where t.b = ? and t.b <= ? order by t.b;]] + | --- + | ... +box.execute(string.format(query, 'CORRUPT_SELECT'), values) + | --- + | - metadata: + | - name: COLUMN_1 + | type: integer + | rows: + | - [1] + | ... +box.execute(string.format(query, 'CORRUPT_GET'), values) + | --- + | - metadata: + | - name: COLUMN_1 + | type: integer + | rows: + | - [1] + | ... +box.execute(string.format(query, 'CORRUPT_COUNT'), values) + | --- + | - metadata: + | - name: COLUMN_1 + | type: integer + | rows: + | - [1] + | ... +box.execute(string.format(query, 'CORRUPT_MAX'), values) + | --- + | - metadata: + | - name: COLUMN_1 + | type: integer + | rows: + | - [1] + | ... +box.execute(string.format(query, 'CORRUPT_MIN'), values) + | --- + | - metadata: + | - name: COLUMN_1 + | type: integer + | rows: + | - [1] + | ... +box.execute([[DROP TABLE t;]]) + | --- + | - row_count: 1 + | ... + +box.func.CORRUPT_SELECT:drop() + | --- + | ... +box.func.CORRUPT_GET:drop() + | --- + | ... +box.func.CORRUPT_COUNT:drop() + | --- + | ... +box.func.CORRUPT_MAX:drop() + | --- + | ... +box.func.CORRUPT_MIN:drop() + | --- + | ... diff --git a/test/sql/gh-5427-lua-func-changes-result.test.lua b/test/sql/gh-5427-lua-func-changes-result.test.lua new file mode 100644 index 000000000..edbbc9ee4 --- /dev/null +++ b/test/sql/gh-5427-lua-func-changes-result.test.lua @@ -0,0 +1,68 @@ +test_run = require('test_run').new() +engine = test_run:get_cfg('engine') +_ = box.space._session_settings:update('sql_default_engine', {{'=', 2, engine}}) + +box.execute([[CREATE TABLE t (b STRING PRIMARY KEY);]]) +box.execute([[INSERT INTO t VALUES ('aaaaaaaaaaaa');]]) +test_run:cmd("setopt delimiter ';'"); +box.schema.func.create('CORRUPT_SELECT', { + returns = 'integer', + body = [[ + function() + box.space.T:select() + return 1 + end]], + exports = {'LUA', 'SQL'}, +}); +box.schema.func.create('CORRUPT_GET', { + returns = 'integer', + body = [[ + function() + box.space.T:get('aaaaaaaaaaaa') + return 1 + end]], + exports = {'LUA', 'SQL'}, +}); +box.schema.func.create('CORRUPT_COUNT', { + returns = 'integer', + body = [[ + function() + box.space.T:count() + return 1 + end]], + exports = {'LUA', 'SQL'}, +}); +box.schema.func.create('CORRUPT_MAX', { + returns = 'integer', + body = [[ + function() + box.space.T.index[0]:max() + return 1 + end]], + exports = {'LUA', 'SQL'}, +}); +box.schema.func.create('CORRUPT_MIN', { + returns = 'integer', + body = [[ + function() + box.space.T.index[0]:min() + return 1 + end]], + exports = {'LUA', 'SQL'}, +}); +test_run:cmd("setopt delimiter ''"); + +values = {"aaaaaaaaaaaa", "aaaaaaaaaaaa"} +query = [[select %s() from t where t.b = ? and t.b <= ? order by t.b;]] +box.execute(string.format(query, 'CORRUPT_SELECT'), values) +box.execute(string.format(query, 'CORRUPT_GET'), values) +box.execute(string.format(query, 'CORRUPT_COUNT'), values) +box.execute(string.format(query, 'CORRUPT_MAX'), values) +box.execute(string.format(query, 'CORRUPT_MIN'), values) +box.execute([[DROP TABLE t;]]) + +box.func.CORRUPT_SELECT:drop() +box.func.CORRUPT_GET:drop() +box.func.CORRUPT_COUNT:drop() +box.func.CORRUPT_MAX:drop() +box.func.CORRUPT_MIN:drop() -- 2.25.1
next reply other threads:[~2020-12-23 19:15 UTC|newest] Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-12-23 19:15 imeevma [this message] 2020-12-24 10:48 ` Nikita Pettik 2020-12-24 10:58 ` Nikita Pettik -- strict thread matches above, loose matches on Subject: below -- 2020-11-02 9:49 imeevma 2020-11-04 22:14 ` Vladislav Shpilevoy 2020-12-16 5:34 ` Mergen Imeev 2020-12-17 22:04 ` Vladislav Shpilevoy 2020-12-22 19:30 ` Mergen Imeev 2020-12-23 14:58 ` Vladislav Shpilevoy 2020-12-23 19:20 ` Mergen Imeev
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=265e8f7a40dcedbfb67813643ef053cc76047e0c.1608750877.git.imeevma@gmail.com \ --to=imeevma@tarantool.org \ --cc=korablev@tarantool.org \ --cc=tarantool-patches@dev.tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH v1 1/1] sql: do not reset region on select' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox