[tarantool-patches] [PATCH v2 1/1] sql: cleanup on failed creation operation

[imeevma@tarantool.org]

Some creation operations create objects even on fail. This is
wrong and should be fixed. This patch adds destructors for such
objects.

Closes #3592
---
Branch: https://github.com/tarantool/tarantool/tree/imeevma/gh-3592-cleanup-on-failed-create
Issue: https://github.com/tarantool/tarantool/issues/3592

 src/box/sql/build.c          | 86 +++++++++++++++++++++++++++++++++++++++++++-
 src/box/sql/sqliteInt.h      |  4 +++
 src/box/sql/vdbe.c           | 55 ++++++++++++++++++++++++----
 test/sql/drop-table.result   | 38 ++++++++++++++++++++
 test/sql/drop-table.test.lua | 19 ++++++++++
 5 files changed, 195 insertions(+), 7 deletions(-)

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index a1e16b2..1b30191 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -55,6 +55,61 @@
 #include "box/tuple_format.h"
 #include "box/coll_id_cache.h"
 
+/**
+ * Last element of list that contains some information about
+ * records which were inserted into system spaces.
+ */
+static struct saved_records
+{
+	/* Pointer on previous element of the list. */
+	struct saved_records *prev;
+	/* Id of space in which record were inserted. */
+	int space_id;
+	/* First register of key of record. */
+	int record_key;
+	/* Length of key of record. */
+	int record_key_len;
+	/* Register with table name.*/
+	int record_name;
+	/* Flags of this element of list. */
+	int flag;
+} *saved_records = NULL;
+
+/**
+ * Save inserted in system space record in list saved_records.
+ *
+ * @param parser SQL Parser object.
+ * @param space_id Id of table in which record is inserted.
+ * @param record_key Register that contains first field of key.
+ * @param record_key_len Exact number of fields of key.
+ * @param record_name Register contains name of table which were
+ *        created when record was inserted into box.space._space.
+ * @param flag OPFLAG_NCHANGE or 0.
+ * @retval -1 memory error.
+ * @retval 0 success.
+ */
+static inline void
+save_record(Parse *parser, int space_id, int record_key, int record_key_len,
+	    int record_name, int flag)
+{
+	struct saved_records *tmp = malloc(sizeof(*tmp));
+	if (tmp == NULL) {
+		diag_set(OutOfMemory, sizeof(*tmp), "malloc", "saved_records");
+		parser->rc = SQL_TARANTOOL_ERROR;
+		parser->nErr++;
+		return;
+	}
+	tmp->prev = saved_records;
+	tmp->space_id = space_id;
+	tmp->record_key = record_key;
+	tmp->record_key_len = record_key_len;
+	tmp->record_name = record_name;
+	tmp->flag = flag | OPFLAG_DESTRUCTOR;
+	if (record_name != 0)
+		tmp->flag |= OPFLAG_CLEAR_HASH;
+	saved_records = tmp;
+}
+
 void
 sql_finish_coding(struct Parse *parse_context)
 {
@@ -62,6 +117,22 @@ sql_finish_coding(struct Parse *parse_context)
 	struct sqlite3 *db = parse_context->db;
 	struct Vdbe *v = sqlite3GetVdbe(parse_context);
 	sqlite3VdbeAddOp0(v, OP_Halt);
+	/**
+	 * Destructors for all created in current statement
+	 * spaces, indexes, sequences etc.
+	 */
+	while (saved_records != NULL) {
+		int record_reg = ++parse_context->nMem;
+		sqlite3VdbeAddOp3(v, OP_MakeRecord, saved_records->record_key,
+				  saved_records->record_key_len, record_reg);
+		sqlite3VdbeAddOp3(v, OP_SDelete, saved_records->space_id,
+				  record_reg, saved_records->record_name);
+		sqlite3VdbeChangeP5(v, saved_records->flag);
+		struct saved_records *tmp = saved_records;
+		saved_records = saved_records->prev;
+		free(tmp);
+	}
+	sqlite3VdbeAddOp0(v, OP_Error);
 	if (db->mallocFailed || parse_context->nErr != 0) {
 		if (parse_context->rc == SQLITE_OK)
 			parse_context->rc = SQLITE_ERROR;
@@ -1240,8 +1311,13 @@ createIndex(Parse * pParse, Index * pIndex, int iSpaceId, int iIndexId,
 	 * Non-NULL value means that index has been created via
 	 * separate CREATE INDEX statement.
 	 */
-	if (zSql != NULL)
+	if (zSql != NULL) {
 		sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE);
+		save_record(pParse, BOX_INDEX_ID, iFirstCol, 2, 0,
+			    OPFLAG_NCHANGE);
+	} else {
+		save_record(pParse, BOX_INDEX_ID, iFirstCol, 2, 0, 0);
+	}
 	return;
 error:
 	pParse->rc = SQL_TARANTOOL_ERROR;
@@ -1344,6 +1420,8 @@ createSpace(Parse * pParse, int iSpaceId, char *zStmt)
 	sqlite3VdbeAddOp3(v, OP_MakeRecord, iFirstCol, 7, iRecord);
 	sqlite3VdbeAddOp2(v, OP_SInsert, BOX_SPACE_ID, iRecord);
 	sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE);
+	save_record(pParse, BOX_SPACE_ID, iFirstCol, 1, iFirstCol + 2,
+		    OPFLAG_NCHANGE);
 	return;
 error:
 	pParse->nErr++;
@@ -1550,6 +1628,8 @@ vdbe_emit_fkey_create(struct Parse *parse_context, const struct fkey_def *fk)
 	sqlite3VdbeAddOp2(vdbe, OP_SInsert, BOX_FK_CONSTRAINT_ID,
 			  constr_tuple_reg + 9);
 	sqlite3VdbeChangeP5(vdbe, OPFLAG_NCHANGE);
+	save_record(parse_context, BOX_FK_CONSTRAINT_ID, constr_tuple_reg, 2, 0,
+		    OPFLAG_NCHANGE);
 	sqlite3ReleaseTempRange(parse_context, constr_tuple_reg, 10);
 	return;
 error:
@@ -1732,12 +1812,16 @@ sqlite3EndTable(Parse * pParse,	/* Parse context */
 						 p->def->name);
 		sqlite3VdbeAddOp2(v, OP_SInsert, BOX_SEQUENCE_ID,
 				  reg_seq_record);
+		save_record(pParse, BOX_SEQUENCE_ID, reg_seq_record + 1, 1, 0,
+			    0);
 		/* Do an insertion into _space_sequence. */
 		int reg_space_seq_record =
 			emitNewSysSpaceSequenceRecord(pParse, reg_space_id,
 						      reg_seq_id);
 		sqlite3VdbeAddOp2(v, OP_SInsert, BOX_SPACE_SEQUENCE_ID,
 				  reg_space_seq_record);
+		save_record(pParse, BOX_SPACE_SEQUENCE_ID,
+			    reg_space_seq_record + 1, 1, 0, 0);
 	}
 
 	/*
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index 1d32c9a..fc479b1 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -2843,6 +2843,10 @@ struct Parse {
 #define OPFLAG_PERMUTE       0x01	/* OP_Compare: use the permutation */
 #define OPFLAG_SAVEPOSITION  0x02	/* OP_Delete: keep cursor position */
 #define OPFLAG_AUXDELETE     0x04	/* OP_Delete: index in a DELETE op */
