[Tarantool-patches] [PATCH v1 1/1] sql: do not reset region on select
imeevma at tarantool.org
imeevma at tarantool.org
Wed Dec 23 22:15:52 MSK 2020
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
More information about the Tarantool-patches
mailing list