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

  • From: "n.pettik" <korablev@xxxxxxxxxxxxx>
  • To: tarantool-patches@xxxxxxxxxxxxx
  • Date: Wed, 18 Jul 2018 20:56:47 +0300

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?

Other related posts: