[tarantool-patches] [PATCH v2 1/1] sql: cleanup on failed creation operation
imeevma at tarantool.org
imeevma at tarantool.org
Thu Sep 6 17:22:39 MSK 2018
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
More information about the Tarantool-patches
mailing list