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 DAB6926256 for ; Wed, 6 Feb 2019 08:25:06 -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 sXTzFPVq1MSo for ; Wed, 6 Feb 2019 08:25:06 -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 81FFA20D81 for ; Wed, 6 Feb 2019 08:25:06 -0500 (EST) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 12.2 \(3445.102.3\)) Subject: [tarantool-patches] Re: [PATCH v2] sql: prohibit negative values under LIMIT clause From: "n.pettik" In-Reply-To: Date: Wed, 6 Feb 2019 16:25:04 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: References: <20190205085636.19053-1-szudin@tarantool.org> <90FE85BE-9EB8-425F-B360-F5DDC5477AB1@tarantool.org> 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: szudin@tarantool.org > On 5 Feb 2019, at 22:51, Stanislav Zudin wrote: >=20 > The remarks are applied in the branch: > = https://github.com/tarantool/tarantool/tree/stanztt/gh-3467-prohibit-negat= ive-limits Ok, but next time please attach diff (between old and new version) right to the letter (but not send it as version++). LGTM. > Issue: https://github.com/tarantool/tarantool/issues/3467 >=20 > On 05.02.2019 18:15, n.pettik wrote: >>> If LIMIT or OFFSET expressions can be casted to the >>> negative integer value VDBE returns an error. >>> If expression in the LIMIT clause can't be converted >>> into integer without data loss the VDBE instead of >>> SQLITE_MISMATCH returns SQL_TARANTOOL_ERROR with >>> message "Only positive numbers are allowed in the >>> LIMIT clause". The same for OFFSET clause. >>>=20 >>> Closes #3467 >> Fix please commit message subject: now not only negative >> values are rejected, but everything except for positive integers. >>=20 >> Below a few minor comments. Don=E2=80=99t send new version of patch, >> just answer to this letter with provided fixes. >>=20 >>> diff --git a/src/box/sql/select.c b/src/box/sql/select.c >>> index 02ee225f1..a8ed3c346 100644 >>> --- a/src/box/sql/select.c >>> +++ b/src/box/sql/select.c >>> @@ -2074,7 +2074,8 @@ computeLimitRegisters(Parse * pParse, Select * = p, int iBreak) >>> Vdbe *v =3D 0; >>> int iLimit =3D 0; >>> int iOffset; >>> - int n; >>> + const char *negativeLimitError =3D "Only positive numbers are = allowed " >>> + "in the LIMIT clause=E2=80=9D; >> Nit: now this variable is used only once, so I suggest to inline it >> or at least move closer to its usage. SQLite uses very ancient >> way of declaring variables at the beginning of function >> (I guess it is due to C 89 standard). >>=20 >> One more nit: >>=20 >> select * from t1 limit 0.5; >> --- >> - error: Only positive numbers are allowed in the LIMIT clause >> =E2=80=A6 >>=20 >> However, 0.5 is a positive number. Lets re-phrase error message a = bit. >> Like: Only positive integers are allowed in the LIMIT clause >>=20 >>> if (p->iLimit) >>> return; >>>=20 >>> @@ -2097,24 +2098,29 @@ computeLimitRegisters(Parse * pParse, Select = * p, int iBreak) >>> p->iLimit =3D iLimit =3D ++pParse->nMem; >>> v =3D sqlite3GetVdbe(pParse); >>> assert(v !=3D 0); >>> - if (sqlite3ExprIsInteger(p->pLimit, &n)) { >>> - sqlite3VdbeAddOp2(v, OP_Integer, n, iLimit); >>> - VdbeComment((v, "LIMIT counter")); >>> - if (n =3D=3D 0) { >>> - sqlite3VdbeGoto(v, iBreak); >>> - } else if (n >=3D 0 >>> - && p->nSelectRow > = sqlite3LogEst((u64) n)) { >>> - p->nSelectRow =3D sqlite3LogEst((u64) = n); >>> - p->selFlags |=3D SF_FixedLimit; >>> - } >>> - } else { >>> - sqlite3ExprCode(pParse, p->pLimit, iLimit); >>> - sqlite3VdbeAddOp1(v, OP_MustBeInt, iLimit); >>> - VdbeCoverage(v); >>> - VdbeComment((v, "LIMIT counter")); >>> - sqlite3VdbeAddOp2(v, OP_IfNot, iLimit, iBreak); >>> - VdbeCoverage(v); >>> - } >>> + int lPosLimitValue =3D sqlite3VdbeMakeLabel(v); >>> + int labelHalt =3D sqlite3VdbeMakeLabel(v); >> Could you please rename these two variables according to our >> code style (as I pointed in previous review)? For example: >>=20 >> int IPosLimitValue -> positive_limit_label; >> int labelHalt -> int halt_label; >>=20 >> The same for code concerning LIMIT clause. >>=20 >> (Yep, it is kind of messing code styles, but we decided to use >> our codestyle in freshly added/refactored code). >>=20 >>> + sqlite3ExprCode(pParse, p->pLimit, iLimit); >>> + sqlite3VdbeAddOp2(v, OP_MustBeInt, iLimit, labelHalt); >>> + /* If LIMIT clause >=3D 0 continue execution */ >>> + int r1 =3D sqlite3GetTempReg(pParse); >>> + sqlite3VdbeAddOp2(v, OP_Integer, 0, r1); >>> + sqlite3VdbeAddOp3(v, OP_Ge, r1, lPosLimitValue, iLimit); >>> + /* Otherwise return an error and stop */ >>> + sqlite3VdbeResolveLabel(v, labelHalt); >>> + sqlite3VdbeAddOp4(v, OP_Halt, >>> + SQL_TARANTOOL_ERROR, >>> + 0, 0, >>> + negativeLimitError, >>> + P4_STATIC); >>> + >>> + sqlite3VdbeResolveLabel(v, lPosLimitValue); >>> + sqlite3ReleaseTempReg(pParse, r1); >>> + VdbeCoverage(v); >>> + VdbeComment((v, "LIMIT counter")); >>> + sqlite3VdbeAddOp2(v, OP_IfNot, iLimit, iBreak); >>> + VdbeCoverage(v); >>> + >>>=20 >>> diff --git a/test/sql/iproto.test.lua b/test/sql/iproto.test.lua >>> index 664090368..919ddcb3c 100644 >>> cn:execute('select * from test where id =3D ?', {1}) >> Add a short comment like: >> gh-3467: allow only positive integers under limit clause. >>=20 >>> +cn:execute('select * from test limit ?', {2}) >>> +cn:execute('select * from test limit ?', {-2}) >>> +cn:execute('select * from test limit ?', {2.7}) >>> +cn:execute('select * from test limit ?', {'Hello'}) >>=20 >=20 >=20