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 B50E121442 for ; Tue, 17 Jul 2018 21:11:09 -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 Cs2wuyukcDus for ; Tue, 17 Jul 2018 21:11:09 -0400 (EDT) Received: from smtp15.mail.ru (smtp15.mail.ru [94.100.176.133]) (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 C2D8B211F4 for ; Tue, 17 Jul 2018 21:11:08 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\)) Subject: [tarantool-patches] Re: [PATCH] sql: do not allow oversized integer literals From: "n.pettik" In-Reply-To: Date: Wed, 18 Jul 2018 04:10:59 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <9995AAE4-3420-4685-9AC9-2E2854D4E905@tarantool.org> References: 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 > Before the patch, big integer constant were silently =E2=80=98was=E2=80=99 or =E2=80=98constants=E2=80=99 =E2=80=94 choose = one. > converted to floating point values. Fix that by issuing > error message if value doesn't fit in int64_t range. >=20 > 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 =3D > - 0 =3D=3D sqlite3Atoi64(&z[1], &i, n - 1); > + 0 =3D=3D sql_atoi64(&z[1], &i, n - 1); You can fit it into one line: int bOk =3D 0 =3D=3D 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=E2=80=99t use =E2=80=99sql_=E2=80=99 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 =E2=80=98is_=E2=80=99= or =E2=80=98if_=E2=80=99 prefixes. > { > - 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 (neg_flag) > i =3D -i; > - sqlite3VdbeAddOp2(v, OP_Integer, i, iMem); > + sqlite3VdbeAddOp2(v, OP_Integer, i, mem); > } else { > int c; > + int64_t value; > + const char *z =3D expr->u.zToken; > + assert(z !=3D NULL); > + c =3D sql_dec_or_hex_to_i64(z, &value); Don=E2=80=99t delay initialisation until first usage: int c =3D sql_dec_or_hex_to_i64(z, &value); > + if (c =3D=3D 1 || (c =3D=3D 2 && !neg_flag) > + || (neg_flag && value =3D=3D SMALLEST_INT64)) { > + if (sqlite3_strnicmp(z, "0x", 2) =3D=3D 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 =3D sqlite3_uri_parameter(zFilename, zParam); > - sqlite3_int64 v; > - if (z && sqlite3DecOrHexToI64(z, &v) =3D=3D SQLITE_OK) { > + int64_t v; > + if (z !=3D NULL && sql_dec_or_hex_to_i64(z, &v) =3D=3D = SQLITE_OK) Why do you use here SQLITE_OK? Lets just use comparison with zero. (AFAIR we decided to avoid using this macros.) > bDflt =3D v; > - } > return bDflt; > } >=20 > 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; > } >=20 > -/* > - * 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 =3D 1; // UTF-8 Remove this comment pls: it is not informative and violates codestyle. > - testcase(i =3D=3D 18); > - testcase(i =3D=3D 19); > - testcase(i =3D=3D 20); > - if (&zNum[i] < zEnd /* Extra bytes at the end */ > - || (i =3D=3D 0 && zStart =3D=3D 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) > } > } >=20 > -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] =3D=3D '0' && (z[1] =3D=3D 'x' || z[1] =3D=3D 'X') > - ) { > - u64 u =3D 0; > + if (z[0] =3D=3D '0' && (z[1] =3D=3D 'x' || z[1] =3D=3D 'X')) { > + uint64_t u =3D 0; > int i, k; > - for (i =3D 2; z[i] =3D=3D '0'; i++) { > - } > - for (k =3D i; sqlite3Isxdigit(z[k]); k++) { > + for (i =3D 2; z[i] =3D=3D '0'; i++); > + for (k =3D i; sqlite3Isxdigit(z[k]); k++) > u =3D u * 16 + sqlite3HexToInt(z[k]); > - } > - memcpy(pOut, &u, 8); > + memcpy(val, &u, 8); > return (z[k] =3D=3D 0 && k - i <=3D 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 =E2=80=98else=E2=80=99 branch: =E2=80=98if' 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 =3D require('test_run').new() > +engine =3D test_run:get_cfg('engine') > +box.sql.execute('pragma sql_default_engine=3D\''..engine..'\'') > + > +box.cfg{} > + > +box.sql.execute("select (9223372036854775807)") > +box.sql.execute("select (-9223372036854775808)") > + > +box.sql.execute("select (9223372036854775808)") > +box.sql.execute("select (-9223372036854775809)") > --=20