From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 34A822ABD4 for ; Fri, 7 Sep 2018 15:04:12 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id D3KD2w3onqDn for ; Fri, 7 Sep 2018 15:04:12 -0400 (EDT) Received: from smtp29.i.mail.ru (smtp29.i.mail.ru [94.100.177.89]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 4E8932AAF2 for ; Fri, 7 Sep 2018 15:04:11 -0400 (EDT) From: imeevma@tarantool.org Subject: [tarantool-patches] [PATCH v2 1/1] sql: cleanup on failed creation operation Date: Fri, 7 Sep 2018 22:04:07 +0300 Message-Id: <433c416428f1ef575324236be22c27962db3f8b5.1536346889.git.imeevma@gmail.com> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org, v.shpilevoy@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 | 109 ++++++++++++++++++++++++++++++++++++++++++- 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, 218 insertions(+), 7 deletions(-) diff --git a/src/box/sql/build.c b/src/box/sql/build.c index a1e16b2..96d0624 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -54,6 +54,78 @@ #include "box/schema.h" #include "box/tuple_format.h" #include "box/coll_id_cache.h" +#include "fiber.h" + +/** + * Element of list that contains some information about records + * which were inserted into system spaces. + */ +struct saved_records +{ + /* A link in record list. */ + struct rlist link; + /* 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; +}; + +/** + * List of all records that were inserted in system spaces in + * current statement. + */ +static struct rlist record_list; +/** + * Flag to show if there were any saved records in current + * statement. + */ +static bool is_records_saved = false; + +/** + * 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) +{ + if (!is_records_saved) { + rlist_create(&record_list); + is_records_saved = true; + } + struct saved_records *saved_records = + region_alloc(&fiber()->gc, sizeof(*saved_records)); + if (saved_records == NULL) { + diag_set(OutOfMemory, sizeof(*saved_records), "region_alloc", + "saved_records"); + parser->rc = SQL_TARANTOOL_ERROR; + parser->nErr++; + return; + } + saved_records->space_id = space_id; + saved_records->record_key = record_key; + saved_records->record_key_len = record_key_len; + saved_records->record_name = record_name; + saved_records->flag = flag | OPFLAG_DESTRUCTOR; + if (record_name != 0) + saved_records->flag |= OPFLAG_CLEAR_HASH; + rlist_add_entry(&record_list, saved_records, link); +} void sql_finish_coding(struct Parse *parse_context) @@ -62,6 +134,28 @@ 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. + */ + if (is_records_saved) { + while (rlist_empty(&record_list) == 0) { + int record_reg = ++parse_context->nMem; + struct saved_records *saved_records = + rlist_shift_entry(&record_list, + struct saved_records, link); + 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); + } + } + sqlite3VdbeAddOp0(v, OP_Error); if (db->mallocFailed || parse_context->nErr != 0) { if (parse_context->rc == SQLITE_OK) parse_context->rc = SQLITE_ERROR; @@ -1240,8 +1334,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 +1443,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 +1651,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 +1835,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