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 BDF4F28818 for ; Wed, 14 Nov 2018 06:07:35 -0500 (EST) 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 yj9HYJrqunF5 for ; Wed, 14 Nov 2018 06:07:35 -0500 (EST) Received: from smtp10.mail.ru (smtp10.mail.ru [94.100.181.92]) (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 5776A254CB for ; Wed, 14 Nov 2018 06:07:35 -0500 (EST) Subject: [tarantool-patches] Re: [PATCH v1 1/1] sql: proper check for index in vdbe_emit_constraint_checks() References: <99a387b77e9908ce686be3d86e1f2045d6482221.1541841136.git.imeevma@gmail.com> From: Vladislav Shpilevoy Message-ID: Date: Wed, 14 Nov 2018 14:07:31 +0300 MIME-Version: 1.0 In-Reply-To: <99a387b77e9908ce686be3d86e1f2045d6482221.1541841136.git.imeevma@gmail.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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: imeevma@tarantool.org, tarantool-patches@freelists.org 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 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 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;