* [tarantool-patches] [PATCH v2 1/2] sql: proper check for index in sqlite3Insert()
2018-11-15 12:09 [tarantool-patches] [PATCH v2 0/2] sql: proper check for index in sqlite3Insert() imeevma
@ 2018-11-15 12:09 ` imeevma
2018-11-15 13:47 ` [tarantool-patches] " n.pettik
2018-11-15 12:09 ` [tarantool-patches] [PATCH v2 2/2] sql: remove space_column_default_expr() imeevma
2018-11-15 12:27 ` [tarantool-patches] Re: [PATCH v2 0/2] sql: proper check for index in sqlite3Insert() Vladislav Shpilevoy
2 siblings, 1 reply; 6+ messages in thread
From: imeevma @ 2018-11-15 12:09 UTC (permalink / raw)
To: tarantool-patches, v.shpilevoy
Index received in function sqlite3Insert() wasn't checked
properly. It lead to segmentation fault when INSERT and
DROP TABLE executed simultaneously for the same table.
Closes #3780
---
src/box/sql/insert.c | 49 ++++++++++++++++++++++++------------------------
test/sql/errinj.result | 33 ++++++++++++++++++++++++++++++++
test/sql/errinj.test.lua | 12 ++++++++++++
3 files changed, 69 insertions(+), 25 deletions(-)
diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index fd05c02..01bffa4 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -117,14 +117,13 @@ sql_space_autoinc_fieldno(struct space *space)
* SELECT.
*
* @param parser Parse context.
- * @param table Table AST object.
- * @retval true if the table table in database or any of its
- * indices have been opened at any point in the VDBE
- * program.
- * @retval false else.
+ * @param space Space to check read of.
+ * @retval true If the space or any of its indices have been
+ * opened at any point in the VDBE program.
+ * @retval false Else.
*/
static bool
-vdbe_has_table_read(struct Parse *parser, const struct Table *table)
+vdbe_has_table_read(struct Parse *parser, const struct space *space)
{
struct Vdbe *v = sqlite3GetVdbe(parser);
int last_instr = sqlite3VdbeCurrentAddr(v);
@@ -136,12 +135,12 @@ vdbe_has_table_read(struct Parse *parser, const struct Table *table)
* and Write cursors.
*/
if (op->opcode == OP_IteratorOpen) {
- struct space *space = NULL;
+ struct space *space_p4 = NULL;
if (op->p4type == P4_SPACEPTR)
- space = op->p4.space;
+ space_p4 = op->p4.space;
else
continue;
- if (space->def->id == table->def->id)
+ if (space_p4->def->id == space->def->id)
return true;
}
}
@@ -315,24 +314,21 @@ sqlite3Insert(Parse * pParse, /* Parser context */
if (pTab == NULL)
goto insert_cleanup;
- space_id = pTab->def->id;
+ struct space *space = pTab->space;
+ struct space_def *def = space->def;
+ space_id = def->id;
/* Figure out if we have any triggers and if the table being
* inserted into is a view
*/
trigger = sql_triggers_exist(pTab, TK_INSERT, NULL, &tmask);
- bool is_view = pTab->def->opts.is_view;
+ bool is_view = def->opts.is_view;
assert((trigger != NULL && tmask != 0) ||
(trigger == NULL && tmask == 0));
- /* If pTab is really a view, make sure it has been initialized.
- * ViewGetColumnNames() is a no-op if pTab is not a view.
- */
- if (is_view &&
- sql_view_assign_cursors(pParse, pTab->def->opts.sql) != 0)
+ if (is_view && sql_view_assign_cursors(pParse, def->opts.sql) != 0)
goto insert_cleanup;
- struct space_def *def = pTab->def;
/* Cannot insert into a read-only table. */
if (is_view && tmask == 0) {
sqlite3ErrorMsg(pParse, "cannot modify %s because it is a view",
@@ -340,6 +336,12 @@ sqlite3Insert(Parse * pParse, /* Parser context */
goto insert_cleanup;
}
+ if (! is_view && index_find(space, 0) == NULL) {
+ pParse->nErr++;
+ pParse->rc = SQL_TARANTOOL_ERROR;
+ goto insert_cleanup;
+ }
+
/* Allocate a VDBE. */
v = sqlite3GetVdbe(pParse);
if (v == NULL)
@@ -358,7 +360,7 @@ sqlite3Insert(Parse * pParse, /* Parser context */
* This is the 2nd template.
*/
if (pColumn == 0 &&
- xferOptimization(pParse, pTab->space, pSelect, on_error)) {
+ xferOptimization(pParse, space, pSelect, on_error)) {
assert(trigger == NULL);
assert(pList == 0);
goto insert_end;
@@ -459,7 +461,7 @@ sqlite3Insert(Parse * pParse, /* Parser context */
* the SELECT statement. Also use a temp table in
* the case of row triggers.
*/
- if (trigger != NULL || vdbe_has_table_read(pParse, pTab))
+ if (trigger != NULL || vdbe_has_table_read(pParse, space))
useTempTable = 1;
if (useTempTable) {
@@ -576,8 +578,6 @@ sqlite3Insert(Parse * pParse, /* Parser context */
sqlite3VdbeAddOp1(v, OP_Yield, dest.iSDParm);
VdbeCoverage(v);
}
- struct space *space = space_by_id(pTab->def->id);
- assert(space != NULL);
uint32_t autoinc_fieldno = sql_space_autoinc_fieldno(space);
/* Run the BEFORE and INSTEAD OF triggers, if there are any
*/
@@ -627,7 +627,7 @@ sqlite3Insert(Parse * pParse, /* Parser context */
* table column affinities.
*/
if (!is_view)
- sql_emit_table_affinity(v, pTab->def, regCols + 1);
+ sql_emit_table_affinity(v, def, regCols + 1);
/* Fire BEFORE or INSTEAD OF triggers */
vdbe_code_row_trigger(pParse, trigger, TK_INSERT, 0,
@@ -662,8 +662,7 @@ sqlite3Insert(Parse * pParse, /* Parser context */
if (i == (int) autoinc_fieldno) {
sqlite3VdbeAddOp2(v,
OP_NextAutoincValue,
- pTab->def->id,
- iRegStore);
+ space_id, iRegStore);
continue;
}
struct Expr *dflt = NULL;
@@ -773,7 +772,7 @@ sqlite3Insert(Parse * pParse, /* Parser context */
on_error, endOfLoop, 0);
fkey_emit_check(pParse, pTab, 0, regIns, 0);
vdbe_emit_insertion_completion(v, space, regIns + 1,
- pTab->def->field_count,
+ def->field_count,
on_error);
}
diff --git a/test/sql/errinj.result b/test/sql/errinj.result
index cb993f8..beceafb 100644
--- a/test/sql/errinj.result
+++ b/test/sql/errinj.result
@@ -280,3 +280,36 @@ errinj.set("ERRINJ_WAL_IO", false)
box.sql.execute("DROP TABLE t3;")
---
...
+-- gh-3780: Segmentation fault with two users changing the same
+-- SQL table
+box.sql.execute('create table test (id int primary key)')
+---
+...
+errinj.set("ERRINJ_WAL_DELAY", true)
+---
+- ok
+...
+function execute_yield_drop_table() box.sql.execute("drop table test") end
+---
+...
+f1 = fiber.create(execute_yield_drop_table)
+---
+...
+while f1:status() ~= 'suspended' do fiber.sleep(0) end
+---
+...
+box.sql.execute("insert into test values (1)")
+---
+- error: 'No index #0 is defined in space ''TEST'''
+...
+errinj.set("ERRINJ_WAL_DELAY", false)
+---
+- ok
+...
+while f1:status() ~= 'dead' do fiber.sleep(0) end
+---
+...
+box.sql.execute("drop table test")
+---
+- error: 'no such table: TEST'
+...
diff --git a/test/sql/errinj.test.lua b/test/sql/errinj.test.lua
index fa7f9f2..a66a812 100644
--- a/test/sql/errinj.test.lua
+++ b/test/sql/errinj.test.lua
@@ -97,3 +97,15 @@ box.sql.execute("ALTER TABLE t3 DROP CONSTRAINT fk1;")
box.sql.execute("INSERT INTO t3 VALUES(1, 1, 3);")
errinj.set("ERRINJ_WAL_IO", false)
box.sql.execute("DROP TABLE t3;")
+
+-- gh-3780: Segmentation fault with two users changing the same
+-- SQL table
+box.sql.execute('create table test (id int primary key)')
+errinj.set("ERRINJ_WAL_DELAY", true)
+function execute_yield_drop_table() box.sql.execute("drop table test") end
+f1 = fiber.create(execute_yield_drop_table)
+while f1:status() ~= 'suspended' do fiber.sleep(0) end
+box.sql.execute("insert into test values (1)")
+errinj.set("ERRINJ_WAL_DELAY", false)
+while f1:status() ~= 'dead' do fiber.sleep(0) end
+box.sql.execute("drop table test")
--
2.7.4
^ permalink raw reply [flat|nested] 6+ messages in thread* [tarantool-patches] [PATCH v2 2/2] sql: remove space_column_default_expr()
2018-11-15 12:09 [tarantool-patches] [PATCH v2 0/2] sql: proper check for index in sqlite3Insert() imeevma
2018-11-15 12:09 ` [tarantool-patches] [PATCH v2 1/2] " imeevma
@ 2018-11-15 12:09 ` imeevma
2018-11-15 13:47 ` [tarantool-patches] " n.pettik
2018-11-15 12:27 ` [tarantool-patches] Re: [PATCH v2 0/2] sql: proper check for index in sqlite3Insert() Vladislav Shpilevoy
2 siblings, 1 reply; 6+ messages in thread
From: imeevma @ 2018-11-15 12:09 UTC (permalink / raw)
To: tarantool-patches, v.shpilevoy
---
src/box/sql.c | 14 --------------
src/box/sql.h | 11 -----------
src/box/sql/insert.c | 19 +++++++++----------
3 files changed, 9 insertions(+), 35 deletions(-)
diff --git a/src/box/sql.c b/src/box/sql.c
index caa6614..98c4ebd 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -1405,20 +1405,6 @@ tarantoolSqlNextSeqId(uint64_t *max_id)
return tuple_field_u64(tuple, BOX_SEQUENCE_FIELD_ID, max_id);
}
-struct Expr*
-space_column_default_expr(uint32_t space_id, uint32_t fieldno)
-{
- struct space *space;
- space = space_cache_find(space_id);
- assert(space != NULL);
- assert(space->def != NULL);
- if (space->def->opts.is_view)
- return NULL;
- assert(space->def->field_count > fieldno);
-
- return space->def->fields[fieldno].default_value_expr;
-}
-
struct space_def *
sql_ephemeral_space_def_new(struct Parse *parser, const char *name)
{
diff --git a/src/box/sql.h b/src/box/sql.h
index ec6d9d3..1c48417 100644
--- a/src/box/sql.h
+++ b/src/box/sql.h
@@ -162,17 +162,6 @@ void
sql_expr_extract_select(struct Parse *parser, struct Select *select);
/**
- * Given space_id and field number, return default value
- * for the field.
- * @param space_id Space ID.
- * @param fieldno Field index.
- * @retval Pointer to AST corresponding to default value.
- * Can be NULL if no DEFAULT specified or it is a view.
- */
-struct Expr*
-space_column_default_expr(uint32_t space_id, uint32_t fieldno);
-
-/**
* Get server checks list by space_id.
* @param space_id Space ID.
* @retval Checks list.
diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index 01bffa4..6f77202 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -601,11 +601,12 @@ sqlite3Insert(Parse * pParse, /* Parser context */
regCols + i + 1);
} else {
struct Expr *dflt = NULL;
- dflt = space_column_default_expr(
- space_id,
- i);
- sqlite3ExprCode(pParse,
- dflt,
+ if (! is_view) {
+ struct field_def *f =
+ &def->fields[i];
+ dflt = f->default_value_expr;
+ }
+ sqlite3ExprCode(pParse, dflt,
regCols + i + 1);
}
} else if (useTempTable) {
@@ -665,10 +666,8 @@ sqlite3Insert(Parse * pParse, /* Parser context */
space_id, iRegStore);
continue;
}
- struct Expr *dflt = NULL;
- dflt = space_column_default_expr(
- space_id,
- i);
+ struct Expr *dflt =
+ def->fields[i].default_value_expr;
sqlite3ExprCodeFactorable(pParse,
dflt,
iRegStore);
@@ -903,7 +902,7 @@ vdbe_emit_constraint_checks(struct Parse *parse_context, struct Table *tab,
/* ABORT is a default error action. */
if (on_conflict_nullable == ON_CONFLICT_ACTION_DEFAULT)
on_conflict_nullable = ON_CONFLICT_ACTION_ABORT;
- struct Expr *dflt = space_column_default_expr(def->id, i);
+ struct Expr *dflt = def->fields[i].default_value_expr;
if (on_conflict_nullable == ON_CONFLICT_ACTION_REPLACE &&
dflt == NULL)
on_conflict_nullable = ON_CONFLICT_ACTION_ABORT;
--
2.7.4
^ permalink raw reply [flat|nested] 6+ messages in thread