+/* OP_SDelete: work as destructor. */
+#define OPFLAG_DESTRUCTOR    0x02
+/* OP_SDelete: remove table from tblHash. */
+#define OPFLAG_CLEAR_HASH    0x04
 
 #define OPFLAG_SAME_FRAME    0x01	/* OP_FCopy: use same frame for source
 					 * register
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index a5b907d..595d7d4 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -610,6 +610,17 @@ int sqlite3VdbeExec(Vdbe *p)
 	u64 start;                 /* CPU clock count at start of opcode */
 #endif
 	struct session *user_session = current_session();
+	/*
+	 * Opcode to where we should jump on error in OP_SInsert.
+	 */
+	int op_clean_on_error = p->nOp - 1;
+	/*
+	 * On error in OP_SInsert rc is saved here before
+	 * clean-up. After clean-up this values is retrieved and
+	 * rc set. If error happened during clean-up this value
+	 * neglected.
+	 */
+	int saved_rc = 0;
 	/*** INSERT STACK UNION HERE ***/
 
 	assert(p->magic==VDBE_MAGIC_RUN);  /* sqlite3_step() verifies this */
@@ -663,10 +674,14 @@ int sqlite3VdbeExec(Vdbe *p)
 	sqlite3EndBenignMalloc();
 #endif
 	for(pOp=&aOp[p->pc]; 1; pOp++) {
-		/* Errors are detected by individual opcodes, with an immediate
-		 * jumps to abort_due_to_error.
+		/**
+		 * Errors are detected by individual opcodes, with
+		 * an immediate jumps to abort_due_to_error. Only
+		 * exception - opcode OP_SInsert in which on error
+		 * jump should be on jump_to_destructors. In this
+		 * case rc saved in saved_rc.
 		 */
-		assert(rc==SQLITE_OK);
+		assert(rc==SQLITE_OK || saved_rc != 0);
 
 		assert(pOp>=aOp && pOp<&aOp[p->nOp]);
 #ifdef VDBE_PROFILE
@@ -845,6 +860,21 @@ case OP_Gosub: {            /* jump */
 			jump_to_p2:
 	pOp = &aOp[pOp->p2 - 1];
 	break;
+			jump_to_destructors:
+	saved_rc = rc;
+	pOp = &aOp[op_clean_on_error - 1];
+	break;
+}
+
+/* Opcode:  Error * * * * *
+ *
+ * Retrieve saved rc and jump to abort_due_to_error. Should be
+ * located after destructors of created in current statement
+ * spaces, indexes etc.
+ */
+case OP_Error: {
+	rc = saved_rc;
+	goto abort_due_to_error;
 }
 
 /* Opcode:  Return P1 * * * *
@@ -4345,9 +4375,13 @@ case OP_SInsert: {
 	assert(space_is_system(space));
 	rc = tarantoolSqlite3Insert(space, pIn2->z, pIn2->z + pIn2->n);
 	if (rc)
-		goto abort_due_to_error;
+		goto jump_to_destructors;
 	if (pOp->p5 & OPFLAG_NCHANGE)
 		p->nChange++;
+	/* OP_SDelete - destructors of newly created record. */
+	op_clean_on_error--;
+	/* OP_MakeRecord - make key for destructor. */
+	op_clean_on_error--;
 	break;
 }
 
