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 E9BE32D1F9 for ; Wed, 11 Apr 2018 15:35:09 -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 SU0eugqFioCC for ; Wed, 11 Apr 2018 15:35:09 -0400 (EDT) Received: from smtp50.i.mail.ru (smtp50.i.mail.ru [94.100.177.110]) (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 83D7D2D1F1 for ; Wed, 11 Apr 2018 15:35:09 -0400 (EDT) From: Nikita Pettik Subject: [tarantool-patches] [PATCH 1/2] sql: fix tuple format leak Date: Wed, 11 Apr 2018 22:35:04 +0300 Message-Id: <8d8d9f3056d4af42638ab403de6f19d2eb76f07a.1523468339.git.korablev@tarantool.org> In-Reply-To: References: In-Reply-To: References: 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: tarantool-patches@freelists.org Cc: v.shpilevoy@tarantool.org, 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