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 65F392FFDC for ; Thu, 15 Nov 2018 08:47:47 -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 m6r3CzAGEmWq for ; Thu, 15 Nov 2018 08:47:47 -0500 (EST) Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 AE3832FF81 for ; Thu, 15 Nov 2018 08:47:46 -0500 (EST) Content-Type: text/plain; charset=us-ascii Mime-Version: 1.0 (Mac OS X Mail 12.0 \(3445.100.39\)) Subject: [tarantool-patches] Re: [PATCH v2 1/2] sql: proper check for index in sqlite3Insert() From: "n.pettik" In-Reply-To: <05acfbeed0d0a02a349aa1ceb5a44624ff333060.1542282499.git.imeevma@gmail.com> Date: Thu, 15 Nov 2018 16:47:44 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <6B17B69F-F470-4010-8B60-B7233317C060@tarantool.org> References: <05acfbeed0d0a02a349aa1ceb5a44624ff333060.1542282499.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 Cc: Imeev Mergen , Vladislav Shpilevoy > On 15 Nov 2018, at 15:09, imeevma@tarantool.org wrote: >=20 > 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... >=20 > 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(-) >=20 > @@ -315,24 +314,21 @@ sqlite3Insert(Parse * pParse, /* Parser = context */ > if (pTab =3D=3D NULL) > goto insert_cleanup; >=20 > - space_id =3D pTab->def->id; > + struct space *space =3D pTab->space; > + struct space_def *def =3D space->def; > + space_id =3D 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 =3D 0; /* One register allocated to each = index */ - uint32_t space_id =3D 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 */ =20 struct space *space =3D pTab->space; struct space_def *def =3D space->def; - space_id =3D def->id; + uint32_t space_id =3D def->id; >=20 > /* Figure out if we have any triggers and if the table being > * inserted into is a view > */ > trigger =3D sql_triggers_exist(pTab, TK_INSERT, NULL, &tmask); > - bool is_view =3D pTab->def->opts.is_view; > + bool is_view =3D def->opts.is_view; > assert((trigger !=3D NULL && tmask !=3D 0) || > (trigger =3D=3D NULL && tmask =3D=3D 0)); >=20 > - /* 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) !=3D 0) > + if (is_view && sql_view_assign_cursors(pParse, def->opts.sql) !=3D= 0) > goto insert_cleanup; >=20 > - struct space_def *def =3D pTab->def; > /* Cannot insert into a read-only table. */ > if (is_view && tmask =3D=3D 0) { > sqlite3ErrorMsg(pParse, "cannot modify %s because it is = a view", > @@ -340,6 +336,12 @@ sqlite3Insert(Parse * pParse, /* Parser = context */ > goto insert_cleanup; > } >=20 > + if (! is_view && index_find(space, 0) =3D=3D NULL) { > + pParse->nErr++; > + pParse->rc =3D 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.