* [tarantool-patches] [PATCH 1/2] sql: fix tuple format leak
2018-04-11 19:35 [tarantool-patches] [PATCH 0/2] sql: SQL bindings refactoring Nikita Pettik
@ 2018-04-11 19:35 ` Nikita Pettik
2018-04-12 11:58 ` [tarantool-patches] " Vladislav Shpilevoy
2018-04-11 19:35 ` [tarantool-patches] [PATCH 2/2] sql: refactor cursor closing routine Nikita Pettik
1 sibling, 1 reply; 6+ messages in thread
From: Nikita Pettik @ 2018-04-11 19:35 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy, Nikita Pettik
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
^ permalink raw reply [flat|nested] 6+ messages in thread* [tarantool-patches] [PATCH 2/2] sql: refactor cursor closing routine
2018-04-11 19:35 [tarantool-patches] [PATCH 0/2] sql: SQL bindings refactoring Nikita Pettik
2018-04-11 19:35 ` [tarantool-patches] [PATCH 1/2] sql: fix tuple format leak Nikita Pettik
@ 2018-04-11 19:35 ` Nikita Pettik
2018-04-12 13:28 ` [tarantool-patches] " Vladislav Shpilevoy
1 sibling, 1 reply; 6+ messages in thread
From: Nikita Pettik @ 2018-04-11 19:35 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy, Nikita Pettik
Unite 'close' and 'clear' functions into one. Fix naming and codestyle.
---
src/box/sql.c | 15 +--------------
src/box/sql/cursor.c | 46 +++++++++++++++++++++++++---------------------
src/box/sql/cursor.h | 6 ++++--
src/box/sql/tarantoolInt.h | 1 -
src/box/sql/vdbe.c | 2 +-
src/box/sql/vdbeaux.c | 2 +-
6 files changed, 32 insertions(+), 40 deletions(-)
diff --git a/src/box/sql.c b/src/box/sql.c
index dd0cfcc1a..3138d9991 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -114,8 +114,6 @@ sql_get()
/*********************************************************************
* SQLite cursor implementation on top of Tarantool storage API-s.
- * See the corresponding SQLite function in btree.c for documentation.
- * Ex: sqlite3BtreeCloseCursor -> tarantoolSqlite3CloseCursor
*
* NB: SQLite btree cursor emulation is less than perfect. The problem
* is that btree cursors are more low-level compared to Tarantool
@@ -184,18 +182,6 @@ is_tarantool_error(int rc)
rc == SQL_TARANTOOL_INSERT_FAIL);
}
-int tarantoolSqlite3CloseCursor(BtCursor *pCur)
-{
- assert(pCur->curFlags & BTCF_TaCursor ||
- pCur->curFlags & BTCF_TEphemCursor);
-
- if (pCur->iter)
- box_iterator_free(pCur->iter);
- if (pCur->last_tuple)
- box_tuple_unref(pCur->last_tuple);
- return SQLITE_OK;
-}
-
const void *tarantoolSqlite3PayloadFetch(BtCursor *pCur, u32 *pAmt)
{
assert(pCur->curFlags & BTCF_TaCursor ||
@@ -467,6 +453,7 @@ int tarantoolSqlite3EphemeralDrop(BtCursor *pCur)
assert(pCur);
assert(pCur->curFlags & BTCF_TEphemCursor);
space_delete_ephemeral(pCur->space);
+ pCur->space = NULL;
return SQLITE_OK;
}
diff --git a/src/box/sql/cursor.c b/src/box/sql/cursor.c
index 533bfb587..c38629f3b 100644
--- a/src/box/sql/cursor.c
+++ b/src/box/sql/cursor.c
@@ -31,18 +31,26 @@
#include "sqliteInt.h"
#include "tarantoolInt.h"
+#include "box/tuple.h"
-/*
- * Clear the current cursor position.
+/**
+ * Release tuple, free iterator, invalidate cursor's state.
+ * Note that this routine doesn't nullify space and index:
+ * it is also used during OP_NullRow opcode to refresh given
+ * cursor.
*/
void
-sqlite3ClearCursor(BtCursor * pCur)
+sql_cursor_cleanup(struct BtCursor *cursor)
{
- free(pCur->key);
- pCur->key = 0;
- pCur->iter = NULL;
- pCur->last_tuple = NULL;
- pCur->eState = CURSOR_INVALID;
+ if (cursor->iter)
+ box_iterator_free(cursor->iter);
+ if (cursor->last_tuple)
+ tuple_unref(cursor->last_tuple);
+ free(cursor->key);
+ cursor->key = NULL;
+ cursor->iter = NULL;
+ cursor->last_tuple = NULL;
+ cursor->eState = CURSOR_INVALID;
}
/*
@@ -64,23 +72,19 @@ sqlite3CursorZero(BtCursor * p)
memset(p, 0, sizeof(*p));
}
-/*
+/**
* Close a cursor and invalidate its state. In case of
* ephemeral cursor, corresponding space should be dropped.
*/
-int
-sqlite3CloseCursor(BtCursor * pCur)
+void
+sql_cursor_close(struct BtCursor *cursor)
{
- assert((pCur->curFlags & BTCF_TaCursor) ||
- (pCur->curFlags & BTCF_TEphemCursor));
-
- if (pCur->curFlags & BTCF_TEphemCursor) {
- tarantoolSqlite3EphemeralDrop(pCur);
- }
- tarantoolSqlite3CloseCursor(pCur);
- sqlite3ClearCursor(pCur);
-
- return SQLITE_OK;
+ assert(cursor->space != NULL);
+ assert((cursor->curFlags & BTCF_TaCursor) ||
+ (cursor->curFlags & BTCF_TEphemCursor));
+ if (cursor->curFlags & BTCF_TEphemCursor)
+ tarantoolSqlite3EphemeralDrop(cursor);
+ sql_cursor_cleanup(cursor);
}
#ifndef NDEBUG /* The next routine used only within assert() statements */
diff --git a/src/box/sql/cursor.h b/src/box/sql/cursor.h
index 5b8e7c29d..e5d2aae3a 100644
--- a/src/box/sql/cursor.h
+++ b/src/box/sql/cursor.h
@@ -72,14 +72,16 @@ struct BtCursor {
void sqlite3CursorZero(BtCursor *);
void sqlite3CursorHintFlags(BtCursor *, unsigned);
-int sqlite3CloseCursor(BtCursor *);
+void
+sql_cursor_close(struct BtCursor *cursor);
int sqlite3CursorMovetoUnpacked(BtCursor *, UnpackedRecord * pUnKey, int *pRes);
int sqlite3CursorNext(BtCursor *, int *pRes);
int sqlite3CursorPrevious(BtCursor *, int *pRes);
int sqlite3CursorPayload(BtCursor *, u32 offset, u32 amt, void *);
-void sqlite3ClearCursor(BtCursor *);
+void
+sql_cursor_cleanup(struct BtCursor *cursor);
int sqlite3CursorHasHint(BtCursor *, unsigned int mask);
#ifndef NDEBUG
diff --git a/src/box/sql/tarantoolInt.h b/src/box/sql/tarantoolInt.h
index 19c6fdca9..ca31013ef 100644
--- a/src/box/sql/tarantoolInt.h
+++ b/src/box/sql/tarantoolInt.h
@@ -52,7 +52,6 @@ const char *tarantoolErrorMessage();
int is_tarantool_error(int rc);
/* Storage interface. */
-int tarantoolSqlite3CloseCursor(BtCursor * pCur);
const void *tarantoolSqlite3PayloadFetch(BtCursor * pCur, u32 * pAmt);
/**
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 8151e1557..0fddbcb66 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -4061,7 +4061,7 @@ case OP_NullRow: {
pC->cacheStatus = CACHE_STALE;
if (pC->eCurType==CURTYPE_TARANTOOL) {
assert(pC->uc.pCursor!=0);
- sqlite3ClearCursor(pC->uc.pCursor);
+ sql_cursor_cleanup(pC->uc.pCursor);
}
break;
}
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index bb121a318..40572fc97 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -2217,7 +2217,7 @@ sqlite3VdbeFreeCursor(Vdbe * p, VdbeCursor * pCx)
}
case CURTYPE_TARANTOOL:{
assert(pCx->uc.pCursor != 0);
- sqlite3CloseCursor(pCx->uc.pCursor);
+ sql_cursor_close(pCx->uc.pCursor);
break;
}
}
--
2.15.1
^ permalink raw reply [flat|nested] 6+ messages in thread