[tarantool-patches] [PATCH 1/2] sql: fix tuple format leak

[Nikita Pettik <korablev@tarantool.org>]

Consider following VDBE program (X stands for cursor number):

OP_OpenTEphemeral X ...
OP_Last X ...
OP_IdxInsert X ...
OP_Last X ...

Before this patch, during execution of OP_IdxInsert key, which is held in
cursor, is nullified. However, during OP_Last key is checked on NULL.
If key is really NULL, new memory is allocated and iterator is
nullified (if key is NULL, then this cursor is accounted for brand
new and should be initialized). In this regard, iterator is lost without
being freed.
As a result, some tuples (alongside with iterator itself) remain
referenced, so format can't be freed. Thus, after certain number of
queries, whcih produce ephemeral spaces, tuple format limit is reached and
execution fails.

This patch simply changes signatures of functions which handle
REPLACE/INSERT, so that cursor is not invlolved in this routine.

Closes #3332
---
 src/box/sql.c                                   | 45 +++++++++++++------------
 src/box/sql/tarantoolInt.h                      |  7 ++--
 src/box/sql/vdbe.c                              | 33 ++++++------------
 test/sql-tap/gh-3332-tuple-format-leak.test.lua | 31 +++++++++++++++++
 4 files changed, 69 insertions(+), 47 deletions(-)
 create mode 100755 test/sql-tap/gh-3332-tuple-format-leak.test.lua

diff --git a/src/box/sql.c b/src/box/sql.c
index a6713f1f0..dd0cfcc1a 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -451,17 +451,13 @@ int tarantoolSqlite3EphemeralCreate(BtCursor *pCur, uint32_t field_count,
  *
  * @retval SQLITE_OK on success, SQLITE_TARANTOOL_ERROR otherwise.
  */
-int tarantoolSqlite3EphemeralInsert(BtCursor *pCur)
+int tarantoolSqlite3EphemeralInsert(struct space *space, char *tuple,
+				    char *tuple_end)
 {
-	assert(pCur);
-	assert(pCur->curFlags & BTCF_TEphemCursor);
-	mp_tuple_assert(pCur->key, pCur->key + pCur->nKey);
-
-	if (space_ephemeral_replace(pCur->space, pCur->key,
-				    pCur->key + pCur->nKey) != 0) {
-		diag_log();
+	assert(space != NULL);
+	mp_tuple_assert(tuple, tuple_end);
+	if (space_ephemeral_replace(space, tuple, tuple_end) != 0)
 		return SQL_TARANTOOL_INSERT_FAIL;
-	}
 	return SQLITE_OK;
 }
 
@@ -475,28 +471,29 @@ int tarantoolSqlite3EphemeralDrop(BtCursor *pCur)
 }
 
 static inline int
-insertOrReplace(BtCursor *pCur, enum iproto_type type)
+insertOrReplace(struct space *space, char *tuple, char *tuple_end,
+		enum iproto_type type)
 {
-	assert(pCur->curFlags & BTCF_TaCursor);
+	assert(space != NULL);
 	struct request request;
 	memset(&request, 0, sizeof(request));
-	request.tuple = pCur->key;
-	request.tuple_end = pCur->key + pCur->nKey;
-	request.space_id = pCur->space->def->id;
+	request.tuple = tuple;
+	request.tuple_end = tuple_end;
+	request.space_id = space->def->id;
 	request.type = type;
 	mp_tuple_assert(request.tuple, request.tuple_end);
-	int rc = box_process_rw(&request, pCur->space, NULL);
+	int rc = box_process_rw(&request, space, NULL);
 	return rc == 0 ? SQLITE_OK : SQL_TARANTOOL_INSERT_FAIL;
 }
 
-int tarantoolSqlite3Insert(BtCursor *pCur)
+int tarantoolSqlite3Insert(struct space *space, char *tuple, char *tuple_end)
 {
-	return insertOrReplace(pCur, IPROTO_INSERT);
+	return insertOrReplace(space, tuple, tuple_end, IPROTO_INSERT);
 }
 
