Tarantool development patches archive
 help / color / mirror / Atom feed
From: Nikita Pettik <korablev@tarantool.org>
To: tarantool-patches@freelists.org
Cc: v.shpilevoy@tarantool.org, Nikita Pettik <korablev@tarantool.org>
Subject: [tarantool-patches] [PATCH 3/4] sql: refactor sqlVdbeMsgpackGet()
Date: Wed, 20 Feb 2019 14:57:39 +0300	[thread overview]
Message-ID: <326c19ce8abd923ae9d0ed4873abcfdcf06c13d6.1550663540.git.korablev@tarantool.org> (raw)
In-Reply-To: <cover.1550663540.git.korablev@tarantool.org>
In-Reply-To: <cover.1550663540.git.korablev@tarantool.org>

Tarantool allows to hold in INTEGER field values in range
[INT64_MAX, UINT64_MAX], which is obviously larger than common int64_t
range. In this regard, if value of integer field in range
[INT64_MAX, UINT64_MAX] is presented in tuple (e.g. after insertion from
Lua interface), then after decoding msgpack (during processing SQL
query) its value won't fit into int64_t (which in turn is basic type
used to hold integers inside VDBE memory). Now if this happens, instead
of raising an overflow error, value is stored as floating point number
(with precise loss, obviously). Such approach is considered to be messy
and we are going to raise "integer overflow" error instead. To make this
happen, lets firstly refactor sqlVdbeMsgpackGet() to make it return
error code to indicate that something went wrong and move length of
decoded part to output parameters. Codestyle fixes are included as well.

Needed for #3735
---
 src/box/sql/vdbe.c    |   5 +-
 src/box/sql/vdbeInt.h |  13 ++++-
 src/box/sql/vdbeaux.c | 132 ++++++++++++++++++++++++--------------------------
 src/box/sql/vdbemem.c |   4 +-
 4 files changed, 81 insertions(+), 73 deletions(-)

diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index d9797b22b..d38f61774 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -2737,8 +2737,9 @@ case OP_Column: {
 	if (VdbeMemDynamic(pDest)) {
 		sqlVdbeMemSetNull(pDest);
 	}
-
-	sqlVdbeMsgpackGet(zData+aOffset[p2], pDest);
+	uint32_t unused;
+	vdbe_decode_msgpack_into_mem((const char *)(zData + aOffset[p2]),
+				     pDest, &unused);
 	/* MsgPack map, array or extension (unsupported in sql).
 	 * Wrap it in a blob verbatim.
 	 */
diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
index d003d6bb3..49f665bb0 100644
--- a/src/box/sql/vdbeInt.h
+++ b/src/box/sql/vdbeInt.h
@@ -547,7 +547,18 @@ int sqlVdbeCompareMsgpack(const char **key1,
  */
 int sqlVdbeRecordCompareMsgpack(const void *key1,
 				    struct UnpackedRecord *key2);
-u32 sqlVdbeMsgpackGet(const unsigned char *buf, Mem * pMem);
+
+/**
+ * Decode msgpack and save value into VDBE memory cell.
+ *
+ * @param buf Buffer to deserialize msgpack from.
+ * @param mem Memory cell to write value into.
+ * @param len[out] Length of decoded part.
+ * @retval Return code: < 0 in case of error.
+ * @retval 0 on success.
+ */
+int
+vdbe_decode_msgpack_into_mem(const char *buf, struct Mem *mem, uint32_t *len);
 
 struct mpstream;
 struct region;
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index b831b52ad..ba9b96645 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -3688,79 +3688,74 @@ sqlVdbeRecordCompareMsgpack(const void *key1,
 	return key2->default_rc;
 }
 
-u32
-sqlVdbeMsgpackGet(const unsigned char *buf,	/* Buffer to deserialize from */
-		      Mem * pMem)		/* Memory cell to write value into */
+int
+vdbe_decode_msgpack_into_mem(const char *buf, struct Mem *mem, uint32_t *len)
 {
-	const char *zParse = (const char *)buf;
-	switch (mp_typeof(*zParse)) {
+	const char *start_buf = buf;
+	switch (mp_typeof(*buf)) {
 	case MP_ARRAY:
 	case MP_MAP:
 	case MP_EXT:
-	default:{
-			pMem->flags = 0;
-			return 0;
-		}
-	case MP_NIL:{
-			mp_decode_nil((const char **)&zParse);	/*  Still need to promote zParse.  */
-			pMem->flags = MEM_Null;
-			break;
-		}
-	case MP_BOOL:{
-			assert((unsigned char)*zParse == 0xc2
-			       || (unsigned char)*zParse == 0xc3);
-			pMem->u.i = (unsigned char)*zParse - 0xc2;
-			pMem->flags = MEM_Int;
-			break;
-		}
-	case MP_UINT:{
-			uint64_t v = mp_decode_uint(&zParse);
-			if (v > INT64_MAX) {
-				/*
-				 * If the value exceeds i64 range, convert to double (lossy).
-				 */
-				pMem->u.r = v;
-				pMem->flags = MEM_Real;
-			} else {
-				pMem->u.i = v;
-				pMem->flags = MEM_Int;
-			}
-			break;
-		}
-	case MP_INT:{
-			pMem->u.i = mp_decode_int(&zParse);
-			pMem->flags = MEM_Int;
-			break;
-		}
-	case MP_STR:{
-			/* XXX u32->int */
-			pMem->n = (int)mp_decode_strl((const char **)&zParse);
-			pMem->flags = MEM_Str | MEM_Ephem;
- install_blob:
-			pMem->z = (char *)zParse;
-			zParse += pMem->n;
-			break;
-		}
-	case MP_BIN:{
-			/* XXX u32->int */
-			pMem->n = (int)mp_decode_binl((const char **)&zParse);
-			pMem->flags = MEM_Blob | MEM_Ephem;
-			goto install_blob;
-		}
-	case MP_FLOAT:{
-			pMem->u.r = mp_decode_float(&zParse);
-			pMem->flags =
-			    sqlIsNaN(pMem->u.r) ? MEM_Null : MEM_Real;
-			break;
-		}
-	case MP_DOUBLE:{
-			pMem->u.r = mp_decode_double(&zParse);
-			pMem->flags =
-			    sqlIsNaN(pMem->u.r) ? MEM_Null : MEM_Real;
-			break;
+	default: {
+		mem->flags = 0;
+		break;
+	}
+	case MP_NIL: {
+		mp_decode_nil(&buf);
+		mem->flags = MEM_Null;
+		break;
+	}
+	case MP_BOOL: {
+		assert((unsigned char)*buf == 0xc2 ||
+		       (unsigned char)*buf == 0xc3);
+		mem->u.i = (unsigned char)*buf - 0xc2;
+		mem->flags = MEM_Int;
+		break;
+	}
+	case MP_UINT: {
+		uint64_t v = mp_decode_uint(&buf);
+		if (v > INT64_MAX) {
+			mem->u.r = v;
+			mem->flags = MEM_Real;
+		} else {
+			mem->u.i = v;
+			mem->flags = MEM_Int;
 		}
+		break;
+	}
+	case MP_INT: {
+		mem->u.i = mp_decode_int(&buf);
+		mem->flags = MEM_Int;
+		break;
+	}
+	case MP_STR: {
+		/* XXX u32->int */
+		mem->n = (int) mp_decode_strl(&buf);
+		mem->flags = MEM_Str | MEM_Ephem;
+install_blob:
+		mem->z = (char *)buf;
+		buf += mem->n;
+		break;
+	}
+	case MP_BIN: {
+		/* XXX u32->int */
+		mem->n = (int) mp_decode_binl(&buf);
+		mem->flags = MEM_Blob | MEM_Ephem;
+		goto install_blob;
 	}
-	return (u32) (zParse - (const char *)buf);
+	case MP_FLOAT: {
+		mem->u.r = mp_decode_float(&buf);
+		mem->flags = sqlIsNaN(mem->u.r) ? MEM_Null : MEM_Real;
+		break;
+	}
+	case MP_DOUBLE: {
+		mem->u.r = mp_decode_double(&buf);
+		mem->flags = sqlIsNaN(mem->u.r) ? MEM_Null : MEM_Real;
+		break;
+	}
+	}
+	*len = (uint32_t)(buf - start_buf);
+	return 0;
 }
 
 void
@@ -3778,7 +3773,8 @@ sqlVdbeRecordUnpackMsgpack(struct key_def *key_def,	/* Information about the rec
 	while (n--) {
 		pMem->szMalloc = 0;
 		pMem->z = 0;
-		u32 sz = sqlVdbeMsgpackGet((u8 *) zParse, pMem);
+		uint32_t sz = 0;
+		vdbe_decode_msgpack_into_mem(zParse, pMem, &sz);
 		if (sz == 0) {
 			/* MsgPack array, map or ext. Treat as blob. */
 			pMem->z = (char *)zParse;
diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c
index a816936f0..19802f746 100644
--- a/src/box/sql/vdbemem.c
+++ b/src/box/sql/vdbemem.c
@@ -1634,8 +1634,8 @@ sql_stat4_column(struct sql *db, const char *record, uint32_t col_num,
 			return -1;
 		}
 	}
-	sqlVdbeMsgpackGet((const unsigned char *) a, mem);
-	return 0;
+	uint32_t unused;
+	return vdbe_decode_msgpack_into_mem(a, mem, &unused);
 }
 
 /*
-- 
2.15.1

  parent reply	other threads:[~2019-02-20 11:57 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-02-20 11:57 [tarantool-patches] [PATCH 0/4] Fix integer overflow behaviour during VDBE execution Nikita Pettik
2019-02-20 11:57 ` [tarantool-patches] [PATCH 1/4] sql: raise an error if int is overflowed during math operations Nikita Pettik
2019-02-20 11:57 ` [tarantool-patches] [PATCH 2/4] sql: raise an integer overflow error during CAST Nikita Pettik
2019-02-20 11:57 ` Nikita Pettik [this message]
2019-02-20 11:57 ` [tarantool-patches] [PATCH 4/4] sql: raise integer overflow error during msgpack decode Nikita Pettik
2019-02-20 18:25   ` [tarantool-patches] " Konstantin Osipov
2019-02-20 18:39     ` n.pettik
2019-02-20 18:46       ` Konstantin Osipov
2019-02-22 18:30 ` [tarantool-patches] Re: [PATCH 0/4] Fix integer overflow behaviour during VDBE execution Vladislav Shpilevoy
2019-02-25 11:58 ` Kirill Yukhin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=326c19ce8abd923ae9d0ed4873abcfdcf06c13d6.1550663540.git.korablev@tarantool.org \
    --to=korablev@tarantool.org \
    --cc=tarantool-patches@freelists.org \
    --cc=v.shpilevoy@tarantool.org \
    --subject='Re: [tarantool-patches] [PATCH 3/4] sql: refactor sqlVdbeMsgpackGet()' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox