[tarantool-patches] Re: [PATCH v2] sql: prohibit negative values under LIMIT clause

[n.pettik]

> On 5 Feb 2019, at 22:51, Stanislav Zudin <szudin at tarantool.org> wrote:
> 
> The remarks are applied in the branch:
> https://github.com/tarantool/tarantool/tree/stanztt/gh-3467-prohibit-negative-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
> 
> 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'})
>> 
> 
>