[tarantool-patches] [PATCH 2/3] sql: allow only string-like arguments for concatenation

[Nikita Pettik <korablev@tarantool.org>]

Original SQLite operator of concatenation accepts all types of
arguments. If type of parameter is not TEXT, it is implicitly converted
to TEXT (except for NULLs). That contradicts ANSI (it is regulated by
[1]), so lets allow only TEXT and BLOB as argument type for
concatenation.  Moreover, they both must be of the same type at the same
time (i.e. both TEXT or BLOB).

[1] SQL ANSI 2013, 9.5 Result of data type combination

Part of #3544
---
 src/box/errcode.h               |  1 +
 src/box/sql/vdbe.c              | 54 +++++++++++++++++++++++++++++++++++++++--
 test/box/misc.result            |  1 +
 test/sql-tap/autoinc.test.lua   | 12 ++++-----
 test/sql-tap/e_select1.test.lua |  4 +--
 test/sql-tap/func.test.lua      |  4 +--
 test/sql-tap/sort.test.lua      |  6 ++---
 test/sql-tap/tkt2192.test.lua   |  4 +--
 test/sql-tap/trigger5.test.lua  |  4 +--
 test/sql/types.result           | 32 ++++++++++++++++++++++++
 test/sql/types.test.lua         | 12 +++++++++
 11 files changed, 115 insertions(+), 19 deletions(-)

