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

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

[Kirill Yukhin]

Before the patch, big integer constant were silently
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.

Closes #2347
---
Issue: https://github.com/tarantool/tarantool/issues/2347
Branch: https://github.com/tarantool/tarantool/commits/kyukhin/gh-2347-report-error-on-int-overflow

 src/box/sql/expr.c                         | 70 +++++++++++----------
 src/box/sql/main.c                         |  5 +-
 src/box/sql/sqliteInt.h                    | 49 ++++++++++++++-
 src/box/sql/util.c                         | 98 +++++++++++++-----------------
 src/box/sql/vdbe.c                         |  8 +--
 src/box/sql/vdbemem.c                      |  6 +-
 test/sql-tap/default.test.lua              |  5 +-
 test/sql/gh-2347-max-int-literals.test.lua | 11 ++++
 8 files changed, 144 insertions(+), 108 deletions(-)
 create mode 100644 test/sql/gh-2347-max-int-literals.test.lua

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);
 			x = (ynVar) i;
 			testcase(i == 0);
 			testcase(i == 1);
@@ -3259,51 +3259,49 @@ 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)
+sql_expr_code_int(struct Parse *parse, struct Expr *expr, bool neg_flag,
+		  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 (neg_flag)
 			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
-			if (sqlite3_strnicmp(z, "0x", 2) == 0) {
-				sqlite3ErrorMsg(pParse,
-						"hex literal too big: %s%s",
-						negFlag ? "-" : "", z);
-			} else
-#endif
-			{
-				codeReal(v, z, negFlag, iMem);
+		int64_t value;
+		const char *z = expr->u.zToken;
+		assert(z != NULL);
+		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);
+			} else {
+				sqlite3ErrorMsg(parse,
+						"oversized integer: %s%s",
+						neg_flag ? "-" : "", z);
 			}
-#endif
 		} else {
-			if (negFlag) {
+			if (neg_flag)
 				value = c == 2 ? SMALLEST_INT64 : -value;
-			}
-			sqlite3VdbeAddOp4Dup8(v, OP_Int64, 0, iMem, 0,
-					      (u8 *) & value, P4_INT64);
+			sqlite3VdbeAddOp4Dup8(v, OP_Int64, 0, mem, 0,
+					      (u8 *)&value, P4_INT64);
 		}
 	}
 }
@@ -3715,7 +3713,7 @@ sqlite3ExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
 							target, pExpr->op2);
 		}
 	case TK_INTEGER:{
-			codeInteger(pParse, pExpr, 0, target);
+			sql_expr_code_int(pParse, pExpr, false, target);
 			return target;
 		}
 #ifndef SQLITE_OMIT_FLOATING_POINT
@@ -3875,7 +3873,7 @@ sqlite3ExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
 			Expr *pLeft = pExpr->pLeft;
 			assert(pLeft);
 			if (pLeft->op == TK_INTEGER) {
-				codeInteger(pParse, pLeft, 1, target);
+				sql_expr_code_int(pParse, pLeft, true, target);
 				return target;
 #ifndef SQLITE_OMIT_FLOATING_POINT
 			} else if (pLeft->op == TK_FLOAT) {
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)
 		bDflt = v;
-	}
 	return bDflt;
 }
 
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index 18bf949..1748607 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
+ */
+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,
+ * whereas sql_atoi64() does not.
+ *
+ * @param z Literal being parsed.
+ * @param[out] val Parsed value.
+ * @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_dec_or_hex_to_i64(const char *z, int64_t *val);
+
 void sqlite3ErrorWithMsg(sqlite3 *, int, const char *, ...);
 void sqlite3Error(sqlite3 *, int);
 void sqlite3SystemError(sqlite3 *, int);
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
  */
 int
