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

[n.pettik]

Except for two minor code-style remarks patch LGTM.

> I think, that this minor change affects visible behaviour. So, let's keep
> tests for such changes in separate files.

As you wish. But I still stick to the point that separate file is ‘over-kill'.

> @@ -3259,51 +3258,48 @@ codeReal(Vdbe * v, const char *z, int negateFlag, int iMem)
> }
> #endif
> 
> -/*
> +/**
>  * Generate an instruction that will put the integer describe by
>  * text z[0..n-1] into register iMem.
>  *
> - * Expr.u.zToken is always UTF8 and zero-terminated.
> + * @param parse Parsing context.
> + * @param expr Expression being parsed. Expr.u.zToken is always
> + *             UTF8 and zero-terminated.
> + * @param neg_flag True if value is negative.
> + * @param mem Register to store parsed integer
>  */
> static void
> -codeInteger(Parse * pParse, Expr * pExpr, int negFlag, int iMem)
> +expr_code_int(struct Parse *parse, struct Expr *expr, bool is_neg,
> +	      int mem)
> {
> -	Vdbe *v = pParse->pVdbe;
> -	if (pExpr->flags & EP_IntValue) {
> -		int i = pExpr->u.iValue;
> +	struct Vdbe *v = parse->pVdbe;
> +	if (expr->flags & EP_IntValue) {
> +		int i = expr->u.iValue;
> 		assert(i >= 0);
> -		if (negFlag)
> +		if (is_neg)
> 			i = -i;
> -		sqlite3VdbeAddOp2(v, OP_Integer, i, iMem);
> +		sqlite3VdbeAddOp2(v, OP_Integer, i, mem);
> 	} else {
> -		int c;
> -		i64 value;
> -		const char *z = pExpr->u.zToken;
> -		assert(z != 0);
> -		c = sqlite3DecOrHexToI64(z, &value);
> -		if (c == 1 || (c == 2 && !negFlag)
> -		    || (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…

> 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.

> 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?

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20180718/bf966500/attachment.html>