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 AA68F2686E for ; Tue, 5 Feb 2019 14:52:00 -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 dJ4XeU5lcLvC for ; Tue, 5 Feb 2019 14:52:00 -0500 (EST) Received: from smtp60.i.mail.ru (smtp60.i.mail.ru [217.69.128.40]) (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 68CA426611 for ; Tue, 5 Feb 2019 14:52:00 -0500 (EST) Subject: [tarantool-patches] Re: [PATCH v2] sql: prohibit negative values under LIMIT clause References: <20190205085636.19053-1-szudin@tarantool.org> <90FE85BE-9EB8-425F-B360-F5DDC5477AB1@tarantool.org> From: Stanislav Zudin Message-ID: Date: Tue, 5 Feb 2019 22:51:57 +0300 MIME-Version: 1.0 In-Reply-To: <90FE85BE-9EB8-425F-B360-F5DDC5477AB1@tarantool.org> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit Content-Language: en-US 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, "n.pettik" The remarks are applied in the branch: https://github.com/tarantool/tarantool/tree/stanztt/gh-3467-prohibit-negative-limits Issue: https://github.com/tarantool/tarantool/issues/3467 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. >> >> 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’t 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 = 0; >> int iLimit = 0; >> int iOffset; >> - int n; >> + const char *negativeLimitError = "Only positive numbers are allowed " >> + "in the LIMIT clause”; > 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 > … > > 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; >> >> @@ -2097,24 +2098,29 @@ computeLimitRegisters(Parse * pParse, Select * p, int iBreak) >> p->iLimit = iLimit = ++pParse->nMem; >> v = sqlite3GetVdbe(pParse); >> assert(v != 0); >> - if (sqlite3ExprIsInteger(p->pLimit, &n)) { >> - sqlite3VdbeAddOp2(v, OP_Integer, n, iLimit); >> - VdbeComment((v, "LIMIT counter")); >> - if (n == 0) { >> - sqlite3VdbeGoto(v, iBreak); >> - } else if (n >= 0 >> - && p->nSelectRow > sqlite3LogEst((u64) n)) { >> - p->nSelectRow = sqlite3LogEst((u64) n); >> - p->selFlags |= 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 = sqlite3VdbeMakeLabel(v); >> + int labelHalt = 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; > 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 >= 0 continue execution */ >> + int r1 = 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); >> + >> >> 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 = ?', {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'}) >