[patches] [PATCH v2 2/3] sql: check identifiers only for printability
AKhatskevich
avkhatskevich at tarantool.org
Tue Feb 6 14:20:13 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 | 108 ++++++++++++++++++++++++++++
test/sql-tap/index1.test.lua | 51 +------------
test/sql-tap/suite.ini | 2 +-
10 files changed, 136 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 bfcce9f92..b690c4757 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));
@@ -2449,11 +2449,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.
@@ -2958,16 +2953,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");
@@ -2992,9 +2977,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,
@@ -3990,6 +3972,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..8192ab28c 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);
+ /* `pName->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 22abdcdf9..ef092c5d3 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",
@@ -95,17 +95,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 adf82cd7a..d9eed8d85 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..c29722963
--- /dev/null
+++ b/test/sql-tap/identifier-characters.test.lua
@@ -0,0 +1,108 @@
+#!/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 5180f2f9f..edf4adf50 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
@@ -1076,58 +1076,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