[tarantool-patches] Re: [PATCH] sql: do not allow oversized integer literals

[Kirill Yukhin]

Hello Nikita,
On 18 июл 20:56, n.pettik wrote:
> Except for two minor code-style remarks patch LGTM.
I've fixed/commented most of your remarks and checked
in the patch into 2.0 branch.

> > static void
> > -codeInteger(Parse * pParse, Expr * pExpr, int negFlag, int iMem)
> > +expr_code_int(struct Parse *parse, struct Expr *expr, bool is_neg,
> > -		    || (negFlag && value == SMALLEST_INT64)) {
> > -#ifdef SQLITE_OMIT_FLOATING_POINT
> > -			sqlite3ErrorMsg(pParse, "oversized integer: %s%s",
> > -					negFlag ? "-" : "", z);
> > -#else
> > -#ifndef SQLITE_OMIT_HEX_INTEGER
> > +		int64_t value;
> > +		const char *z = expr->u.zToken;
> > +		assert(z != NULL);
> > +		int c = sql_dec_or_hex_to_i64(z, &value);
> > +		if (c == 1 || (c == 2 && !is_neg)
> > +		    || (is_neg && value == SMALLEST_INT64)) {
> Put binary operator to line above:
> 
> if (A &&
>    B…
Done.

> > diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
> > index 18bf949..4a972a1 100644
> > --- a/src/box/sql/sqliteInt.h
> > +++ b/src/box/sql/sqliteInt.h
> > @@ -4354,8 +4354,53 @@ char
> > sqlite3TableColumnAffinity(struct space_def *def, int idx);
> > 
> > char sqlite3ExprAffinity(Expr * pExpr);
> > -int sqlite3Atoi64(const char *, i64 *, int);
> > -int sqlite3DecOrHexToI64(const char *, i64 *);
> > +
> > +
> > +/**
> > + * Convert z to a 64-bit signed integer.  z must be decimal. This
> > + * routine does *not* accept hexadecimal notation.
> > + *
> > + * If the z value is representable as a 64-bit twos-complement
> > + * integer, then write that value into *val and return 0.
> > + *
> > + * If z is exactly 9223372036854775808, return 2.  This special
> > + * case is broken out because while 9223372036854775808 cannot be a
> > + * signed 64-bit integer, its negative -9223372036854775808 can be.
> > + *
> > + * If z is too big for a 64-bit integer and is not
> > + * 9223372036854775808  or if z contains any non-numeric text,
> > + * then return 1.
> > + *
> > + * length is the number of bytes in the string (bytes, not characters).
> > + * The string is not necessarily zero-terminated.  The encoding is
> > + * given by enc.
> > + *
> > + * @param z String being parsed.
> > + * @param[out] val Output integer value.
> > + * @param length String length in bytes.
> > + * @retval
> > + *     0    Successful transformation.  Fits in a 64-bit signed integer.
> > + *     1    Integer too large for a 64-bit signed integer or is malformed
> > + *     2    Special case of 9223372036854775808
> > + */
> 
> I still see that comment to this function violates code style..
> And comment to sql_dec_or_hex_to_i64 below too.
Checked.

> > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> > index f50e389..195638e 100644
> > --- a/src/box/sql/vdbe.c
> > +++ b/src/box/sql/vdbe.c
> > @@ -291,7 +291,7 @@ applyNumericAffinity(Mem *pRec, int bTryForInt)
> > 	i64 iValue;
> > 	assert((pRec->flags & (MEM_Str|MEM_Int|MEM_Real))==MEM_Str);
> > 	if (sqlite3AtoF(pRec->z, &rValue, pRec->n)==0) return;
> > -	if (0==sqlite3Atoi64(pRec->z, &iValue, pRec->n)) {
> > +	if (0 == sql_atoi64(pRec->z, (int64_t *)&iValue, pRec->n)) {
> 
> Are you sure this cast is not redundant? If not so, why don’t use int64_t as type of iValue?
Yeah, I've double checked:

In file included from /home/kyukhin/w/tarantool/src/src/box/sql/vdbe.c:44:0:
/home/kyukhin/w/tarantool/src/src/box/sql/sqliteInt.h:4390:1: замечание: expected «int64_t *» but argument is of type «i64 *»
 sql_atoi64(const char *z, int64_t *val, int length);

--
Regards, Kirill Yukhin