-sqlite3Atoi64(const char *zNum, i64 * pNum, int length)
+sql_atoi64(const char *z, int64_t *val, int length)
 {
 	int incr = 1; // UTF-8
 	u64 u = 0;
@@ -650,38 +658,35 @@ sqlite3Atoi64(const char *zNum, i64 * pNum, int length)
 	int c = 0;
 	int nonNum = 0;		/* True if input contains UTF16 with high byte non-zero */
 	const char *zStart;
-	const char *zEnd = zNum + length;
+	const char *zEnd = z + length;
 	incr = 1;
-	while (zNum < zEnd && sqlite3Isspace(*zNum))
-		zNum += incr;
-	if (zNum < zEnd) {
-		if (*zNum == '-') {
+	while (z < zEnd && sqlite3Isspace(*z))
+		z += incr;
+	if (z < zEnd) {
+		if (*z == '-') {
 			neg = 1;
-			zNum += incr;
-		} else if (*zNum == '+') {
-			zNum += incr;
+			z += incr;
+		} else if (*z == '+') {
+			z += incr;
 		}
 	}
-	zStart = zNum;
-	while (zNum < zEnd && zNum[0] == '0') {
-		zNum += incr;
+	zStart = z;
+	while (z < zEnd && z[0] == '0') {
+		z += incr;
 	}			/* Skip leading zeros. */
-	for (i = 0; &zNum[i] < zEnd && (c = zNum[i]) >= '0' && c <= '9';
+	for (i = 0; &z[i] < zEnd && (c = z[i]) >= '0' && c <= '9';
 	     i += incr) {
 		u = u * 10 + c - '0';
 	}
 	if (u > LARGEST_INT64) {
-		*pNum = neg ? SMALLEST_INT64 : LARGEST_INT64;
+		*val = neg ? SMALLEST_INT64 : LARGEST_INT64;
 	} else if (neg) {
-		*pNum = -(i64) u;
+		*val = -(i64) u;
 	} else {
-		*pNum = (i64) u;
+		*val = (i64) u;
 	}
-	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 */
+	    || (i == 0 && zStart == z)	/* No digits */
 	    ||i > 19 * incr	/* Too many digits */
 	    || nonNum		/* UTF16 with high-order bytes non-zero */
 	    ) {
@@ -695,7 +700,7 @@ sqlite3Atoi64(const char *zNum, i64 * pNum, int length)
 		return 0;
 	} else {
 		/* zNum is a 19-digit numbers.  Compare it against 9223372036854775808. */
-		c = compare2pow63(zNum, incr);
+		c = compare2pow63(z, incr);
 		if (c < 0) {
 			/* zNum is less than 9223372036854775808 so it fits */
 			assert(u <= LARGEST_INT64);
@@ -713,36 +718,19 @@ sqlite3Atoi64(const char *zNum, i64 * pNum, int length)
 	}
 }
 
-/*
- * Transform a UTF-8 integer literal, in either decimal or hexadecimal,
- * into a 64-bit signed integer.  This routine accepts hexadecimal literals,
- * whereas sqlite3Atoi64() does not.
- *
- * Returns:
- *
- *     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
-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));
 	}
 }
 
@@ -768,7 +756,6 @@ sqlite3GetInt32(const char *zNum, int *pValue)
 	} else if (zNum[0] == '+') {
 		zNum++;
 	}
-#ifndef SQLITE_OMIT_HEX_INTEGER
 	else if (zNum[0] == '0' && (zNum[1] == 'x' || zNum[1] == 'X')
 		 && sqlite3Isxdigit(zNum[2])
 	    ) {
@@ -786,7 +773,6 @@ sqlite3GetInt32(const char *zNum, int *pValue)
 			return 0;
 		}
 	}
-#endif
 	while (zNum[0] == '0')
 		zNum++;
 	for (i = 0; i < 11 && (c = zNum[i] - '0') >= 0 && c <= 9; i++) {
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)) {
 		pRec->u.i = iValue;
 		pRec->flags |= MEM_Int;
 	} else {
@@ -389,12 +389,10 @@ static u16 SQLITE_NOINLINE computeNumericType(Mem *pMem)
 {
 	assert((pMem->flags & (MEM_Int|MEM_Real))==0);
 	assert((pMem->flags & (MEM_Str|MEM_Blob))!=0);
-	if (sqlite3AtoF(pMem->z, &pMem->u.r, pMem->n)==0) {
+	if (sqlite3AtoF(pMem->z, &pMem->u.r, pMem->n)==0)
 		return 0;
-	}
-	if (sqlite3Atoi64(pMem->z, &pMem->u.i, pMem->n)==SQLITE_OK) {
+	if (sql_atoi64(pMem->z, (int64_t *)&pMem->u.i, pMem->n)==SQLITE_OK)
 		return MEM_Int;
-	}
 	return MEM_Real;
 }
 
diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c
index 2ce9074..5d92a27 100644
--- a/src/box/sql/vdbemem.c
+++ b/src/box/sql/vdbemem.c
@@ -459,9 +459,9 @@ sqlite3VdbeIntValue(Mem * pMem)
 	} else if (flags & MEM_Real) {
 		return doubleToInt64(pMem->u.r);
 	} else if (flags & (MEM_Str | MEM_Blob)) {
-		i64 value = 0;
+		int64_t value = 0;
 		assert(pMem->z || pMem->n == 0);
-		sqlite3Atoi64(pMem->z, &value, pMem->n);
+		sql_atoi64(pMem->z, &value, pMem->n);
 		return value;
 	} else {
 		return 0;
@@ -562,7 +562,7 @@ sqlite3VdbeMemNumerify(Mem * pMem)
 {
 	if ((pMem->flags & (MEM_Int | MEM_Real | MEM_Null)) == 0) {
 		assert((pMem->flags & (MEM_Blob | MEM_Str)) != 0);
-		if (0 == sqlite3Atoi64(pMem->z, &pMem->u.i, pMem->n)) {
+		if (0 == sql_atoi64(pMem->z, (int64_t *)&pMem->u.i, pMem->n)) {
 			MemSetTypeFlag(pMem, MEM_Int);
 		} else {
 			pMem->u.r = sqlite3VdbeRealValue(pMem);
diff --git a/test/sql-tap/default.test.lua b/test/sql-tap/default.test.lua
index 9d59767..2f08a51 100755
--- a/test/sql-tap/default.test.lua
+++ b/test/sql-tap/default.test.lua
@@ -146,14 +146,13 @@ test:do_execsql_test(
 	d INT DEFAULT -2147483647,
 	e INT DEFAULT -2147483648,
 	f INT DEFAULT (-9223372036854775808),
-	g INT DEFAULT 9223372036854775808,
 	h INT DEFAULT (-(-9223372036854775807))
 	);
 	INSERT INTO t300 DEFAULT VALUES;
-	SELECT a, b, c, d, e, f, cast(g as text), h FROM t300;
+	SELECT a, b, c, d, e, f, h FROM t300;
 	]], {
 	-- <default-3.3>
-	2147483647, 2147483648, 9223372036854775807LL, -2147483647, -2147483648, -9223372036854775808LL, "9.22337203685478e+18", 9223372036854775807LL
+	2147483647, 2147483648, 9223372036854775807LL, -2147483647, -2147483648, -9223372036854775808LL, 9223372036854775807LL
 	-- </default-3.3>
 })
 
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
@@ -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)")
-- 
2.16.2

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

[n.pettik]

> 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)")
> -- 

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

[Kirill Yukhin]

Hello Nikita,
Thanks for review, my answers are inlined. Updated patch in the bottom.

On 18 июл 04:10, n.pettik wrote:
> 
> > Before the patch, big integer constant were silently
> 
> ‘was’ or ‘constants’ — choose one.
Fixed.

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

> > 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);
Fixed. Also, replaced w/ bool here.

> > 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.
Well, news to me. Ok, prefix removed.

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

> > {
> > -	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);
Fixed.

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

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

> > diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
> > +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.
Done.

> > 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
> > + *
> > + * @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.
Done.

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

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

> > -	{
> > -		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).
Done.

> > 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.
I think, that this minor change affects visible behaviour. So, let's keep
tests for such changes in separate files.

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

--
Regards, Kirill Yukhin

commit 5ff577f62222bdc3ebd227161774669a9d1b6396
Author: Kirill Yukhin <kyukhin@tarantool.org>
Date:   Tue Jul 17 08:54:24 2018 +0300

    sql: do not allow oversized integer literals
    
    Before the patch, big integer constants were silently
    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.
    
    Closes #2347

diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index b1650cf..f662a99 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -1087,15 +1087,14 @@ sqlite3ExprAssignVarNumber(Parse * pParse, Expr * pExpr, u32 n)
 			 * "nnn" to an integer and use it as the
 			 * variable number
 			 */
-			i64 i;
-			int bOk =
-			    0 == sqlite3Atoi64(&z[1], &i, n - 1);
+			int64_t i;
+			bool is_ok = 0 == sql_atoi64(&z[1], &i, n - 1);
 			x = (ynVar) i;
 			testcase(i == 0);
 			testcase(i == 1);
 			testcase(i == SQL_BIND_PARAMETER_MAX - 1);
 			testcase(i == SQL_BIND_PARAMETER_MAX);
-			if (bOk == 0 || i < 1 || i > SQL_BIND_PARAMETER_MAX) {
+			if (!is_ok || i < 1 || i > SQL_BIND_PARAMETER_MAX) {
 				sqlite3ErrorMsg(pParse,
 						"variable number must be between $1 and $%d",
 						SQL_BIND_PARAMETER_MAX);
@@ -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)) {
 			if (sqlite3_strnicmp(z, "0x", 2) == 0) {
-				sqlite3ErrorMsg(pParse,
+				sqlite3ErrorMsg(parse,
 						"hex literal too big: %s%s",
-						negFlag ? "-" : "", z);
-			} else
-#endif
-			{
-				codeReal(v, z, negFlag, iMem);
+						is_neg ? "-" : "", z);
+			} else {
+				sqlite3ErrorMsg(parse,
+						"oversized integer: %s%s",
+						is_neg ? "-" : "", z);
 			}
-#endif
 		} else {
-			if (negFlag) {
+			if (is_neg)
 				value = c == 2 ? SMALLEST_INT64 : -value;
-			}
-			sqlite3VdbeAddOp4Dup8(v, OP_Int64, 0, iMem, 0,
-					      (u8 *) & value, P4_INT64);
+			sqlite3VdbeAddOp4Dup8(v, OP_Int64, 0, mem, 0,
+					      (u8 *)&value, P4_INT64);
 		}
 	}
 }
@@ -3715,7 +3711,7 @@ sqlite3ExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
 							target, pExpr->op2);
 		}
 	case TK_INTEGER:{
-			codeInteger(pParse, pExpr, 0, target);
+			expr_code_int(pParse, pExpr, false, target);
 			return target;
 		}
 #ifndef SQLITE_OMIT_FLOATING_POINT