diff --git a/src/box/errcode.h b/src/box/errcode.h
index f7dbb948e..8ef60e861 100644
--- a/src/box/errcode.h
+++ b/src/box/errcode.h
@@ -227,6 +227,7 @@ struct errcode_record {
 	/*172 */_(ER_ROWID_OVERFLOW,            "Rowid is overflowed: too many entries in ephemeral space") \
 	/*173 */_(ER_DROP_COLLATION,		"Can't drop collation %s : %s") \
 	/*174 */_(ER_ILLEGAL_COLLATION_MIX,	"Illegal mix of collations") \
+	/*175 */_(ER_INCONSISTENT_TYPES,	"Inconsistent types: expected %s got %s") \
 
 /*
  * !IMPORTANT! Please follow instructions at start of the file
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index c370b81ec..37955858d 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -619,6 +619,32 @@ vdbe_add_new_autoinc_id(struct Vdbe *vdbe, int64_t id)
 	return 0;
 }
 
+/**
+ * Simple type to str convertor. It is used to simplify
+ * error reporting.
+ */
+static char *
+mem_type_to_str(const struct Mem *p)
+{
+	assert(p != NULL);
+	switch (p->flags & MEM_PURE_TYPE_MASK) {
+	case MEM_Null:
+		return "NULL";
+	case MEM_Str:
+		return "TEXT";
+	case MEM_Int:
+		return "INTEGER";
+	case MEM_Real:
+		return "REAL";
+	case MEM_Blob:
+		return "BLOB";
+	case MEM_Bool:
+		return "BOOLEAN";
+	default:
+		unreachable();
+	}
+}
+
 /*
  * Execute as much of a VDBE program as we can.
  * This is the core of sql_step().
@@ -1515,6 +1541,9 @@ case OP_ResultRow: {
  * It is illegal for P1 and P3 to be the same register. Sometimes,
  * if P3 is the same register as P2, the implementation is able
  * to avoid a memcpy().
+ *
+ * Concatenation operator accepts only arguments of string-like
+ * types (i.e. TEXT and BLOB).
  */
 case OP_Concat: {           /* same as TK_CONCAT, in1, in2, out3 */
 	i64 nByte;
@@ -1527,9 +1556,30 @@ case OP_Concat: {           /* same as TK_CONCAT, in1, in2, out3 */
 		sqlVdbeMemSetNull(pOut);
 		break;
 	}
+	/*
+	 * Concatenation operation can be applied only to
+	 * strings and blobs.
+	 */
+	uint32_t str_type_p1 = pIn1->flags & (MEM_Blob | MEM_Str);
+	uint32_t str_type_p2 = pIn2->flags & (MEM_Blob | MEM_Str);
+	if (str_type_p1 == 0 || str_type_p2 == 0) {
+		char *inconsistent_type = str_type_p1 == 0 ?
+					  mem_type_to_str(pIn1) :
+					  mem_type_to_str(pIn2);
+		diag_set(ClientError, ER_INCONSISTENT_TYPES, "TEXT or BLOB",
+			 inconsistent_type);
+		rc = SQL_TARANTOOL_ERROR;
+		goto abort_due_to_error;
+	}
+
+	/* Moreover, both operands must be of the same type. */
+	if (str_type_p1 != str_type_p2) {
+		diag_set(ClientError, ER_INCONSISTENT_TYPES,
+			 mem_type_to_str(pIn2), mem_type_to_str(pIn1));
+		rc = SQL_TARANTOOL_ERROR;
+		goto abort_due_to_error;
+	}
 	if (ExpandBlob(pIn1) || ExpandBlob(pIn2)) goto no_mem;
-	Stringify(pIn1);
-	Stringify(pIn2);
 	nByte = pIn1->n + pIn2->n;
 	if (nByte>db->aLimit[SQL_LIMIT_LENGTH]) {
 		goto too_big;
diff --git a/test/box/misc.result b/test/box/misc.result
index 97189ecbb..345593655 100644
--- a/test/box/misc.result
+++ b/test/box/misc.result
@@ -502,6 +502,7 @@ t;
   172: box.error.ROWID_OVERFLOW
   173: box.error.DROP_COLLATION
   174: box.error.ILLEGAL_COLLATION_MIX
+  175: box.error.INCONSISTENT_TYPES
 ...
 test_run:cmd("setopt delimiter ''");
 ---
diff --git a/test/sql-tap/autoinc.test.lua b/test/sql-tap/autoinc.test.lua
index 1110a2472..5ec9fd632 100755
--- a/test/sql-tap/autoinc.test.lua
+++ b/test/sql-tap/autoinc.test.lua
@@ -686,11 +686,11 @@ test:do_test(
             DROP TRIGGER t3928r2;
             CREATE TRIGGER t3928r3 BEFORE UPDATE ON t3928
               WHEN new.b=='456' BEGIN
-                 INSERT INTO t3928(b) VALUES('before-int-' || new.b);
+                 INSERT INTO t3928(b) VALUES('before-int-' || CAST(new.b AS TEXT));
             END;
             CREATE TRIGGER t3928r4 AFTER UPDATE ON t3928
               WHEN new.b=='456' BEGIN
-                 INSERT INTO t3928(b) VALUES('after-int-' || new.b);
+                 INSERT INTO t3928(b) VALUES('after-int-' || CAST(new.b AS TEXT));
             END;
             DELETE FROM t3928 WHERE a!=1;
             UPDATE t3928 SET b=456 WHERE a=1;
@@ -725,12 +725,12 @@ test:do_test(
             DELETE FROM t3928;
             CREATE TABLE t3928c(y INTEGER PRIMARY KEY AUTOINCREMENT, z TEXT);
             CREATE TRIGGER t3928br1 BEFORE DELETE ON t3928b BEGIN
-              INSERT INTO t3928(b) VALUES('before-del-'||old.x);
-              INSERT INTO t3928c(z) VALUES('before-del-'||old.x);
+              INSERT INTO t3928(b) VALUES('before-del-'|| CAST(old.x AS TEXT));
+              INSERT INTO t3928c(z) VALUES('before-del-'|| CAST(old.x AS TEXT));
             END;
             CREATE TRIGGER t3928br2 AFTER DELETE ON t3928b BEGIN
-              INSERT INTO t3928(b) VALUES('after-del-'||old.x);
-              INSERT INTO t3928c(z) VALUES('after-del-'||old.x);
+              INSERT INTO t3928(b) VALUES('after-del-'|| CAST(old.x AS TEXT));
+              INSERT INTO t3928c(z) VALUES('after-del-'|| CAST(old.x AS TEXT));
             END;
             DELETE FROM t3928b;
             SELECT * FROM t3928 ORDER BY a;
diff --git a/test/sql-tap/e_select1.test.lua b/test/sql-tap/e_select1.test.lua
index 271fa1547..cdd088209 100755
--- a/test/sql-tap/e_select1.test.lua
+++ b/test/sql-tap/e_select1.test.lua
@@ -875,7 +875,7 @@ test:do_select_tests(
                 0, -22.18, -2.2, -23.18, "suiters", 1, 68, "", 67, "quartets", 0, -31.3,
                 -1.04, -32.3, "aspen", 0, 1, 63, 0, "-26"}},
 
-        {"3", "SELECT 32*32, d||e FROM z2", {1024, "", 1024, "36.06.0"}},
+        {"3", "SELECT 32*32, CAST(d AS TEXT) || CAST(e AS TEXT) FROM z2", {1024, "", 1024, "36.06.0"}},
     })
 
 -- Test cases e_select-4.5.* and e_select-4.6.* together show that:
@@ -1136,7 +1136,7 @@ test:do_select_tests(
         {"1.1", "SELECT up FROM c1 GROUP BY up HAVING count(*)>3", {"x"}},
         {"1.2", "SELECT up FROM c1 GROUP BY up HAVING sum(down)>16", {"y"}},
         {"1.3", "SELECT up FROM c1 GROUP BY up HAVING sum(down)<16", {"x"}},
-        {"1.4", "SELECT up||down FROM c1 GROUP BY (down<5) HAVING max(down)<10", {"x4"}},
+        {"1.4", "SELECT up|| CAST(down AS TEXT) FROM c1 GROUP BY (down<5) HAVING max(down)<10", {"x4"}},
 
         {"2.1", "SELECT up FROM c1 GROUP BY up HAVING down>10", {"y"}},
         {"2.2", "SELECT up FROM c1 GROUP BY up HAVING up='y'", {"y"}},
diff --git a/test/sql-tap/func.test.lua b/test/sql-tap/func.test.lua
index e6b7956aa..495be7d29 100755
--- a/test/sql-tap/func.test.lua
+++ b/test/sql-tap/func.test.lua
@@ -475,7 +475,7 @@ test:do_catchsql_test(
 test:do_catchsql_test(
     "func-4.10",
     [[
-        SELECT 'x' || round(c,a) || 'y' FROM t1 ORDER BY a
+        SELECT 'x' || CAST(round(c,a) AS TEXT) || 'y' FROM t1 ORDER BY a
     ]], {
         -- <func-4.10>
         0, {"x3.0y", "x-12345.68y", "x-5.0y"}
@@ -863,7 +863,7 @@ test:do_test(
 test:do_execsql_test(
     "func-8.2",
     [[
-        SELECT max('z+'||a||'abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOP') FROM t2;
+        SELECT max('z+'|| CAST(a AS TEXT) ||'abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOP') FROM t2;
     ]], {
         -- <func-8.2>
         "z+67890abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOP"
diff --git a/test/sql-tap/sort.test.lua b/test/sql-tap/sort.test.lua
index df666070f..a84c549cc 100755
--- a/test/sql-tap/sort.test.lua
+++ b/test/sql-tap/sort.test.lua
@@ -238,7 +238,7 @@ test:do_execsql_test(
 test:do_execsql_test(
     "sort-2.1.1",
     [[
-        UPDATE t1 SET v='x' || -flt;
+        UPDATE t1 SET v='x' || CAST(-flt AS TEXT);
         UPDATE t1 SET v='x-2b' where v=='x-0.123';
         SELECT v FROM t1 ORDER BY v;
     ]], {
@@ -338,7 +338,7 @@ test:do_execsql_test(
 test:do_execsql_test(
     "sort-4.2",
     [[
-        SELECT n||'' FROM t1 ORDER BY 1;
+        SELECT CAST(n AS TEXT) || '' FROM t1 ORDER BY 1;
     ]], {
         -- <sort-4.2>
         "1", "10", "11", "12", "2", "3", "4", "5", "6", "7", "8", "9"
@@ -358,7 +358,7 @@ test:do_execsql_test(
 test:do_execsql_test(
     "sort-4.4",
     [[
-        SELECT n||'' FROM t1 ORDER BY 1 DESC;
+        SELECT CAST(n AS TEXT) || '' FROM t1 ORDER BY 1 DESC;
     ]], {
         -- <sort-4.4>
         "9", "8", "7", "6", "5", "4", "3", "2", "12", "11", "10", "1"
diff --git a/test/sql-tap/tkt2192.test.lua b/test/sql-tap/tkt2192.test.lua
index eb53863d4..b118d6310 100755
--- a/test/sql-tap/tkt2192.test.lua
+++ b/test/sql-tap/tkt2192.test.lua
@@ -146,7 +146,7 @@ test:do_execsql_test(
 test:do_execsql_test(
     "tkt2192-2.3",
     [[
-        SELECT x.a || '/' || x.b || '/' || y.b
+        SELECT CAST(x.a AS TEXT) || '/' || CAST(x.b AS TEXT) || '/' || CAST(y.b AS TEXT)
           FROM v1 AS x JOIN v1 AS y ON x.a=y.a AND x.b<y.b
          ORDER BY x.a, x.b, y.b
     ]], {
@@ -159,7 +159,7 @@ test:do_execsql_test(
     "tkt2192-2.4",
     [[
         CREATE VIEW v2 AS
-        SELECT x.a || '/' || x.b || '/' || y.b AS z
+        SELECT CAST(x.a AS TEXT) || '/' || CAST(x.b AS TEXT) || '/' || CAST(y.b AS TEXT) AS z
           FROM v1 AS x JOIN v1 AS y ON x.a=y.a AND x.b<y.b
          ORDER BY x.a, x.b, y.b;
         SELECT * FROM v2;
diff --git a/test/sql-tap/trigger5.test.lua b/test/sql-tap/trigger5.test.lua
index 8d6fd8339..f71649925 100755
--- a/test/sql-tap/trigger5.test.lua
+++ b/test/sql-tap/trigger5.test.lua
@@ -30,8 +30,8 @@ test:do_execsql_test(
         BEGIN
           INSERT INTO Undo VALUES
              ((SELECT coalesce(max(id),0) + 1 FROM Undo),
-              (SELECT 'INSERT INTO Item (a,b,c) VALUES (' || coalesce(old.a,'NULL')
-                  || ',' || quote(old.b) || ',' || old.c || ');'));
+              (SELECT 'INSERT INTO Item (a,b,c) VALUES (' || CAST(coalesce(old.a,'NULL') AS TEXT)
+                  || ',' || quote(old.b) || ',' || CAST(old.c AS TEXT) || ');'));
         END;
         DELETE FROM Item WHERE a = 1;
         SELECT * FROM Undo;
diff --git a/test/sql/types.result b/test/sql/types.result
index 9043cbfd5..0cbbd1642 100644
--- a/test/sql/types.result
+++ b/test/sql/types.result
@@ -131,3 +131,35 @@ box.sql.execute("SELECT * FROM test")
 sp:drop()
 ---
 ...
+-- gh-3544: concatenation operator accepts only TEXT and BLOB.
+--
+box.sql.execute("SELECT 'abc' || 1;")
+---
+- error: 'Inconsistent types: expected TEXT or BLOB got INTEGER'
+...
+box.sql.execute("SELECT 'abc' || 1.123;")
+---
+- error: 'Inconsistent types: expected TEXT or BLOB got REAL'
+...
+box.sql.execute("SELECT 1 || 'abc';")
+---
+- error: 'Inconsistent types: expected TEXT or BLOB got INTEGER'
+...
+box.sql.execute("SELECT 1.123 || 'abc';")
+---
+- error: 'Inconsistent types: expected TEXT or BLOB got REAL'
+...
+box.sql.execute("SELECt 'a' || 'b' || 1;")
+---
+- error: 'Inconsistent types: expected TEXT or BLOB got INTEGER'
+...
+-- What is more, they must be of the same type.
+--
+box.sql.execute("SELECT 'abc' || CAST('x' AS BLOB);")
+---
+- error: 'Inconsistent types: expected TEXT got BLOB'
+...
+box.sql.execute("SELECT CAST('abc' AS BLOB) || 'x';")
+---
+- error: 'Inconsistent types: expected BLOB got TEXT'
+...
diff --git a/test/sql/types.test.lua b/test/sql/types.test.lua
index 8eda36159..e0331bb8f 100644
--- a/test/sql/types.test.lua
+++ b/test/sql/types.test.lua
@@ -40,3 +40,15 @@ sp:insert({1, true})
 sp:insert({2, false})
 box.sql.execute("SELECT * FROM test")
 sp:drop()
+
+-- gh-3544: concatenation operator accepts only TEXT and BLOB.
+--
+box.sql.execute("SELECT 'abc' || 1;")
+box.sql.execute("SELECT 'abc' || 1.123;")
+box.sql.execute("SELECT 1 || 'abc';")
+box.sql.execute("SELECT 1.123 || 'abc';")
+box.sql.execute("SELECt 'a' || 'b' || 1;")
+-- What is more, they must be of the same type.
+--
+box.sql.execute("SELECT 'abc' || CAST('x' AS BLOB);")
+box.sql.execute("SELECT CAST('abc' AS BLOB) || 'x';")
-- 
2.15.1