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 5B9D126974 for ; Tue, 5 Feb 2019 10:15:22 -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 AE7r6oH92mDp for ; Tue, 5 Feb 2019 10:15:22 -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 15F9D268C2 for ; Tue, 5 Feb 2019 10:15:22 -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: <20190205085636.19053-1-szudin@tarantool.org> Date: Tue, 5 Feb 2019 18:15:19 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <90FE85BE-9EB8-425F-B360-F5DDC5477AB1@tarantool.org> References: <20190205085636.19053-1-szudin@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 > 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. Below a few minor comments. Don=E2=80=99t send new version of patch, just answer to this letter with provided fixes. > 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). One more nit: select * from t1 limit 0.5; --- - error: Only positive numbers are allowed in the LIMIT clause =E2=80=A6 However, 0.5 is a positive number. Lets re-phrase error message a bit. Like: Only positive integers are allowed in the LIMIT clause > 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: int IPosLimitValue -> positive_limit_label;=20 int labelHalt -> int halt_label; The same for code concerning LIMIT clause. (Yep, it is kind of messing code styles, but we decided to use our codestyle in freshly added/refactored code). > + 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. > +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'})