[tarantool-patches] [PATCH v1 1/1] sql: proper check for index in vdbe_emit_constraint_checks()

[tarantool-patches] [PATCH v1 1/1] sql: proper check for index in vdbe_emit_constraint_checks()

[imeevma]

Index received in function vdbe_emit_constraint_checks() wasn't
checked properly. It lead to segmentation fault when INSERT and
DROP TABLE executed simultaneously for the same table.

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

 src/box/sql/insert.c     | 24 +++++++++++++-----------
 test/sql/errinj.result   | 33 +++++++++++++++++++++++++++++++++
 test/sql/errinj.test.lua | 12 ++++++++++++
 3 files changed, 58 insertions(+), 11 deletions(-)

diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index fd05c02..45f59b1 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -983,18 +983,20 @@ vdbe_emit_constraint_checks(struct Parse *parse_context, struct Table *tab,
 	 * strict typing.
 	 */
 	struct index *pk = space_index(tab->space, 0);
-	uint32_t part_count = pk->def->key_def->part_count;
-	if (part_count == 1) {
-		uint32_t fieldno = pk->def->key_def->parts[0].fieldno;
-		int reg_pk = new_tuple_reg + fieldno;
-		if (def->fields[fieldno].affinity == AFFINITY_INTEGER) {
-			int skip_if_null = sqlite3VdbeMakeLabel(v);
-			if (autoinc_fieldno != UINT32_MAX) {
-				sqlite3VdbeAddOp2(v, OP_IsNull, reg_pk,
-						  skip_if_null);
+	if (pk != NULL) {
+		uint32_t part_count = pk->def->key_def->part_count;
+		if (part_count == 1) {
+			uint32_t fieldno = pk->def->key_def->parts[0].fieldno;
+			int reg_pk = new_tuple_reg + fieldno;
+			if (def->fields[fieldno].affinity == AFFINITY_INTEGER) {
+				int skip_if_null = sqlite3VdbeMakeLabel(v);
+				if (autoinc_fieldno != UINT32_MAX) {
+					sqlite3VdbeAddOp2(v, OP_IsNull, reg_pk,
+							  skip_if_null);
+				}
+				sqlite3VdbeAddOp2(v, OP_MustBeInt, reg_pk, 0);
+				sqlite3VdbeResolveLabel(v, skip_if_null);
 			}
-			sqlite3VdbeAddOp2(v, OP_MustBeInt, reg_pk, 0);
-			sqlite3VdbeResolveLabel(v, skip_if_null);
 		}
 	}
 	/*
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] Re: [PATCH v1 1/1] sql: proper check for index in vdbe_emit_constraint_checks()

[Vladislav Shpilevoy]

Hi! Thanks for the patch! See 1 comment
below, my fixes at the end of the email and
on the branch.

On 10/11/2018 12:14, imeevma@tarantool.org wrote:
> Index received in function vdbe_emit_constraint_checks() wasn't
> checked properly. It lead to segmentation fault when INSERT and
> DROP TABLE executed simultaneously for the same table.
> 
> Closes #3780
> ---
> Issue: https://github.com/tarantool/tarantool/issues/3780
> Branch: https://github.com/tarantool/tarantool/tree/imeevma/gh-3780-proper-index-check
> 
>   src/box/sql/insert.c     | 24 +++++++++++++-----------
>   test/sql/errinj.result   | 33 +++++++++++++++++++++++++++++++++
>   test/sql/errinj.test.lua | 12 ++++++++++++
>   3 files changed, 58 insertions(+), 11 deletions(-)
> 
> diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
> index fd05c02..45f59b1 100644
> --- a/src/box/sql/insert.c
> +++ b/src/box/sql/insert.c
> @@ -983,18 +983,20 @@ vdbe_emit_constraint_checks(struct Parse *parse_context, struct Table *tab,
>   	 * strict typing.
>   	 */
>   	struct index *pk = space_index(tab->space, 0);
> -	uint32_t part_count = pk->def->key_def->part_count;
> -	if (part_count == 1) {
> -		uint32_t fieldno = pk->def->key_def->parts[0].fieldno;
> -		int reg_pk = new_tuple_reg + fieldno;
> -		if (def->fields[fieldno].affinity == AFFINITY_INTEGER) {
> -			int skip_if_null = sqlite3VdbeMakeLabel(v);
> -			if (autoinc_fieldno != UINT32_MAX) {
> -				sqlite3VdbeAddOp2(v, OP_IsNull, reg_pk,
> -						  skip_if_null);
> +	if (pk != NULL) {
> +		uint32_t part_count = pk->def->key_def->part_count;
> +		if (part_count == 1) {
> +			uint32_t fieldno = pk->def->key_def->parts[0].fieldno;
> +			int reg_pk = new_tuple_reg + fieldno;
> +			if (def->fields[fieldno].affinity == AFFINITY_INTEGER) {
> +				int skip_if_null = sqlite3VdbeMakeLabel(v);
> +				if (autoinc_fieldno != UINT32_MAX) {
> +					sqlite3VdbeAddOp2(v, OP_IsNull, reg_pk,
> +							  skip_if_null);
> +				}
> +				sqlite3VdbeAddOp2(v, OP_MustBeInt, reg_pk, 0);
> +				sqlite3VdbeResolveLabel(v, skip_if_null);

I do not think such a fix is crashproof. It works only because sqlite3Insert
does not use pk before calling this function almost in the end. Or uses, but
ignores an error in other places, which are not tested yet. IMO we shall check
that a space is able to accept DML at the begin of sqlite3Insert.

It was not easy to do though since sqlite3Insert still heavily uses Table mixed
with struct space, so alongside with my review fix of this patch I slightly
reduced struct Table usage in this function.

Also, after my refactoring it became obvious that space_column_default_expr()
function can be removed, so I did it in a next commit.

==============================================================================

commit 5f2a30deab7c99754ae674542102b32cd8d4d6b9
Author: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Date:   Wed Nov 14 13:52:24 2018 +0300

     Review fixes

diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index 45f59b119..01bffa44d 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);
  	}
  
@@ -983,20 +982,18 @@ vdbe_emit_constraint_checks(struct Parse *parse_context, struct Table *tab,
  	 * strict typing.
  	 */
  	struct index *pk = space_index(tab->space, 0);
-	if (pk != NULL) {
-		uint32_t part_count = pk->def->key_def->part_count;
-		if (part_count == 1) {
-			uint32_t fieldno = pk->def->key_def->parts[0].fieldno;
-			int reg_pk = new_tuple_reg + fieldno;
-			if (def->fields[fieldno].affinity == AFFINITY_INTEGER) {
-				int skip_if_null = sqlite3VdbeMakeLabel(v);
-				if (autoinc_fieldno != UINT32_MAX) {
-					sqlite3VdbeAddOp2(v, OP_IsNull, reg_pk,
-							  skip_if_null);
-				}
-				sqlite3VdbeAddOp2(v, OP_MustBeInt, reg_pk, 0);
-				sqlite3VdbeResolveLabel(v, skip_if_null);
+	uint32_t part_count = pk->def->key_def->part_count;
+	if (part_count == 1) {
+		uint32_t fieldno = pk->def->key_def->parts[0].fieldno;
+		int reg_pk = new_tuple_reg + fieldno;
+		if (def->fields[fieldno].affinity == AFFINITY_INTEGER) {
+			int skip_if_null = sqlite3VdbeMakeLabel(v);
+			if (autoinc_fieldno != UINT32_MAX) {
+				sqlite3VdbeAddOp2(v, OP_IsNull, reg_pk,
+						  skip_if_null);
  			}
+			sqlite3VdbeAddOp2(v, OP_MustBeInt, reg_pk, 0);
+			sqlite3VdbeResolveLabel(v, skip_if_null);
  		}
  	}
  	/*

==============================================================================

commit 557093ecf46e741f1fa98f510082a078a2cc8140
Author: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Date:   Wed Nov 14 14:04:38 2018 +0300

     sql: remove space_column_default_expr()

diff --git a/src/box/sql.c b/src/box/sql.c
index caa66144f..98c4ebd97 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 ec6d9d3cc..1c4841716 100644
--- a/src/box/sql.h
+++ b/src/box/sql.h
@@ -161,17 +161,6 @@ sql_trigger_space_id(struct sql_trigger *trigger);
  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.
diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index 01bffa44d..6f7720201 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;