@@ -3875,7 +3871,7 @@ sqlite3ExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
 			Expr *pLeft = pExpr->pLeft;
 			assert(pLeft);
 			if (pLeft->op == TK_INTEGER) {
-				codeInteger(pParse, pLeft, 1, target);
+				expr_code_int(pParse, pLeft, true, target);
 				return target;
 #ifndef SQLITE_OMIT_FLOATING_POINT
 			} else if (pLeft->op == TK_FLOAT) {
diff --git a/src/box/sql/main.c b/src/box/sql/main.c
index 00dc7a6..85bc7e9 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) == 0)
 		bDflt = v;
-	}
 	return bDflt;
 }
 
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
+ */
+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, whereas sql_atoi64() does not.
+ *
+ * @param z Literal being parsed.
+ * @param[out] val Parsed value.
+ * @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_dec_or_hex_to_i64(const char *z, int64_t *val);
+
 void sqlite3ErrorWithMsg(sqlite3 *, int, const char *, ...);
 void sqlite3Error(sqlite3 *, int);
 void sqlite3SystemError(sqlite3 *, int);
diff --git a/src/box/sql/util.c b/src/box/sql/util.c
index e4c2c5d..a7406c0 100644
--- a/src/box/sql/util.c
+++ b/src/box/sql/util.c
@@ -621,70 +621,46 @@ compare2pow63(const char *zNum, int incr)
 	return c;
 }
 