@@ -4371,8 +4405,17 @@ case OP_SDelete: {
 	rc = sql_delete_by_key(space, 0, pIn2->z, pIn2->n);
 	if (rc)
 		goto abort_due_to_error;
-	if (pOp->p5 & OPFLAG_NCHANGE)
-		p->nChange++;
+	if ((pOp->p5 & OPFLAG_DESTRUCTOR) == 0) {
+		if (pOp->p5 & OPFLAG_NCHANGE)
+			p->nChange++;
+	} else {
+		if ((pOp->p5 & OPFLAG_CLEAR_HASH) != 0) {
+			pIn3 = &aMem[pOp->p3];
+			sqlite3HashInsert(&db->pSchema->tblHash, pIn3->z, 0);
+		}
+		if (pOp->p5 & OPFLAG_NCHANGE)
+			p->nChange--;
+	}
 	break;
 }
 
diff --git a/test/sql/drop-table.result b/test/sql/drop-table.result
index 08f2496..e0349cc 100644
--- a/test/sql/drop-table.result
+++ b/test/sql/drop-table.result
@@ -33,3 +33,41 @@ box.sql.execute("INSERT INTO zzzoobar VALUES (111, 222, 'c3', 444)")
 -- DROP TABLE should do the job
 -- Debug
 -- require("console").start()
+--
+-- gh-3592: segmentation fault when table with error during
+-- creation is dropped.
+--
+box.schema.user.create('tmp')
+---
+...
+box.schema.user.grant('tmp', 'create', 'space')
+---
+...
+box.schema.user.grant('tmp', 'write', 'space', '_space')
+---
+...
+box.schema.user.grant('tmp', 'write', 'space', '_schema')
+---
+...
+box.session.su('tmp')
+---
+...
+--
+-- Error: user do not have rights to write in box.space._index.
+-- Space that was already created should be automatically dropped.
+--
+box.sql.execute('create table t1 (id int primary key, a int)')
+---
+- error: Write access to space '_index' is denied for user 'tmp'
+...
+-- Error: no such table.
+box.sql.execute('drop table t1')
+---
+- error: 'no such table: T1'
+...
+box.session.su('admin')
+---
+...
+box.schema.user.drop('tmp')
+---
+...
diff --git a/test/sql/drop-table.test.lua b/test/sql/drop-table.test.lua
index 9663074..7a27416 100644
--- a/test/sql/drop-table.test.lua
+++ b/test/sql/drop-table.test.lua
@@ -25,3 +25,22 @@ box.sql.execute("INSERT INTO zzzoobar VALUES (111, 222, 'c3', 444)")
 
 -- Debug
 -- require("console").start()
+
+--
+-- gh-3592: segmentation fault when table with error during
+-- creation is dropped.
+--
+box.schema.user.create('tmp')
+box.schema.user.grant('tmp', 'create', 'space')
+box.schema.user.grant('tmp', 'write', 'space', '_space')
+box.schema.user.grant('tmp', 'write', 'space', '_schema')
+box.session.su('tmp')
+--
+-- Error: user do not have rights to write in box.space._index.
+-- Space that was already created should be automatically dropped.
+--
+box.sql.execute('create table t1 (id int primary key, a int)')
+-- Error: no such table.
+box.sql.execute('drop table t1')
+box.session.su('admin')
+box.schema.user.drop('tmp')
-- 
2.7.4