From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 22207278C3 for ; Wed, 18 Jul 2018 13:56:51 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id Bcyzusuf117g for ; Wed, 18 Jul 2018 13:56:50 -0400 (EDT) Received: from smtp58.i.mail.ru (smtp58.i.mail.ru [217.69.128.38]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 4396B278BF for ; Wed, 18 Jul 2018 13:56:50 -0400 (EDT) From: "n.pettik" Message-Id: <19FED891-E36F-4D49-84BF-EFE30048CD46@tarantool.org> Content-Type: multipart/alternative; boundary="Apple-Mail=_1447CAC9-77E1-428F-92CF-51F9F050FB16" Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\)) Subject: [tarantool-patches] Re: [PATCH] sql: do not allow oversized integer literals Date: Wed, 18 Jul 2018 20:56:47 +0300 In-Reply-To: <20180718075714.4dl3hlfuuoqimv5p@tarantool.org> References: <9995AAE4-3420-4685-9AC9-2E2854D4E905@tarantool.org> <20180718075714.4dl3hlfuuoqimv5p@tarantool.org> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org Cc: Kirill Yukhin --Apple-Mail=_1447CAC9-77E1-428F-92CF-51F9F050FB16 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 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 = =E2=80=98over-kill'. > @@ -3259,51 +3258,48 @@ codeReal(Vdbe * v, const char *z, int = negateFlag, int iMem) > } > #endif >=20 > -/* > +/** > * 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 =3D pParse->pVdbe; > - if (pExpr->flags & EP_IntValue) { > - int i =3D pExpr->u.iValue; > + struct Vdbe *v =3D parse->pVdbe; > + if (expr->flags & EP_IntValue) { > + int i =3D expr->u.iValue; > assert(i >=3D 0); > - if (negFlag) > + if (is_neg) > i =3D -i; > - sqlite3VdbeAddOp2(v, OP_Integer, i, iMem); > + sqlite3VdbeAddOp2(v, OP_Integer, i, mem); > } else { > - int c; > - i64 value; > - const char *z =3D pExpr->u.zToken; > - assert(z !=3D 0); > - c =3D sqlite3DecOrHexToI64(z, &value); > - if (c =3D=3D 1 || (c =3D=3D 2 && !negFlag) > - || (negFlag && value =3D=3D 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 =3D expr->u.zToken; > + assert(z !=3D NULL); > + int c =3D sql_dec_or_hex_to_i64(z, &value); > + if (c =3D=3D 1 || (c =3D=3D 2 && !is_neg) > + || (is_neg && value =3D=3D SMALLEST_INT64)) { Put binary operator to line above: if (A && B=E2=80=A6 > 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); >=20 > 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))=3D=3DMEM_Str); > if (sqlite3AtoF(pRec->z, &rValue, pRec->n)=3D=3D0) return; > - if (0=3D=3Dsqlite3Atoi64(pRec->z, &iValue, pRec->n)) { > + if (0 =3D=3D sql_atoi64(pRec->z, (int64_t *)&iValue, pRec->n)) { Are you sure this cast is not redundant? If not so, why don=E2=80=99t = use int64_t as type of iValue? --Apple-Mail=_1447CAC9-77E1-428F-92CF-51F9F050FB16 Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8
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 =E2=80=98over-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
+ * =             UT= F8 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 =3D pParse->pVdbe;
- = if (pExpr->flags & EP_IntValue) = {
- = = int i =3D pExpr->u.iValue;
+ = struct Vdbe *v =3D parse->pVdbe;
+ = if (expr->flags & EP_IntValue) = {
+ = = int i =3D expr->u.iValue;
= = assert(i >=3D 0);
- = = if (negFlag)
+ = if (is_neg)
= = i =3D -i;
- = sqlite3VdbeAddOp2(v, OP_Integer, i, = iMem);
+ = = sqlite3VdbeAddOp2(v, OP_Integer, i, = mem);
= } else {
- = int c;
- = i64 value;
- = const char *z =3D pExpr->u.zToken;
- = = assert(z !=3D 0);
- = c =3D sqlite3DecOrHexToI64(z, = &value);
- = = if (c =3D=3D 1 || (c =3D=3D 2 && = !negFlag)
- = =     || = (negFlag && value =3D=3D 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 =3D expr->u.zToken;
+ = = assert(z !=3D NULL);
+ = = int c =3D sql_dec_or_hex_to_i64(z, = &value);
+ = = if (c =3D=3D 1 || (c =3D=3D 2 && = !is_neg)
+ = =     || = (is_neg && value =3D=3D SMALLEST_INT64)) = {

Put binary operator = to line above:

if (A = &&
   B=E2=80=A6

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))=3D=3DMEM_Str);
if (sqlite3AtoF(pRec->z, &rValue, = pRec->n)=3D=3D0) return;
- if (0=3D=3Dsqlite3Atoi64(pRec->z, = &iValue, pRec->n)) {
+ if (0 =3D=3D sql_atoi64(pRec->z, = (int64_t *)&iValue, pRec->n)) {

Are you = sure this cast is not redundant? If not so, why don=E2=80=99t use = int64_t as type of iValue?

= --Apple-Mail=_1447CAC9-77E1-428F-92CF-51F9F050FB16--