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

[Stanislav Zudin <szudin@tarantool.org>]

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'})
>