[patches] [PATCH 2/3] sql: check identifiers only for printability

[Kirill Yukhin]

Hello Alex,

My comments are inlined.

On 05 Feb 19:51, AKhatskevich wrote:
>   This patch makes identifiers in tarantool and delimited identifiers in
> SQL obey the same rules.
Extra space in beginning of the line.

>   From this moment, delimited identifiers in SQL are checked only for
> printability. For more info, see #2914.
Ditto.

>   There was option to check any identifier that tokenizer faces, however,
> in that case many identifiers could have been checked more than once.
Ditto.

> 
> 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 * */
Extra star. Could you pls re-pharse the comment.

> +		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
More than 80 chars.

> +		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))
More than 80 chars. We're about to start working on statuc typing. Pls, put
specific type to columns in all new test cases.

> +	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))
Ditto.

> +	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))
More than 80 chars.

> +	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))
More than 80 chars.

> +	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))
Ditto.

> +	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))
Ditto.

> +	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))
Ditto.

> +		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
>