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 AB97D278C2 for ; Thu, 19 Jul 2018 04:53:36 -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 kVWwcp6CrvE3 for ; Thu, 19 Jul 2018 04:53:36 -0400 (EDT) Received: from smtp57.i.mail.ru (smtp57.i.mail.ru [217.69.128.37]) (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 5FDB8278C0 for ; Thu, 19 Jul 2018 04:53:36 -0400 (EDT) Date: Thu, 19 Jul 2018 11:53:33 +0300 From: Kirill Yukhin Subject: [tarantool-patches] Re: [PATCH] sql: do not allow oversized integer literals Message-ID: <20180719085333.qxbjllkwisyhsrhm@tarantool.org> References: <9995AAE4-3420-4685-9AC9-2E2854D4E905@tarantool.org> <20180718075714.4dl3hlfuuoqimv5p@tarantool.org> <19FED891-E36F-4D49-84BF-EFE30048CD46@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <19FED891-E36F-4D49-84BF-EFE30048CD46@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: "n.pettik" Cc: tarantool-patches@freelists.org 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