-/*
- * Convert zNum to a 64-bit signed integer.  zNum 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 zNum 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,
- * 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.
- */
 int
-sqlite3Atoi64(const char *zNum, i64 * pNum, int length)
+sql_atoi64(const char *z, int64_t *val, int length)
 {
-	int incr = 1; // UTF-8
+	int incr = 1;
 	u64 u = 0;
 	int neg = 0;		/* assume positive */
 	int i;
 	int c = 0;
 	int nonNum = 0;		/* True if input contains UTF16 with high byte non-zero */
 	const char *zStart;
-	const char *zEnd = zNum + length;
+	const char *zEnd = z + length;
 	incr = 1;
-	while (zNum < zEnd && sqlite3Isspace(*zNum))
-		zNum += incr;
-	if (zNum < zEnd) {
-		if (*zNum == '-') {
+	while (z < zEnd && sqlite3Isspace(*z))
+		z += incr;
+	if (z < zEnd) {
+		if (*z == '-') {
 			neg = 1;
-			zNum += incr;
-		} else if (*zNum == '+') {
-			zNum += incr;
+			z += incr;
+		} else if (*z == '+') {
+			z += incr;
 		}
 	}
-	zStart = zNum;
-	while (zNum < zEnd && zNum[0] == '0') {
-		zNum += incr;
-	}			/* Skip leading zeros. */
-	for (i = 0; &zNum[i] < zEnd && (c = zNum[i]) >= '0' && c <= '9';
+	zStart = z;
+	/* Skip leading zeros. */
+	while (z < zEnd && z[0] == '0') {
+		z += incr;
+	}
+	for (i = 0; &z[i] < zEnd && (c = z[i]) >= '0' && c <= '9';
 	     i += incr) {
 		u = u * 10 + c - '0';
 	}
 	if (u > LARGEST_INT64) {
-		*pNum = neg ? SMALLEST_INT64 : LARGEST_INT64;
+		*val = neg ? SMALLEST_INT64 : LARGEST_INT64;
 	} else if (neg) {
-		*pNum = -(i64) u;
+		*val = -(i64) u;
 	} else {
-		*pNum = (i64) u;
+		*val = (i64) u;
 	}
-	testcase(i == 18);
-	testcase(i == 19);
-	testcase(i == 20);
-	if (&zNum[i] < zEnd	/* Extra bytes at the end */
-	    || (i == 0 && zStart == zNum)	/* No digits */
-	    ||i > 19 * incr	/* Too many digits */
-	    || nonNum		/* UTF16 with high-order bytes non-zero */
-	    ) {
+	if (&z[i] < zEnd || (i == 0 && zStart == z) || i > 19 * incr ||
+	    nonNum) {
 		/* zNum is empty or contains non-numeric text or is longer
 		 * than 19 digits (thus guaranteeing that it is too large)
 		 */
@@ -695,7 +671,7 @@ sqlite3Atoi64(const char *zNum, i64 * pNum, int length)
 		return 0;
 	} else {
 		/* zNum is a 19-digit numbers.  Compare it against 9223372036854775808. */
-		c = compare2pow63(zNum, incr);
+		c = compare2pow63(z, incr);
 		if (c < 0) {
 			/* zNum is less than 9223372036854775808 so it fits */
 			assert(u <= LARGEST_INT64);
@@ -713,37 +689,19 @@ sqlite3Atoi64(const char *zNum, i64 * pNum, int length)
 	}
 }
 
-/*
- * Transform a UTF-8 integer literal, in either decimal or hexadecimal,
- * into a 64-bit signed integer.  This routine accepts hexadecimal literals,
- * whereas sqlite3Atoi64() does not.
- *
- * Returns:
- *
- *     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
-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));
 	}
+	return sql_atoi64(z, val, sqlite3Strlen30(z));
 }
 
 /*
@@ -768,7 +726,6 @@ sqlite3GetInt32(const char *zNum, int *pValue)
 	} else if (zNum[0] == '+') {
 		zNum++;
 	}
-#ifndef SQLITE_OMIT_HEX_INTEGER
 	else if (zNum[0] == '0' && (zNum[1] == 'x' || zNum[1] == 'X')
 		 && sqlite3Isxdigit(zNum[2])
 	    ) {
@@ -786,7 +743,6 @@ sqlite3GetInt32(const char *zNum, int *pValue)
 			return 0;
 		}
 	}
-#endif
 	while (zNum[0] == '0')
 		zNum++;
 	for (i = 0; i < 11 && (c = zNum[i] - '0') >= 0 && c <= 9; i++) {
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)) {
 		pRec->u.i = iValue;
 		pRec->flags |= MEM_Int;
 	} else {
@@ -389,12 +389,10 @@ static u16 SQLITE_NOINLINE computeNumericType(Mem *pMem)
 {
 	assert((pMem->flags & (MEM_Int|MEM_Real))==0);
 	assert((pMem->flags & (MEM_Str|MEM_Blob))!=0);
-	if (sqlite3AtoF(pMem->z, &pMem->u.r, pMem->n)==0) {
+	if (sqlite3AtoF(pMem->z, &pMem->u.r, pMem->n)==0)
 		return 0;
-	}
-	if (sqlite3Atoi64(pMem->z, &pMem->u.i, pMem->n)==SQLITE_OK) {
+	if (sql_atoi64(pMem->z, (int64_t *)&pMem->u.i, pMem->n)==SQLITE_OK)
 		return MEM_Int;
-	}
 	return MEM_Real;
 }
 
diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c
index 2ce9074..5d92a27 100644
--- a/src/box/sql/vdbemem.c
+++ b/src/box/sql/vdbemem.c
@@ -459,9 +459,9 @@ sqlite3VdbeIntValue(Mem * pMem)
 	} else if (flags & MEM_Real) {
 		return doubleToInt64(pMem->u.r);
 	} else if (flags & (MEM_Str | MEM_Blob)) {
-		i64 value = 0;
+		int64_t value = 0;
 		assert(pMem->z || pMem->n == 0);
-		sqlite3Atoi64(pMem->z, &value, pMem->n);
+		sql_atoi64(pMem->z, &value, pMem->n);
 		return value;
 	} else {
 		return 0;
@@ -562,7 +562,7 @@ sqlite3VdbeMemNumerify(Mem * pMem)
 {
 	if ((pMem->flags & (MEM_Int | MEM_Real | MEM_Null)) == 0) {
 		assert((pMem->flags & (MEM_Blob | MEM_Str)) != 0);
-		if (0 == sqlite3Atoi64(pMem->z, &pMem->u.i, pMem->n)) {
+		if (0 == sql_atoi64(pMem->z, (int64_t *)&pMem->u.i, pMem->n)) {
 			MemSetTypeFlag(pMem, MEM_Int);
 		} else {
 			pMem->u.r = sqlite3VdbeRealValue(pMem);
diff --git a/test/sql-tap/default.test.lua b/test/sql-tap/default.test.lua
index 9d59767..2f08a51 100755
--- a/test/sql-tap/default.test.lua
+++ b/test/sql-tap/default.test.lua
@@ -146,14 +146,13 @@ test:do_execsql_test(
 	d INT DEFAULT -2147483647,
 	e INT DEFAULT -2147483648,
 	f INT DEFAULT (-9223372036854775808),
-	g INT DEFAULT 9223372036854775808,
 	h INT DEFAULT (-(-9223372036854775807))
 	);
 	INSERT INTO t300 DEFAULT VALUES;
-	SELECT a, b, c, d, e, f, cast(g as text), h FROM t300;
+	SELECT a, b, c, d, e, f, h FROM t300;
 	]], {
 	-- <default-3.3>
-	2147483647, 2147483648, 9223372036854775807LL, -2147483647, -2147483648, -9223372036854775808LL, "9.22337203685478e+18", 9223372036854775807LL
+	2147483647, 2147483648, 9223372036854775807LL, -2147483647, -2147483648, -9223372036854775808LL, 9223372036854775807LL
 	-- </default-3.3>
 })
 
diff --git a/test/sql/gh-2347-max-int-literals.result b/test/sql/gh-2347-max-int-literals.result
new file mode 100644
index 0000000..c289a80
--- /dev/null
+++ b/test/sql/gh-2347-max-int-literals.result
@@ -0,0 +1,28 @@
+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)")
+---
+- - [9223372036854775807]
+...
+box.sql.execute("select (-9223372036854775808)")
+---
+- - [-9223372036854775808]
+...
+box.sql.execute("select (9223372036854775808)")
+---
+- error: 'oversized integer: 9223372036854775808'
+...
+box.sql.execute("select (-9223372036854775809)")
+---
+- error: 'oversized integer: -9223372036854775809'
+...
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
@@ -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)")

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

[n.pettik]

[-- Attachment #1: Type: text/plain, Size: 4358 bytes --]

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?


[-- Attachment #2: Type: text/html, Size: 96405 bytes --]

[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