[patches] [PATCH 2/3] sql: check identifiers only for printability
AKhatskevich
avkhatskevich at tarantool.org
Mon Feb 5 19:51:37 MSK 2018
This patch makes identifiers in tarantool and delimited identifiers in
SQL obey the same rules.
From this moment, delimited identifiers in SQL are checked only for
printability. For more info, see #2914.
There was option to check any identifier that tokenizer faces, however,
in that case many identifiers could have been checked more than once.
Part of #2126
---
src/box/sql/alter.c | 30 ---------
src/box/sql/build.c | 46 +++++---------
src/box/sql/expr.c | 3 +
src/box/sql/sqliteInt.h | 2 +-
src/box/sql/trigger.c | 2 +-
test/sql-tap/alter.test.lua | 14 +----
test/sql-tap/autoinc.test.lua | 2 +-
test/sql-tap/identifier-characters.test.lua | 97 +++++++++++++++++++++++++++++
test/sql-tap/index1.test.lua | 51 +--------------
test/sql-tap/suite.ini | 2 +-
10 files changed, 125 insertions(+), 124 deletions(-)
create mode 100755 test/sql-tap/identifier-characters.test.lua
diff --git a/src/box/sql/alter.c b/src/box/sql/alter.c
index 293cca232..00d160a33 100644
--- a/src/box/sql/alter.c
+++ b/src/box/sql/alter.c
@@ -62,24 +62,6 @@ reloadTableSchema(Parse * pParse, Table * pTab, const char *zName)
sqlite3VdbeAddRenameTableOp(v, pTab->tnum, zNewName);
}
-/*
- * Parameter zName is the name of a table that is about to be altered
- * (either with ALTER TABLE ... RENAME TO or ALTER TABLE ... ADD COLUMN).
- * If the table is a system table, this function leaves an error message
- * in pParse->zErr (system tables may not be altered) and returns non-zero.
- *
- * Or, if zName is not a system table, zero is returned.
- */
-static int
-isSystemTable(Parse * pParse, const char *zName)
-{
- if (0 == sqlite3StrNICmp(zName, "_", 1)) {
- sqlite3ErrorMsg(pParse, "table %s may not be altered", zName);
- return 1;
- }
- return 0;
-}
-
/*
* Generate code to implement the "ALTER TABLE xxx RENAME TO yyy"
* command.
@@ -124,15 +106,6 @@ sqlite3AlterRenameTable(Parse * pParse, /* Parser context. */
goto exit_rename_table;
}
- /* Make sure it is not a system table being altered, or a reserved name
- * that the table is being renamed to.
- */
- if (SQLITE_OK != isSystemTable(pParse, pTab->zName)) {
- goto exit_rename_table;
- }
- if (SQLITE_OK != sqlite3CheckObjectName(pParse, zName)) {
- goto exit_rename_table;
- }
#ifndef SQLITE_OMIT_VIEW
if (pTab->pSelect) {
sqlite3ErrorMsg(pParse, "view %s may not be altered",
@@ -318,9 +291,6 @@ sqlite3AlterBeginAddColumn(Parse * pParse, SrcList * pSrc)
sqlite3ErrorMsg(pParse, "Cannot add a column to a view");
goto exit_begin_add_column;
}
- if (SQLITE_OK != isSystemTable(pParse, pTab->zName)) {
- goto exit_begin_add_column;
- }
assert(pTab->addColOffset > 0);
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index e4c1e61ce..6674e98a7 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -47,6 +47,7 @@
#include "vdbeInt.h"
#include "tarantoolInt.h"
#include "box/session.h"
+#include "box/identifier.h"
/*
* This routine is called after a single SQL statement has been
@@ -583,18 +584,20 @@ sqlite3FindDb(sqlite3 * db, Token * pName)
/*
* This routine is used to check if the UTF-8 string zName is a legal
- * unqualified name for a new schema object (table, index, view or
- * trigger). All names are legal except those that begin with the string
- * "sqlite_" (in upper, lower or mixed case). This portion of the namespace
- * is reserved for internal use.
+ * unqualified name for an identifier.
+ * Some objects may not be checked, because they are validated in Tarantool.
+ * (e.g. table, index, column name of a real table)
+ * All names are legal except those that cantain non-printable
+ * characters or have length greater than BOX_NAME_MAX.
*/
int
-sqlite3CheckObjectName(Parse * pParse, char *zName)
+sqlite3CheckIdentifierName(Parse *pParse, char *zName)
{
- if (!pParse->db->init.busy && pParse->nested == 0
- && 0 == sqlite3StrNICmp(zName, "_", 1)) {
+ ssize_t len = strlen(zName);
+
+ if (len > BOX_NAME_MAX || identifier_check(zName, len) != 0) {
sqlite3ErrorMsg(pParse,
- "object name reserved for internal use: %s",
+ "identifier name is invalid: %s",
zName);
return SQLITE_ERROR;
}
@@ -668,9 +671,6 @@ sqlite3StartTable(Parse *pParse, Token *pName, int noErr)
pParse->sNameToken = *pName;
if (zName == 0)
return;
- if (SQLITE_OK != sqlite3CheckObjectName(pParse, zName)) {
- goto begin_table_error;
- }
#ifndef SQLITE_OMIT_AUTHORIZATION
assert(isTemp == 0 || isTemp == 1);
assert(isView == 0 || (isView == 1 && isTemp == 0));
@@ -2447,11 +2447,6 @@ sqlite3DropTable(Parse * pParse, SrcList * pName, int isView, int noErr)
}
}
#endif
- if (sqlite3StrNICmp(pTab->zName, "_", 1) == 0) {
- sqlite3ErrorMsg(pParse, "table %s may not be dropped",
- pTab->zName);
- goto exit_drop_table;
- }
#ifndef SQLITE_OMIT_VIEW
/* Ensure DROP TABLE is not used on a view, and DROP VIEW is not used
* on a table.
@@ -2956,16 +2951,6 @@ sqlite3CreateIndex(Parse * pParse, /* All information about this parse */
assert(pTab != 0);
assert(pParse->nErr == 0);
- if (sqlite3CheckObjectName(pParse, pTab->zName) != 0
- && db->init.busy == 0
-#if SQLITE_USER_AUTHENTICATION
- && sqlite3UserAuthTable(pTab->zName) == 0
-#endif
- && sqlite3StrNICmp(&pTab->zName[7], "altertab_", 9) != 0) {
- sqlite3ErrorMsg(pParse, "table %s may not be indexed",
- pTab->zName);
- goto exit_create_index;
- }
#ifndef SQLITE_OMIT_VIEW
if (pTab->pSelect) {
sqlite3ErrorMsg(pParse, "views may not be indexed");
@@ -2990,9 +2975,6 @@ sqlite3CreateIndex(Parse * pParse, /* All information about this parse */
if (zName == 0)
goto exit_create_index;
assert(pName->z != 0);
- if (SQLITE_OK != sqlite3CheckObjectName(pParse, zName)) {
- goto exit_create_index;
- }
if (!db->init.busy) {
if (sqlite3FindTable(db, zName) != 0) {
sqlite3ErrorMsg(pParse,
@@ -3988,6 +3970,12 @@ sqlite3Savepoint(Parse * pParse, int op, Token * pName)
sqlite3DbFree(pParse->db, zName);
return;
}
+ if (op == SAVEPOINT_BEGIN &&
+ sqlite3CheckIdentifierName(pParse, zName)
+ != SQLITE_OK) {
+ sqlite3ErrorMsg(pParse, "bad savepoint name");
+ return;
+ }
sqlite3VdbeAddOp4(v, OP_Savepoint, op, 0, 0, zName, P4_DYNAMIC);
}
}
diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index 42e7df74d..7edb96dc8 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -1850,6 +1850,9 @@ sqlite3ExprListSetName(Parse * pParse, /* Parsing context */
pItem->zName = sqlite3DbStrNDup(pParse->db, pName->z, pName->n);
if (dequote)
sqlite3NormalizeName(pItem->zName);
+ /* n = 0 in case of select * */
+ if (pName->n != 0)
+ sqlite3CheckIdentifierName(pParse, pItem->zName);
}
}
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index 4dee24809..310a3b7dc 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -3438,7 +3438,7 @@ Expr *sqlite3ExprAddCollateToken(Parse * pParse, Expr *, const Token *, int);
Expr *sqlite3ExprAddCollateString(Parse *, Expr *, const char *);
Expr *sqlite3ExprSkipCollate(Expr *);
int sqlite3CheckCollSeq(Parse *, struct coll *);
-int sqlite3CheckObjectName(Parse *, char *);
+int sqlite3CheckIdentifierName(Parse *, char *);
void sqlite3VdbeSetChanges(sqlite3 *, int);
int sqlite3AddInt64(i64 *, i64);
int sqlite3SubInt64(i64 *, i64);
diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c
index aeb80d0cf..128cc05dd 100644
--- a/src/box/sql/trigger.c
+++ b/src/box/sql/trigger.c
@@ -120,7 +120,7 @@ sqlite3BeginTrigger(Parse * pParse, /* The parse context of the CREATE TRIGGER s
* specified name exists
*/
zName = sqlite3NameFromToken(db, pName);
- if (!zName || SQLITE_OK != sqlite3CheckObjectName(pParse, zName)) {
+ if (!zName || SQLITE_OK != sqlite3CheckIdentifierName(pParse, zName)) {
goto trigger_cleanup;
}
if (sqlite3HashFind(&(db->mdb.pSchema->trigHash), zName)) {
diff --git a/test/sql-tap/alter.test.lua b/test/sql-tap/alter.test.lua
index 18de92d00..1a4ceaaf3 100755
--- a/test/sql-tap/alter.test.lua
+++ b/test/sql-tap/alter.test.lua
@@ -1,6 +1,6 @@
#!/usr/bin/env tarantool
test = require("sqltester")
-test:plan(42)
+test:plan(41)
test:do_execsql_test(
"alter-1.1",
@@ -93,17 +93,7 @@ test:do_catchsql_test(
ALTER TABLE "_space" RENAME TO space;
]], {
-- <alter-2.3>
- 1, "table _space may not be altered"
- -- </alter-2.3>
- })
-
-test:do_catchsql_test(
- "alter-2.4",
- [[
- ALTER TABLE t3 RENAME TO _test;
- ]], {
- -- <alter-2.3>
- 1, "object name reserved for internal use: _TEST"
+ 1, "/logic error/"
-- </alter-2.3>
})
diff --git a/test/sql-tap/autoinc.test.lua b/test/sql-tap/autoinc.test.lua
index 2fa48e657..030c12725 100755
--- a/test/sql-tap/autoinc.test.lua
+++ b/test/sql-tap/autoinc.test.lua
@@ -71,7 +71,7 @@ test:do_catchsql_test(
DROP TABLE "_sequence"
]], {
-- <autoinc-1.5>
- 1, "table _sequence may not be dropped"
+ 1, "/system space/"
-- </autoinc-1.5>
})
diff --git a/test/sql-tap/identifier-characters.test.lua b/test/sql-tap/identifier-characters.test.lua
new file mode 100755
index 000000000..31b45c6e7
--- /dev/null
+++ b/test/sql-tap/identifier-characters.test.lua
@@ -0,0 +1,97 @@
+#!/usr/bin/env tarantool
+test = require("sqltester")
+identifier = require("identifier")
+test:plan(9)
+
+local test_prefix = "identifier_char-"
+
+local testcases = {
+ -- table creation testcase
+ {"table",
+ -- create
+ function (id)
+ -- sql autogenerated index name rules add "sqlite_autoindex_") prefix
+ if string.len(id) == box.schema.NAME_MAX then
+ id = string.sub(id, string.len(id))
+ end
+ test:execsql(string.format("create table \"%s\" (a primary key);", id))
+ end,
+ -- cleanup
+ function (id)
+ if string.len(id) == box.schema.NAME_MAX then
+ id = string.sub(id, string.len(id))
+ end
+ test:execsql(string.format("drop table \"%s\";", id))
+ end},
+ {"column name",
+ function (id)
+ test:execsql(string.format("create table table1(a primary key, \"%s\");", id))
+ end,
+ function (id)
+ test:execsql(string.format("drop table table1;", id))
+ end},
+ {"view as select",
+ function (id)
+ test:execsql(string.format("create view \"%s\" as select * from test;", id))
+ end,
+ function (id)
+ test:execsql(string.format("drop view \"%s\";", id))
+ end},
+ {"view column name",
+ function (id)
+ test:execsql(string.format("create view v1 (\"%s\") as select b from test;", id))
+ end,
+ function (id)
+ test:execsql(string.format("drop view v1;", id))
+ end},
+ {"as column name",
+ function (id)
+ test:execsql(string.format("create view v1 as select b as \"%s\" from test;", id))
+ end,
+ function (id)
+ test:execsql(string.format("drop view v1;", id))
+ end},
+ {"index name",
+ function (id)
+ test:execsql(string.format("create index \"%s\" on test(b,c);", id))
+ end,
+ function (id)
+ test:execsql(string.format("drop index \"%s\" on test", id))
+ end},
+ {"savepoint name",
+ function (id)
+ test:execsql("begin")
+ local ok, res = pcall(test.execsql, test, string.format("savepoint \"%s\"", id))
+ test:execsql("commit")
+ if ok == false then error(res) end
+ end,
+ function (id) end},
+ {"trigger name",
+ function (id)
+ test:execsql(string.format([[
+ CREATE TRIGGER "%s" UPDATE ON test
+ BEGIN
+ SELECT RAISE(ABORT, 'newer');
+ END;]], id));
+ end,
+ function (id)
+ test:execsql(string.format("drop trigger \"%s\"", id))
+ end}
+}
+
+test:do_execsql_test(
+ test_prefix.."preparition",
+ "create table test(a primary key, b, c)")
+
+for _, testcase in ipairs(testcases) do
+ test:do_test(
+ test_prefix..testcase[1],
+ function ()
+ return identifier.run_test(
+ testcase[2],
+ testcase[3])
+ end,
+ "All tests passed")
+end
+
+test:finish_test()
diff --git a/test/sql-tap/index1.test.lua b/test/sql-tap/index1.test.lua
index 7bfcf77b1..cf5443e3c 100755
--- a/test/sql-tap/index1.test.lua
+++ b/test/sql-tap/index1.test.lua
@@ -1,6 +1,6 @@
#!/usr/bin/env tarantool
test = require("sqltester")
-test:plan(83)
+test:plan(79)
--!./tcltestrunner.lua
-- 2001 September 15
@@ -1051,58 +1051,11 @@ test:do_catchsql_test(
-- </index-17.4>
})
--- The following tests ensure that it is not possible to explicitly name
--- a schema object with a name beginning with "sqlite_". Granted that is a
--- little outside the focus of this test scripts, but this has got to be
--- tested somewhere.
-test:do_catchsql_test(
- "index-18.1",
- [[
- CREATE TABLE _sqlite_t1(a, b, c);
- ]], {
- -- <index-18.1>
- 1, "object name reserved for internal use: _SQLITE_T1"
- -- </index-18.1>
- })
-
-test:do_catchsql_test(
- "index-18.2",
- [[
- CREATE INDEX _sqlite_i1 ON t7(c);
- ]], {
- -- <index-18.2>
- 1, "object name reserved for internal use: _SQLITE_I1"
- -- </index-18.2>
- })
-
-test:do_catchsql_test(
- "index-18.3",
- [[
- CREATE VIEW _sqlite_v1 AS SELECT * FROM t7;
- ]], {
- -- <index-18.3>
- 1, "object name reserved for internal use: _SQLITE_V1"
- -- </index-18.3>
- })
-
-test:do_catchsql_test(
- "index-18.4",
- [[
- CREATE TRIGGER _sqlite_tr1 BEFORE INSERT ON t7 BEGIN SELECT 1; END;
- ]], {
- -- <index-18.4>
- 1, "object name reserved for internal use: _SQLITE_TR1"
- -- </index-18.4>
- })
-
test:do_execsql_test(
- "index-18.5",
+ "index-17.5",
[[
DROP TABLE t7;
]], {
- -- <index-18.5>
-
- -- </index-18.5>
})
diff --git a/test/sql-tap/suite.ini b/test/sql-tap/suite.ini
index 64713cdc3..0bc9e83dc 100644
--- a/test/sql-tap/suite.ini
+++ b/test/sql-tap/suite.ini
@@ -1,5 +1,5 @@
[default]
core = app
description = Database tests with #! using TAP
-lua_libs = lua/sqltester.lua ../sql/lua/sql_tokenizer.lua
+lua_libs = lua/sqltester.lua ../sql/lua/sql_tokenizer.lua ../box/lua/identifier.lua
is_parallel = True
--
2.14.1
More information about the Tarantool-patches
mailing list