[tarantool-patches] Re: [PATCH v2 1/2] sql: proper check for index in sqlite3Insert()

[n.pettik]

> On 15 Nov 2018, at 15:09, imeevma at tarantool.org wrote:
> 
> Index received in function sqlite3Insert() wasn't checked
> properly. It lead to segmentation fault when INSERT and

Typo: led (or leads).

> DROP TABLE executed simultaneously for the same table.

This particular fix seems to be OK, but what about other cases?
For example executing different SELECTs when table/index is dropped,
or executing deletes/updates, or executing INSERT with different riggers
or foreign keys and so on and so forth.

Could you please spend some time to deeply investigate this issue?
I am afraid that SQL in its current state is unlikely to be adopted to
such emergencies...

> 
> Closes #3780
> ---
> src/box/sql/insert.c     | 49 ++++++++++++++++++++++++------------------------
> test/sql/errinj.result   | 33 ++++++++++++++++++++++++++++++++
> test/sql/errinj.test.lua | 12 ++++++++++++
> 3 files changed, 69 insertions(+), 25 deletions(-)
> 
> @@ -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;

Small refactoring:

diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index 6f7720201..235be2b9f 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -281,7 +281,6 @@ sqlite3Insert(Parse * pParse,       /* Parser context */
        int regTupleid;         /* registers holding insert tupleid */
        int regData;            /* register holding first column to insert */
        int *aRegIdx = 0;       /* One register allocated to each index */
-       uint32_t space_id = 0;
        /* List of triggers on pTab, if required. */
        struct sql_trigger *trigger;
        int tmask;              /* Mask of trigger times */
@@ -316,7 +315,7 @@ sqlite3Insert(Parse * pParse,       /* Parser context */
 
        struct space *space = pTab->space;
        struct space_def *def = space->def;
-       space_id = def->id;
+       uint32_t 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;
> +	}

Please, put explanation comment here. In a few weeks everyone
will forget about this bug, so this snippet will look quite strange.

Also, lets separate refactoring (in our case substituting usage of
struct Table with struct space) and functional changes (validating PK)
and put them into different commits (not now but for the next patches).
I know that in SQL patches as a rule they come together since there are
a lot of codestyle violations, but it makes reviewing process MUCH more easier.