-int tarantoolSqlite3Replace(BtCursor *pCur)
+int tarantoolSqlite3Replace(struct space *space, char *tuple, char *tuple_end)
 {
-	return insertOrReplace(pCur, IPROTO_REPLACE);
+	return insertOrReplace(space, tuple, tuple_end, IPROTO_REPLACE);
 }
 
 /*
@@ -1092,8 +1089,14 @@ key_alloc(BtCursor *cur, size_t key_size)
 			diag_set(OutOfMemory, key_size, "malloc", "cur->key");
 			return -1;
 		}
-		cur->iter = NULL;
-		cur->last_tuple = NULL;
+		/*
+		 * Key can be NULL, only if it is a brand new
+		 * cursor. In this case, iterator and tuple must
+		 * also be NULLs, since memory for cursor is
+		 * filled with 0.
+		 */
+		assert(cur->iter == NULL);
+		assert(cur->last_tuple == NULL);
 	} else {
 		char *new_key = realloc(cur->key, key_size);
 		if (new_key == NULL) {
diff --git a/src/box/sql/tarantoolInt.h b/src/box/sql/tarantoolInt.h
index 65acf1198..19c6fdca9 100644
--- a/src/box/sql/tarantoolInt.h
+++ b/src/box/sql/tarantoolInt.h
@@ -74,8 +74,8 @@ int tarantoolSqlite3Previous(BtCursor * pCur, int *pRes);
 int tarantoolSqlite3MovetoUnpacked(BtCursor * pCur, UnpackedRecord * pIdxKey,
 				   int *pRes);
 int tarantoolSqlite3Count(BtCursor * pCur, i64 * pnEntry);
-int tarantoolSqlite3Insert(BtCursor * pCur);
-int tarantoolSqlite3Replace(BtCursor * pCur);
+int tarantoolSqlite3Insert(struct space *space, char *tuple, char *tuple_end);
+int tarantoolSqlite3Replace(struct space *space, char *tuple, char *tuple_end);
 int tarantoolSqlite3Delete(BtCursor * pCur, u8 flags);
 int
 sql_delete_by_key(struct space *space, char *key, uint32_t key_size);
@@ -98,7 +98,8 @@ int tarantoolSqlite3RenameParentTable(int iTab, const char *zOldParentName,
 /* Interface for ephemeral tables. */
 int tarantoolSqlite3EphemeralCreate(BtCursor * pCur, uint32_t filed_count,
 				    struct coll *aColl);
-int tarantoolSqlite3EphemeralInsert(BtCursor * pCur);
+int tarantoolSqlite3EphemeralInsert(struct space *space, char *tuple,
+				    char *tuple_end);
 int tarantoolSqlite3EphemeralDelete(BtCursor * pCur);
 int tarantoolSqlite3EphemeralCount(BtCursor * pCur, i64 * pnEntry);
 int tarantoolSqlite3EphemeralDrop(BtCursor * pCur);
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 823718398..8151e1557 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -4358,33 +4358,26 @@ case OP_IdxInsert: {        /* in2 */
 		rc = sqlite3VdbeSorterWrite(pC, pIn2);
 	} else {
 		BtCursor *pBtCur = pC->uc.pCursor;
-		pBtCur->nKey = pIn2->n;
-		pBtCur->key = pIn2->z;
 		if (pBtCur->curFlags & BTCF_TaCursor) {
 			/* Make sure that memory has been allocated on region. */
 			assert(aMem[pOp->p2].flags & MEM_Ephem);
 			if (pOp->opcode == OP_IdxInsert)
-				rc = tarantoolSqlite3Insert(pBtCur);
+				rc = tarantoolSqlite3Insert(pBtCur->space,
+							    pIn2->z,
+							    pIn2->z + pIn2->n);
 			else
-				rc = tarantoolSqlite3Replace(pBtCur);
+				rc = tarantoolSqlite3Replace(pBtCur->space,
+							     pIn2->z,
+							     pIn2->z + pIn2->n);
 		} else if (pBtCur->curFlags & BTCF_TEphemCursor) {
-			rc = tarantoolSqlite3EphemeralInsert(pBtCur);
+			rc = tarantoolSqlite3EphemeralInsert(pBtCur->space,
+							     pIn2->z,
+							     pIn2->z + pIn2->n);
 		} else {
 			unreachable();
 		}
 		assert(pC->deferredMoveto==0);
 		pC->cacheStatus = CACHE_STALE;
-
-		/*
-		 * Memory for tuple passed to Tarantool is
-		 * allocated in region. This memory will be
-		 * automatically released by Tarantool.
-		 * However, VDBE in the end will also try to
-		 * release it, so pointers should be explicitly
-		 * nullified.
-		 */
-		pBtCur->nKey = 0;
-		pBtCur->key = NULL;
 	}
 
 	if (pOp->p5 & OPFLAG_OE_IGNORE) {
@@ -4424,13 +4417,7 @@ case OP_SInsert: {
 	struct space *space = space_by_id(pOp->p1);
 	assert(space != NULL);
 	assert(space_is_system(space));
-	/* Create surrogate cursor to pass to SQL bindings. */
-	BtCursor surrogate_cur;
-	surrogate_cur.space = space;
-	surrogate_cur.key = pIn2->z;
-	surrogate_cur.nKey = pIn2->n;
-	surrogate_cur.curFlags = BTCF_TaCursor;
-	rc = tarantoolSqlite3Insert(&surrogate_cur);
+	rc = tarantoolSqlite3Insert(space, pIn2->z, pIn2->z + pIn2->n);
 	if (rc)
 		goto abort_due_to_error;
 	if (pOp->p5 & OPFLAG_NCHANGE)
diff --git a/test/sql-tap/gh-3332-tuple-format-leak.test.lua b/test/sql-tap/gh-3332-tuple-format-leak.test.lua
new file mode 100755
index 000000000..05b30aa31
--- /dev/null
+++ b/test/sql-tap/gh-3332-tuple-format-leak.test.lua
@@ -0,0 +1,31 @@
+#!/usr/bin/env tarantool
+test = require("sqltester")
+test:plan(2)
+
+test:do_test(
+    "format-leak-prep",
+    function()
+        box.sql.execute("CREATE TABLE t1(id UNSIGNED BIG INT PRIMARY KEY,\
+                         max_players INTEGER, n_players INTEGER, flags INTEGER);");
+        box.sql.execute("CREATE INDEX IDX_MAX_PLAYERS ON t1(max_players);");
+        box.sql.execute("CREATE INDEX IDX_N_PLAYERS ON t1(n_players);");
+        box.sql.execute("CREATE INDEX IDX_FLAGS ON t1(flags);");
+        for i = 1, 10 do
+            box.sql.execute(string.format("INSERT INTO t1 VALUES (%s, %s, %s, %s);",
+                                          i, 15, 6, 3));
+        end
+    end, {
+
+    })
+
+test:do_test(
+    "format-leak",
+    function()
+        for i = 1, 100000 do
+            box.sql.execute("SELECT id FROM t1 WHERE flags=3 ORDER BY id LIMIT 2");
+        end
+    end, {
+
+    })
+
+test:finish_test()
-- 
2.15.1