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

["n.pettik" <korablev@tarantool.org>]

> Before the patch, big integer constant were silently

‘was’ or ‘constants’ — choose one.

> converted to floating point values. Fix that by issuing
> error message if value doesn't fit in int64_t range.
> 
> Also, refactor surrounding code as per Tarantool's code style.

Well, it was quite complicated to find your initial fix through
code style corrections. Next time lets separate somehow
such patches or, at least left notes what (and/or where) exactly
has been fixed.

> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
> index b1650cf..43d71eb 100644
> --- a/src/box/sql/expr.c
> +++ b/src/box/sql/expr.c
> @@ -1087,9 +1087,9 @@ sqlite3ExprAssignVarNumber(Parse * pParse, Expr * pExpr, u32 n)
> 			 * "nnn" to an integer and use it as the
> 			 * variable number
> 			 */
> -			i64 i;
> +			int64_t i;
> 			int bOk =
> -			    0 == sqlite3Atoi64(&z[1], &i, n - 1);
> +			    0 == sql_atoi64(&z[1], &i, n - 1);

You can fit it into one line:

int bOk = 0 == sql_atoi64(&z[1], &i, n - 1);

> static void
> -codeInteger(Parse * pParse, Expr * pExpr, int negFlag, int iMem)
> +sql_expr_code_int(struct Parse *parse, struct Expr *expr, bool neg_flag,
> +		  int mem)

I wouldn’t use ’sql_’ prefix for static functions. Btw, I still have doubts
concerning usage of prefixes in SQL - lets document it somewhere.

Also, according to codestyle bool vars are used with ‘is_’ or ‘if_’ prefixes.

> {
> -	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 (neg_flag)
> 			i = -i;
> -		sqlite3VdbeAddOp2(v, OP_Integer, i, iMem);
> +		sqlite3VdbeAddOp2(v, OP_Integer, i, mem);
> 	} else {
> 		int c;
> +		int64_t value;
> +		const char *z = expr->u.zToken;
> +		assert(z != NULL);
> +		c = sql_dec_or_hex_to_i64(z, &value);

Don’t delay initialisation until first usage:

int c = sql_dec_or_hex_to_i64(z, &value);

> +		if (c == 1 || (c == 2 && !neg_flag)
> +		    || (neg_flag && value == SMALLEST_INT64)) {
> +                        if (sqlite3_strnicmp(z, "0x", 2) == 0) {
> +                                sqlite3ErrorMsg(parse,
> +                                                "hex literal too big: %s%s",
> +                                                neg_flag ? "-" : "", z);

I see here broken indentation.

> diff --git a/src/box/sql/main.c b/src/box/sql/main.c
> index 00dc7a6..cbca90a 100644
> --- a/src/box/sql/main.c
> +++ b/src/box/sql/main.c
> @@ -2468,10 +2468,9 @@ sqlite3_uri_int64(const char *zFilename,	/* Filename as passed to xOpen */
> 		  sqlite3_int64 bDflt)	/* return if parameter is missing */
> {
> 	const char *z = sqlite3_uri_parameter(zFilename, zParam);
> -	sqlite3_int64 v;
> -	if (z && sqlite3DecOrHexToI64(z, &v) == SQLITE_OK) {
> +	int64_t v;
> +	if (z != NULL && sql_dec_or_hex_to_i64(z, &v) == SQLITE_OK)

Why do you use here SQLITE_OK? Lets just use comparison with zero.
(AFAIR we decided to avoid using this macros.)

> 		bDflt = v;
> -	}
> 	return bDflt;
> }
> 
> diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
> +/**
> + * 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
> + */
> +int
> +sql_atoi64(const char *z, int64_t *val, int length);
> +
> +/**
> + * Transform a UTF-8 integer literal, in either decimal or hexadecimal,
> + * into a 64-bit signed integer.  This routine accepts hexadecimal literals,

Fit comments into 66 chars.

> diff --git a/src/box/sql/util.c b/src/box/sql/util.c
> index e4c2c5d..d6e750c 100644
> --- a/src/box/sql/util.c
> +++ b/src/box/sql/util.c
> @@ -621,27 +621,35 @@ compare2pow63(const char *zNum, int incr)
> 	return c;
> }
> 
> -/*
> - * Convert zNum to a 64-bit signed integer.  zNum must be decimal. This
> +/**
> + * Convert z to a 64-bit signed integer.  z must be decimal. This
>  * routine does *not* accept hexadecimal notation.
>  *
> - * If the zNum value is representable as a 64-bit twos-complement
> - * integer, then write that value into *pNum and return 0.
> + * If the z value is representable as a 64-bit twos-complement
> + * integer, then write that value into *val and return 0.
>  *
> - * If zNum is exactly 9223372036854775808, return 2.  This special
> + * 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 zNum is too big for a 64-bit integer and is not
> - * 9223372036854775808  or if zNum contains any non-numeric text,
> + * 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
>  */

You can remove comment from definition: it repeats one from declaration.

> int
> -sqlite3Atoi64(const char *zNum, i64 * pNum, int length)
> +sql_atoi64(const char *z, int64_t *val, int length)
> {
> 	int incr = 1; // UTF-8

Remove this comment pls: it is not informative and violates codestyle.

> -	testcase(i == 18);
> -	testcase(i == 19);
> -	testcase(i == 20);
> -	if (&zNum[i] < zEnd	/* Extra bytes at the end */
> -	    || (i == 0 && zStart == zNum)	/* No digits */
> +	if (&z[i] < zEnd	/* Extra bytes at the end */

Remove comments (or put them above code to be explained);
Put binary operator to line above:

if (A &&
    B...

> @@ -713,36 +718,19 @@ sqlite3Atoi64(const char *zNum, i64 * pNum, int length)
> 	}
> }
> 
> -sqlite3DecOrHexToI64(const char *z, i64 * pOut)
> +sql_dec_or_hex_to_i64(const char *z, int64_t *val)
> {
> -#ifndef SQLITE_OMIT_HEX_INTEGER
> -	if (z[0] == '0' && (z[1] == 'x' || z[1] == 'X')
> -	    ) {
> -		u64 u = 0;
> +	if (z[0] == '0' && (z[1] == 'x' || z[1] == 'X')) {
> +		uint64_t u = 0;
> 		int i, k;
> -		for (i = 2; z[i] == '0'; i++) {
> -		}
> -		for (k = i; sqlite3Isxdigit(z[k]); k++) {
> +		for (i = 2; z[i] == '0'; i++);
> +		for (k = i; sqlite3Isxdigit(z[k]); k++)
> 			u = u * 16 + sqlite3HexToInt(z[k]);
> -		}
> -		memcpy(pOut, &u, 8);
> +		memcpy(val, &u, 8);
> 		return (z[k] == 0 && k - i <= 16) ? 0 : 1;
> -	} else
> -#endif				/* SQLITE_OMIT_HEX_INTEGER */
> -	{
> -		return sqlite3Atoi64(z, pOut, sqlite3Strlen30(z));
> +	} else {
> +		return sql_atoi64(z, val, sqlite3Strlen30(z));
> 	}

You can remove ‘else’ branch: ‘if' branch ends with return
(It is not vital, but code will look clearer, at least for me).

> diff --git a/test/sql/gh-2347-max-int-literals.test.lua b/test/sql/gh-2347-max-int-literals.test.lua
> new file mode 100644
> index 0000000..4b1ef0d
> --- /dev/null
> +++ b/test/sql/gh-2347-max-int-literals.test.lua

Lets avoid creating separate test-file for such insignificant issue.
You can put it for instance to sql/mist.test.lua or to one of sql-tap tests.

Also, it seems that you forgot to commit .result file.

> @@ -0,0 +1,11 @@
> +test_run = require('test_run').new()
> +engine = test_run:get_cfg('engine')
> +box.sql.execute('pragma sql_default_engine=\''..engine..'\'')
> +
> +box.cfg{}
> +
> +box.sql.execute("select (9223372036854775807)")
> +box.sql.execute("select (-9223372036854775808)")
> +
> +box.sql.execute("select (9223372036854775808)")
> +box.sql.execute("select (-9223372036854775809)")
> --