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 9A96A22E61 for ; Wed, 25 Apr 2018 12:52:30 -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 Qmtq4QSCLgeK for ; Wed, 25 Apr 2018 12:52:30 -0400 (EDT) Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (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 2AA7322E53 for ; Wed, 25 Apr 2018 12:52:30 -0400 (EDT) From: Kirill Shcherbatov Subject: [tarantool-patches] [PATCH v3 4/4] sql: Region-based allocations. Date: Wed, 25 Apr 2018 19:52:23 +0300 Message-Id: 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, Kirill Shcherbatov Function sql_field_retrieve could leak names in error cases. Let allocate all memory with region allocator. Needed for #3272. --- src/box/sql.c | 44 +++++++++++++++++++++++++++++++------------- src/box/sql.h | 9 +++++++++ src/box/sql/build.c | 37 ++++++++++++++++++++++++------------- src/box/sql/prepare.c | 26 ++++++++++++++++---------- src/box/sql/select.c | 24 +++++++++++++++++++++--- src/box/sql/sqliteInt.h | 2 ++ src/box/sql/tokenize.c | 3 +++ src/box/sql/trigger.c | 2 ++ 8 files changed, 108 insertions(+), 39 deletions(-) diff --git a/src/box/sql.c b/src/box/sql.c index 6d4255e..2893d70 100644 --- a/src/box/sql.c +++ b/src/box/sql.c @@ -1685,22 +1685,43 @@ space_column_default_expr(uint32_t space_id, uint32_t fieldno) return space->def->fields[fieldno].default_value_expr; } +struct space_def * +sql_ephemeral_space_def_new(Parse *parser) +{ + struct space_def *def = NULL; + struct region *region = &fiber()->gc; + size_t size = sizeof(struct space_def) + 1; + def = (struct space_def *)region_alloc(region, size); + if (def != NULL) { + memset(def, 0, size); + def->dict = + (struct tuple_dictionary *) + region_alloc(region, + sizeof(struct tuple_dictionary)); + } + if (def == NULL || def->dict == NULL) { + parser->rc = SQLITE_NOMEM_BKPT; + parser->nErr++; + return NULL; + } + def->dict->refs = 1; + def->opts.temporary = true; + return def; +} + Table * sql_ephemeral_table_new(Parse *parser) { sqlite3 *db = parser->db; struct space_def *def = NULL; Table *table = sqlite3DbMallocZero(db, sizeof(Table)); - if (table != NULL) { - def = space_def_new(0, 0, 0, NULL, 0, NULL, 0, - &space_opts_default, NULL, 0); - } + if (table != NULL) + def = sql_ephemeral_space_def_new(parser); if (def == NULL) { sqlite3DbFree(db, table); - parser->rc = SQLITE_NOMEM_BKPT; - parser->nErr++; return NULL; } + table->def = def; return table; } @@ -1708,6 +1729,9 @@ sql_ephemeral_table_new(Parse *parser) int sql_table_def_rebuild(struct sqlite3 *db, struct Table *pTable) { + assert(pTable->def->opts.temporary == true); + + /* All allocations are on region. */ struct space_def *old_def = pTable->def; struct space_def *new_def = NULL; new_def = space_def_new(old_def->id, old_def->uid, @@ -1719,13 +1743,7 @@ sql_table_def_rebuild(struct sqlite3 *db, struct Table *pTable) sqlite3OomFault(db); return -1; } - struct field_def *fields = old_def->fields; - for (uint32_t i = 0; i < old_def->field_count; ++i) { - sqlite3DbFree(db, fields[i].default_value); - sqlite3DbFree(db, fields[i].name); - } - space_def_delete(old_def); - sqlite3DbFree(db, fields); pTable->def = new_def; + pTable->def->opts.temporary = false; return 0; } diff --git a/src/box/sql.h b/src/box/sql.h index 9fb3ad1..410653b 100644 --- a/src/box/sql.h +++ b/src/box/sql.h @@ -155,6 +155,15 @@ struct Table * sql_ephemeral_table_new(struct Parse *parser); /** + * Create and initialize a new ephemeric space_def object. + * @param pParse SQL Parser object. + * @retval NULL on memory allocation error, Parser state changed. + * @retval not NULL on success. + */ +struct space_def * +sql_ephemeral_space_def_new(struct Parse *parser); + +/** * Rebuild struct def in Table with memory allocated on a single * malloc. Fields and strings are expected to be allocated with * sqlite3DbMalloc. diff --git a/src/box/sql/build.c b/src/box/sql/build.c index 584e6b1..4db4356 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -386,7 +386,9 @@ deleteTable(sqlite3 * db, Table * pTable) sqlite3DbFree(db, pTable->zColAff); sqlite3SelectDelete(db, pTable->pSelect); sqlite3ExprListDelete(db, pTable->pCheck); - if (pTable->def != NULL) + /* Do not delete pTable->def allocated not on region. */ + assert(pTable->def != NULL); + if (pTable->def->opts.temporary == false) space_def_delete(pTable->def); sqlite3DbFree(db, pTable); @@ -483,6 +485,7 @@ sqlite3PrimaryKeyIndex(Table * pTab) /** * Create and initialize a new SQL Table object. + * All memory is allocated on region. * @param parser SQL Parser object. * @param name Table to create name. * @retval NULL on memory allocation error. @@ -603,29 +606,33 @@ sqlite3StartTable(Parse *pParse, Token *pName, int noErr) static struct field_def * sql_field_retrieve(Parse *parser, Table *table, uint32_t id) { - sqlite3 *db = parser->db; struct field_def *field; assert(table->def != NULL); assert(id < SQLITE_MAX_COLUMN); if (id >= table->def->exact_field_count) { - uint32_t columns = table->def->exact_field_count; - columns = (columns > 0) ? 2 * columns : 1; - field = sqlite3DbRealloc(db, table->def->fields, - columns * sizeof(table->def->fields[0])); + uint32_t columns_new = table->def->exact_field_count; + columns_new = (columns_new > 0) ? 2 * columns_new : 1; + struct region *region = &fiber()->gc; + field = region_alloc(region, + columns_new * sizeof(table->def->fields[0])); if (field == NULL) { parser->rc = SQLITE_NOMEM_BKPT; parser->nErr++; return NULL; } - for (uint32_t i = columns / 2; i < columns; i++) { + for (uint32_t i = 0; i < table->def->exact_field_count; i++) { + memcpy(&field[i], &table->def->fields[i], + sizeof(struct field_def)); + } + for (uint32_t i = columns_new / 2; i < columns_new; i++) { memcpy(&field[i], &field_def_default, sizeof(struct field_def)); } table->def->fields = field; - table->def->exact_field_count = columns; + table->def->exact_field_count = columns_new; } field = &table->def->fields[id]; @@ -651,6 +658,7 @@ sqlite3AddColumn(Parse * pParse, Token * pName, Token * pType) sqlite3 *db = pParse->db; if ((p = pParse->pNewTable) == 0) return; + assert(p->def->opts.temporary == true); #if SQLITE_MAX_COLUMN if ((int)p->def->field_count + 1 > db->aLimit[SQLITE_LIMIT_COLUMN]) { sqlite3ErrorMsg(pParse, "too many columns on %s", p->zName); @@ -659,7 +667,8 @@ sqlite3AddColumn(Parse * pParse, Token * pName, Token * pType) #endif if (sql_field_retrieve(pParse, p, (uint32_t) p->def->field_count) == NULL) return; - z = sqlite3DbMallocRaw(db, pName->n + 1); + struct region *region = &fiber()->gc; + z = region_alloc(region, pName->n + 1); if (z == 0) return; memcpy(z, pName->z, pName->n); @@ -668,7 +677,6 @@ sqlite3AddColumn(Parse * pParse, Token * pName, Token * pType) for (i = 0; i < (int)p->def->field_count; i++) { if (strcmp(z, p->def->fields[i].name) == 0) { sqlite3ErrorMsg(pParse, "duplicate column name: %s", z); - sqlite3DbFree(db, z); return; } } @@ -677,10 +685,8 @@ sqlite3AddColumn(Parse * pParse, Token * pName, Token * pType) aNew = sqlite3DbRealloc(db, p->aCol, (p->def->field_count + 8) * sizeof(p->aCol[0])); - if (aNew == 0) { - sqlite3DbFree(db, z); + if (aNew == 0) return; - } p->aCol = aNew; } pCol = &p->aCol[p->def->field_count]; @@ -848,6 +854,7 @@ sqlite3AddDefaultValue(Parse * pParse, ExprSpan * pSpan) Table *p; sqlite3 *db = pParse->db; p = pParse->pNewTable; + assert(p->def->opts.temporary == true); if (p != 0) { if (!sqlite3ExprIsConstantOrFunction (pSpan->pExpr, db->init.busy)) { @@ -1956,6 +1963,7 @@ sqlite3EndTable(Parse * pParse, /* Parse context */ if (db->init.busy) { Table *pOld; Schema *pSchema = p->pSchema; + assert(p->def->opts.temporary == false); pOld = sqlite3HashInsert(&pSchema->tblHash, p->zName, p); if (pOld) { assert(p == pOld); /* Malloc must have failed inside HashInsert() */ @@ -2127,6 +2135,7 @@ sqlite3ViewGetColumnNames(Parse * pParse, Table * pTable) pSel); } } else if (pSelTab) { + assert(pTable->def->opts.temporary == false); /* CREATE VIEW name AS... without an argument list. Construct * the column names from the SELECT statement that defines the view. */ @@ -2167,6 +2176,8 @@ sqliteViewResetAll(sqlite3 * db) if (pTab->pSelect) { sqlite3DeleteColumnNames(db, pTab); struct space_def *old_def = pTab->def; + assert(old_def->opts.temporary == false); + /* Ignore fields allocated on region */ pTab->def = space_def_new(old_def->id, old_def->uid, 0, old_def->name, diff --git a/src/box/sql/prepare.c b/src/box/sql/prepare.c index 2ab8751..7dc14f6 100644 --- a/src/box/sql/prepare.c +++ b/src/box/sql/prepare.c @@ -211,17 +211,19 @@ sqlite3InitDatabase(sqlite3 * db) void sqlite3ParserReset(Parse * pParse) { - if (pParse) { - sqlite3 *db = pParse->db; - sqlite3DbFree(db, pParse->aLabel); - sqlite3ExprListDelete(db, pParse->pConstExpr); - if (db) { - assert(db->lookaside.bDisable >= - pParse->disableLookaside); - db->lookaside.bDisable -= pParse->disableLookaside; - } - pParse->disableLookaside = 0; + if (pParse == NULL) + return; + sqlite3 *db = pParse->db; + sqlite3DbFree(db, pParse->aLabel); + sqlite3ExprListDelete(db, pParse->pConstExpr); + if (db) { + assert(db->lookaside.bDisable >= + pParse->disableLookaside); + db->lookaside.bDisable -= pParse->disableLookaside; } + pParse->disableLookaside = 0; + struct region *region = &fiber()->gc; + region_truncate(region, pParse->region_initial_size); } /* @@ -265,6 +267,10 @@ sqlite3Prepare(sqlite3 * db, /* Database handle. */ * works even if READ_UNCOMMITTED is set. */ sParse.db = db; + /* Store region initial size to revert future allocations */ + struct region *region = &fiber()->gc; + sParse.region_initial_size = region_used(region); + if (nBytes >= 0 && (nBytes == 0 || zSql[nBytes - 1] != 0)) { char *zSqlCopy; int mxLen = db->aLimit[SQLITE_LIMIT_SQL_LENGTH]; diff --git a/src/box/sql/select.c b/src/box/sql/select.c index 9eeff8e..03bfcf9 100644 --- a/src/box/sql/select.c +++ b/src/box/sql/select.c @@ -1822,9 +1822,20 @@ sqlite3ColumnsFromExprList(Parse * pParse, /* Parsing context */ aCol = 0; } assert(nCol == (i16) nCol); + + struct region *region = &fiber()->gc; assert(pTable->def->fields == NULL); + if (pTable->def->opts.temporary == false) { + /* CREATE VIEW name AS... without an argument list. Construct + * the column names from the SELECT statement that defines the view. + */ + pTable->def->field_count = 0; + space_def_delete(pTable->def); + pTable->def = sql_ephemeral_space_def_new(pParse); + } pTable->def->fields = - sqlite3DbMallocZero(db, nCol*sizeof(pTable->def->fields[0])); + region_alloc(region, nCol*sizeof(pTable->def->fields[0])); + memset(pTable->def->fields, 0, nCol*sizeof(pTable->def->fields[0])); pTable->def->field_count = (uint32_t)nCol; pTable->aCol = aCol; @@ -1877,9 +1888,16 @@ sqlite3ColumnsFromExprList(Parse * pParse, /* Parsing context */ if (cnt > 3) sqlite3_randomness(sizeof(cnt), &cnt); } - pTable->def->fields[i].name = zName; - if (zName && sqlite3HashInsert(&ht, zName, pCol) == pCol) { + uint32_t zNameLen = (uint32_t)strlen(zName); + if (zName && sqlite3HashInsert(&ht, zName, pCol) == pCol) + sqlite3OomFault(db); + pTable->def->fields[i].name = + region_alloc(region, zNameLen + 1); + if (pTable->def->fields[i].name == NULL) { sqlite3OomFault(db); + } else { + memcpy(pTable->def->fields[i].name, zName, zNameLen); + pTable->def->fields[i].name[zNameLen] = '\0'; } } sqlite3HashClear(&ht); diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h index 94d5c80..520b74c 100644 --- a/src/box/sql/sqliteInt.h +++ b/src/box/sql/sqliteInt.h @@ -2933,6 +2933,8 @@ struct Parse { u8 eTriggerOp; /* TK_UPDATE, TK_INSERT or TK_DELETE */ u8 eOrconf; /* Default ON CONFLICT policy for trigger steps */ u8 disableTriggers; /* True to disable triggers */ + /** Region size at the Parser launch */ + size_t region_initial_size; /************************************************************************** * Fields above must be initialized to zero. The fields that follow, diff --git a/src/box/sql/tokenize.c b/src/box/sql/tokenize.c index c77aa9b..9055bd0 100644 --- a/src/box/sql/tokenize.c +++ b/src/box/sql/tokenize.c @@ -664,6 +664,9 @@ sql_expr_compile(sqlite3 *db, const char *expr, struct Expr **result) memset(&parser, 0, sizeof(parser)); parser.db = db; parser.parse_only = true; + struct region *region = &fiber()->gc; + parser.region_initial_size = region_used(region); + char *unused; if (sqlite3RunParser(&parser, stmt, &unused) != SQLITE_OK) { diag_set(ClientError, ER_SQL_EXECUTE, expr); diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c index 28c56db..9b03be1 100644 --- a/src/box/sql/trigger.c +++ b/src/box/sql/trigger.c @@ -873,6 +873,8 @@ codeRowTrigger(Parse * pParse, /* Current parse context */ pSubParse->pToplevel = pTop; pSubParse->eTriggerOp = pTrigger->op; pSubParse->nQueryLoop = pParse->nQueryLoop; + struct region *region = &fiber()->gc; + pSubParse->region_initial_size = region_used(region); v = sqlite3GetVdbe(pSubParse); if (v) { -- 2.7.4