[tarantool-patches] [PATCH v2 0/2] sql: proper check for index in sqlite3Insert()

[tarantool-patches] [PATCH v2 0/2] sql: proper check for index in sqlite3Insert()

[imeevma]

In case when different fibers try to "INSERT INTO table" and
"DROP TABLE table" at the same time it is possible to get
SEGMENTATION FAULT because INSERT tried to use nonexistent index.

Patch 1 adds a check for index in given space and do some
refactoring.

Patch 2 do some refactoring.

Changes in v2:
 - Check that index exists instead of do less work if it doesn't.
 - Refactoring: reduced "struct Table" usage.
 - Refactoring: removed space_column_default_expr().

https://github.com/tarantool/tarantool/issues/3780
https://github.com/tarantool/tarantool/tree/imeevma/gh-3780-proper-index-check

Mergen Imeev (1):
  sql: proper check for index in sqlite3Insert()

Vladislav Shpilevoy (1):
  sql: remove space_column_default_expr()

 src/box/sql.c            | 14 ----------
 src/box/sql.h            | 11 --------
 src/box/sql/insert.c     | 68 +++++++++++++++++++++++-------------------------
 test/sql/errinj.result   | 33 +++++++++++++++++++++++
 test/sql/errinj.test.lua | 12 +++++++++
 5 files changed, 78 insertions(+), 60 deletions(-)

-- 
2.7.4

[tarantool-patches] [PATCH v2 1/2] sql: proper check for index in sqlite3Insert()

[imeevma]

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

[tarantool-patches] [PATCH v2 2/2] sql: remove space_column_default_expr()

[imeevma]

---
 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

[tarantool-patches] Re: [PATCH v2 0/2] sql: proper check for index in sqlite3Insert()

[Vladislav Shpilevoy]

Nikita, please, review.

On 15/11/2018 15:09, imeevma@tarantool.org wrote:
> In case when different fibers try to "INSERT INTO table" and
> "DROP TABLE table" at the same time it is possible to get
> SEGMENTATION FAULT because INSERT tried to use nonexistent index.
> 
> Patch 1 adds a check for index in given space and do some
> refactoring.
> 
> Patch 2 do some refactoring.
> 
> Changes in v2:
>   - Check that index exists instead of do less work if it doesn't.
>   - Refactoring: reduced "struct Table" usage.
>   - Refactoring: removed space_column_default_expr().
> 
> https://github.com/tarantool/tarantool/issues/3780
> https://github.com/tarantool/tarantool/tree/imeevma/gh-3780-proper-index-check
> 
> Mergen Imeev (1):
>    sql: proper check for index in sqlite3Insert()
> 
> Vladislav Shpilevoy (1):
>    sql: remove space_column_default_expr()
> 
>   src/box/sql.c            | 14 ----------
>   src/box/sql.h            | 11 --------
>   src/box/sql/insert.c     | 68 +++++++++++++++++++++++-------------------------
>   test/sql/errinj.result   | 33 +++++++++++++++++++++++
>   test/sql/errinj.test.lua | 12 +++++++++
>   5 files changed, 78 insertions(+), 60 deletions(-)
> 

[tarantool-patches] Re: [PATCH v2 1/2] sql: proper check for index in sqlite3Insert()

[n.pettik]

> On 15 Nov 2018, at 15:09, imeevma@tarantool.org wrote:
> 
> Index received in function sqlite3Insert() wasn't checked
> properly. It lead to segmentation fault when INSERT and

Typo: led (or leads).

> DROP TABLE executed simultaneously for the same table.

This particular fix seems to be OK, but what about other cases?
For example executing different SELECTs when table/index is dropped,
or executing deletes/updates, or executing INSERT with different riggers
or foreign keys and so on and so forth.

Could you please spend some time to deeply investigate this issue?
I am afraid that SQL in its current state is unlikely to be adopted to
such emergencies...

> 
> 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(-)
> 
> @@ -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;

Small refactoring:

diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index 6f7720201..235be2b9f 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -281,7 +281,6 @@ sqlite3Insert(Parse * pParse,       /* Parser context */
        int regTupleid;         /* registers holding insert tupleid */
        int regData;            /* register holding first column to insert */
        int *aRegIdx = 0;       /* One register allocated to each index */
-       uint32_t space_id = 0;
        /* List of triggers on pTab, if required. */
        struct sql_trigger *trigger;
        int tmask;              /* Mask of trigger times */
@@ -316,7 +315,7 @@ sqlite3Insert(Parse * pParse,       /* Parser context */
 
        struct space *space = pTab->space;
        struct space_def *def = space->def;
-       space_id = def->id;
+       uint32_t 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;
> +	}

Please, put explanation comment here. In a few weeks everyone
will forget about this bug, so this snippet will look quite strange.

Also, lets separate refactoring (in our case substituting usage of
struct Table with struct space) and functional changes (validating PK)
and put them into different commits (not now but for the next patches).
I know that in SQL patches as a rule they come together since there are
a lot of codestyle violations, but it makes reviewing process MUCH more easier.

[tarantool-patches] Re: [PATCH v2 2/2] sql: remove space_column_default_expr()

[n.pettik]

This is OK.