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