[tarantool-patches] Re: [PATCH v1 2/2] sql: check returned value type for UDF
Kirill Shcherbatov
kshcherbatov at tarantool.org
Tue Oct 8 11:38:42 MSK 2019
> All user-defined functions feature type of returned value (if it is not
> specified during function creation, it is assumed to be ANY), but currently
> it is ignored. This patch introduces check which verifies that returned
> value is of specified in function's definition type. There's no attempt
> at implicit conversion to specified (target) type - returned value must
> be literally of the specified type.
Tnx. Applied.
>> + /*204 */_(ER_FUNC_INVALID_RETURN, "Function '%s' returned invalid value: expected %s got %s") \
>
> I'd say: Function %s returned value of type %s but expected %s OR
> Function %s returned value of wrong/invalid type: expected %s got %s
Ok, done.
> -> so pointer to the function may turn out to be invalid
> after call.
>
> Did you add test case covering this situation? If not, please
> introduce it.
Yes, there is such test already. It is func-recreate.test.lua.
> What about returning NULL value and cdata?
Good idea.
I've extended test coverage with nil, NULL and all important ffi types.
Now it works perfectly. Tnx.
=====================================================================
All user-defined functions feature type of returned value (if it
is not specified during function creation, it is assumed to be
ANY), but currently it is ignored. This patch introduces check
which verifies that returned value is of specified in function's
definition type. There's no attempt at implicit conversion to
specified (target) type - returned value must be literally of
the specified type.
Closes #4387
---
src/box/errcode.h | 1 +
src/box/field_def.h | 17 ++++-
src/box/sql/vdbe.c | 11 +++
test/sql-tap/func.test.lua | 123 +++++++++++++++++++++++++++++++-
test/sql/func-recreate.result | 8 +--
test/sql/func-recreate.test.lua | 6 +-
6 files changed, 156 insertions(+), 10 deletions(-)
diff --git a/src/box/errcode.h b/src/box/errcode.h
index d5d396d87..0dd94e47f 100644
--- a/src/box/errcode.h
+++ b/src/box/errcode.h
@@ -256,6 +256,7 @@ struct errcode_record {
/*201 */_(ER_NO_SUCH_FIELD_NAME, "Field '%s' was not found in the tuple") \
/*202 */_(ER_FUNC_WRONG_ARG_COUNT, "Wrong number of arguments is passed to %s(): expected %s, got %d") \
/*203 */_(ER_BOOTSTRAP_READONLY, "Trying to bootstrap a local read-only instance as master") \
+ /*204 */_(ER_FUNC_INVALID_RETURN, "Function '%s' returned value of invalid type: expected %s got %s") \
/*
* !IMPORTANT! Please follow instructions at start of the file
diff --git a/src/box/field_def.h b/src/box/field_def.h
index fdedc9622..49c2998f5 100644
--- a/src/box/field_def.h
+++ b/src/box/field_def.h
@@ -144,6 +144,19 @@ struct field_def {
struct Expr *default_value_expr;
};
+/**
+ * Checks if mp_type (except MP_EXT) (MsgPack) is compatible
+ * with given field type.
+ */
+static inline bool
+field_mp_plain_type_is_compatible(enum field_type type, enum mp_type mp_type,
+ bool is_nullable)
+{
+ assert(mp_type != MP_EXT);
+ uint32_t mask = field_mp_type[type] | (is_nullable * (1U << MP_NIL));
+ return (mask & (1U << mp_type)) != 0;
+}
+
/** Checks if mp_type (MsgPack) is compatible with field type. */
static inline bool
field_mp_type_is_compatible(enum field_type type, const char *data,
@@ -154,8 +167,8 @@ field_mp_type_is_compatible(enum field_type type, const char *data,
assert((size_t)mp_type < CHAR_BIT * sizeof(*field_mp_type));
uint32_t mask;
if (mp_type != MP_EXT) {
- mask = field_mp_type[type] | (is_nullable * (1U << MP_NIL));
- return (mask & (1U << mp_type)) != 0;
+ return field_mp_plain_type_is_compatible(type, mp_type,
+ is_nullable);
} else {
int8_t ext_type;
mp_decode_extl(&data, &ext_type);
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 9495ada37..32d5b10a2 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -1843,6 +1843,11 @@ case OP_FunctionByName: {
diag_set(ClientError, ER_NO_SUCH_FUNCTION, pOp->p4.z);
goto abort_due_to_error;
}
+ /*
+ * Function call may yield so pointer to the function may
+ * turn out to be invalid after call.
+ */
+ enum field_type returns = func->def->returns;
int argc = pOp->p5;
struct Mem *argv = &aMem[pOp->p2];
struct port args, ret;
@@ -1862,6 +1867,12 @@ case OP_FunctionByName: {
region_truncate(region, region_svp);
if (mem == NULL)
goto abort_due_to_error;
+ enum mp_type type = sql_value_type((sql_value *)pOut);
+ if (!field_mp_plain_type_is_compatible(returns, type, false)) {
+ diag_set(ClientError, ER_FUNC_INVALID_RETURN, pOp->p4.z,
+ field_type_strs[returns], mp_type_strs[type]);
+ goto abort_due_to_error;
+ }
/*
* Copy the result of the function invocation into
diff --git a/test/sql-tap/func.test.lua b/test/sql-tap/func.test.lua
index 39d9ccf2d..e65fba150 100755
--- a/test/sql-tap/func.test.lua
+++ b/test/sql-tap/func.test.lua
@@ -1,6 +1,6 @@
#!/usr/bin/env tarantool
test = require("sqltester")
-test:plan(14610)
+test:plan(14694)
--!./tcltestrunner.lua
-- 2001 September 15
@@ -2952,4 +2952,125 @@ test:do_catchsql_test(
-- </func-76.4>
})
+ffi = require('ffi')
+box.schema.func.create('TOSTRING', {language = 'Lua', is_deterministic = true,
+ body = 'function(a) return a end',
+ param_list = {'any'}, returns = 'string',
+ exports = {'LUA', 'SQL'}})
+test:do_execsql_test("func-77.1", "SELECT TOSTRING('1');", {'1'})
+test:do_execsql_test("func-77.2", "SELECT TOSTRING('a');", {'a'})
+test:do_catchsql_test("func-77.3", "SELECT TOSTRING(1);", {1, "Function 'TOSTRING' returned value of invalid type: expected string got unsigned"})
+test:do_catchsql_test("func-77.4", "SELECT TOSTRING(-1);", {1, "Function 'TOSTRING' returned value of invalid type: expected string got integer"})
+test:do_catchsql_test("func-77.5", "SELECT TOSTRING(-1.1);", {1, "Function 'TOSTRING' returned value of invalid type: expected string got double"})
+test:do_catchsql_test("func-77.6", "SELECT TOSTRING(TRUE);", {1, "Function 'TOSTRING' returned value of invalid type: expected string got boolean"})
+test:do_catchsql_test("func-77.7", "SELECT TOSTRING(NULL);", {1, "Function 'TOSTRING' returned value of invalid type: expected string got nil"})
+test:do_catchsql_test("func-77.8", "SELECT TOSTRING(LUA('return nil'));", {1, "Function 'TOSTRING' returned value of invalid type: expected string got nil"})
+test:do_catchsql_test("func-77.9", "SELECT TOSTRING(LUA('return box.NULL'));", {1, "Function 'TOSTRING' returned value of invalid type: expected string got nil"})
+test:do_catchsql_test("func-77.10", "SELECT TOSTRING(LUA('return ffi.new(\"unsigned\", 666)'));", {1, "Function 'TOSTRING' returned value of invalid type: expected string got unsigned"})
+test:do_catchsql_test("func-77.11", "SELECT TOSTRING(LUA('return ffi.new(\"int\", -666)'));", {1, "Function 'TOSTRING' returned value of invalid type: expected string got integer"})
+test:do_catchsql_test("func-77.12", "SELECT TOSTRING(LUA('return ffi.new(\"double\", -666.666)'));", {1, "Function 'TOSTRING' returned value of invalid type: expected string got double"})
+test:do_catchsql_test("func-77.14", "SELECT TOSTRING(LUA('return ffi.new(\"bool\", true)'));", {1, "Function 'TOSTRING' returned value of invalid type: expected string got boolean"})
+test:do_execsql_test("func-77.13", "SELECT TOSTRING(LUA('return ffi.string(\"hello\", 5)'));", {'hello'})
+box.func.TOSTRING:drop()
+
+box.schema.func.create('TOUNSIGNED', {language = 'Lua', is_deterministic = true,
+ body = 'function(a) return a end',
+ param_list = {'any'}, returns = 'unsigned',
+ exports = {'LUA', 'SQL'}})
+test:do_catchsql_test("func-78.1", "SELECT TOUNSIGNED('1');", {1, "Function 'TOUNSIGNED' returned value of invalid type: expected unsigned got string"})
+test:do_catchsql_test("func-78.2", "SELECT TOUNSIGNED('a');", {1, "Function 'TOUNSIGNED' returned value of invalid type: expected unsigned got string"})
+test:do_execsql_test("func-78.3", "SELECT TOUNSIGNED(1);", {1})
+test:do_catchsql_test("func-78.4", "SELECT TOUNSIGNED(-1);", {1, "Function 'TOUNSIGNED' returned value of invalid type: expected unsigned got integer"})
+test:do_catchsql_test("func-78.5", "SELECT TOUNSIGNED(-1.1);", {1, "Function 'TOUNSIGNED' returned value of invalid type: expected unsigned got double"})
+test:do_catchsql_test("func-78.6", "SELECT TOUNSIGNED(TRUE);", {1, "Function 'TOUNSIGNED' returned value of invalid type: expected unsigned got boolean"})
+test:do_catchsql_test("func-78.7", "SELECT TOUNSIGNED(NULL);", {1, "Function 'TOUNSIGNED' returned value of invalid type: expected unsigned got nil"})
+test:do_catchsql_test("func-78.8", "SELECT TOUNSIGNED(LUA('return nil'));", {1, "Function 'TOUNSIGNED' returned value of invalid type: expected unsigned got nil"})
+test:do_catchsql_test("func-78.9", "SELECT TOUNSIGNED(LUA('return box.NULL'));", {1, "Function 'TOUNSIGNED' returned value of invalid type: expected unsigned got nil"})
+test:do_execsql_test("func-78.10", "SELECT TOUNSIGNED(LUA('return ffi.new(\"unsigned\", 666)'));", {666})
+test:do_catchsql_test("func-78.11", "SELECT TOUNSIGNED(LUA('return ffi.new(\"int\", -666)'));", {1, "Function 'TOUNSIGNED' returned value of invalid type: expected unsigned got integer"})
+test:do_catchsql_test("func-78.12", "SELECT TOUNSIGNED(LUA('return ffi.new(\"double\", -666.666)'));", {1, "Function 'TOUNSIGNED' returned value of invalid type: expected unsigned got double"})
+test:do_catchsql_test("func-78.14", "SELECT TOUNSIGNED(LUA('return ffi.new(\"bool\", true)'));", {1, "Function 'TOUNSIGNED' returned value of invalid type: expected unsigned got boolean"})
+test:do_catchsql_test("func-78.13", "SELECT TOUNSIGNED(LUA('return ffi.string(\"hello\", 5)'));", {1, "Function 'TOUNSIGNED' returned value of invalid type: expected unsigned got string"})
+box.func.TOUNSIGNED:drop()
+
+box.schema.func.create('TOINTEGER', {language = 'Lua', is_deterministic = true,
+ body = 'function(a) return a end',
+ param_list = {'any'}, returns = 'integer',
+ exports = {'LUA', 'SQL'}})
+test:do_catchsql_test("func-79.1", "SELECT TOINTEGER('1');", {1, "Function 'TOINTEGER' returned value of invalid type: expected integer got string"})
+test:do_catchsql_test("func-79.2", "SELECT TOINTEGER('a');", {1, "Function 'TOINTEGER' returned value of invalid type: expected integer got string"})
+test:do_execsql_test("func-79.3", "SELECT TOINTEGER(1);", {1})
+test:do_execsql_test("func-79.4", "SELECT TOINTEGER(-1);", {-1})
+test:do_catchsql_test("func-79.5", "SELECT TOINTEGER(-1.1);", {1, "Function 'TOINTEGER' returned value of invalid type: expected integer got double"})
+test:do_catchsql_test("func-79.6", "SELECT TOINTEGER(TRUE);", {1, "Function 'TOINTEGER' returned value of invalid type: expected integer got boolean"})
+test:do_catchsql_test("func-79.7", "SELECT TOINTEGER(NULL);", {1, "Function 'TOINTEGER' returned value of invalid type: expected integer got nil"})
+test:do_catchsql_test("func-79.8", "SELECT TOINTEGER(LUA('return nil'));", {1, "Function 'TOINTEGER' returned value of invalid type: expected integer got nil"})
+test:do_catchsql_test("func-79.9", "SELECT TOINTEGER(LUA('return box.NULL'));", {1, "Function 'TOINTEGER' returned value of invalid type: expected integer got nil"})
+test:do_execsql_test("func-79.10", "SELECT TOINTEGER(LUA('return ffi.new(\"unsigned\", 666)'));", {666})
+test:do_execsql_test("func-79.11", "SELECT TOINTEGER(LUA('return ffi.new(\"int\", -666)'));", {-666})
+test:do_catchsql_test("func-79.12", "SELECT TOINTEGER(LUA('return ffi.new(\"double\", -666.666)'));", {1, "Function 'TOINTEGER' returned value of invalid type: expected integer got double"})
+test:do_catchsql_test("func-79.14", "SELECT TOINTEGER(LUA('return ffi.new(\"bool\", true)'));", {1, "Function 'TOINTEGER' returned value of invalid type: expected integer got boolean"})
+test:do_catchsql_test("func-79.13", "SELECT TOINTEGER(LUA('return ffi.string(\"hello\", 5)'));", {1, "Function 'TOINTEGER' returned value of invalid type: expected integer got string"})
+box.func.TOINTEGER:drop()
+
+box.schema.func.create('TONUMBER', {language = 'Lua', is_deterministic = true,
+ body = 'function(a) return a end',
+ param_list = {'any'}, returns = 'number',
+ exports = {'LUA', 'SQL'}})
+test:do_catchsql_test("func-80.1", "SELECT TONUMBER('1');", {1, "Function 'TONUMBER' returned value of invalid type: expected number got string"})
+test:do_catchsql_test("func-80.2", "SELECT TONUMBER('a');", {1, "Function 'TONUMBER' returned value of invalid type: expected number got string"})
+test:do_execsql_test("func-80.3", "SELECT TONUMBER(1);", {1})
+test:do_execsql_test("func-80.4", "SELECT TONUMBER(-1);", {-1})
+test:do_execsql_test("func-80.5", "SELECT TONUMBER(-1.1);", {-1.1})
+test:do_catchsql_test("func-80.6", "SELECT TONUMBER(TRUE);", {1, "Function 'TONUMBER' returned value of invalid type: expected number got boolean"})
+test:do_catchsql_test("func-80.7", "SELECT TONUMBER(NULL);", {1, "Function 'TONUMBER' returned value of invalid type: expected number got nil"})
+test:do_catchsql_test("func-80.8", "SELECT TONUMBER(LUA('return nil'));", {1, "Function 'TONUMBER' returned value of invalid type: expected number got nil"})
+test:do_catchsql_test("func-80.9", "SELECT TONUMBER(LUA('return box.NULL'));", {1, "Function 'TONUMBER' returned value of invalid type: expected number got nil"})
+test:do_execsql_test("func-80.10", "SELECT TONUMBER(LUA('return ffi.new(\"unsigned\", 666)'));", {666})
+test:do_execsql_test("func-80.11", "SELECT TONUMBER(LUA('return ffi.new(\"int\", -666)'));", {-666})
+test:do_execsql_test("func-80.12", "SELECT TONUMBER(LUA('return ffi.new(\"double\", -666.666)'));", {-666.666})
+test:do_catchsql_test("func-80.14", "SELECT TONUMBER(LUA('return ffi.new(\"bool\", true)'));", {1, "Function 'TONUMBER' returned value of invalid type: expected number got boolean"})
+test:do_catchsql_test("func-80.13", "SELECT TONUMBER(LUA('return ffi.string(\"hello\", 5)'));", {1, "Function 'TONUMBER' returned value of invalid type: expected number got string"})
+box.func.TONUMBER:drop()
+
+box.schema.func.create('TOBOOLEAN', {language = 'Lua', is_deterministic = true,
+ body = 'function(a) return a end',
+ param_list = {'any'}, returns = 'boolean',
+ exports = {'LUA', 'SQL'}})
+test:do_catchsql_test("func-81.1", "SELECT TOBOOLEAN('1');", {1, "Function 'TOBOOLEAN' returned value of invalid type: expected boolean got string"})
+test:do_catchsql_test("func-81.2", "SELECT TOBOOLEAN('a');", {1, "Function 'TOBOOLEAN' returned value of invalid type: expected boolean got string"})
+test:do_catchsql_test("func-81.3", "SELECT TOBOOLEAN(1);", {1, "Function 'TOBOOLEAN' returned value of invalid type: expected boolean got unsigned"})
+test:do_catchsql_test("func-81.4", "SELECT TOBOOLEAN(-1);", {1, "Function 'TOBOOLEAN' returned value of invalid type: expected boolean got integer"})
+test:do_catchsql_test("func-81.5", "SELECT TOBOOLEAN(-1.1);", {1, "Function 'TOBOOLEAN' returned value of invalid type: expected boolean got double"})
+test:do_execsql_test("func-81.6", "SELECT TOBOOLEAN(TRUE);", {true})
+test:do_catchsql_test("func-81.7", "SELECT TOBOOLEAN(NULL);", {1, "Function 'TOBOOLEAN' returned value of invalid type: expected boolean got nil"})
+test:do_catchsql_test("func-81.8", "SELECT TOBOOLEAN(LUA('return nil'));", {1, "Function 'TOBOOLEAN' returned value of invalid type: expected boolean got nil"})
+test:do_catchsql_test("func-81.9", "SELECT TOBOOLEAN(LUA('return box.NULL'));", {1, "Function 'TOBOOLEAN' returned value of invalid type: expected boolean got nil"})
+test:do_catchsql_test("func-81.10", "SELECT TOBOOLEAN(LUA('return ffi.new(\"unsigned\", 666)'));", {1, "Function 'TOBOOLEAN' returned value of invalid type: expected boolean got unsigned"})
+test:do_catchsql_test("func-81.11", "SELECT TOBOOLEAN(LUA('return ffi.new(\"int\", -666)'));", {1, "Function 'TOBOOLEAN' returned value of invalid type: expected boolean got integer"})
+test:do_catchsql_test("func-81.12", "SELECT TOBOOLEAN(LUA('return ffi.new(\"double\", -666.666)'));", {1, "Function 'TOBOOLEAN' returned value of invalid type: expected boolean got double"})
+test:do_execsql_test("func-81.14", "SELECT TOBOOLEAN(LUA('return ffi.new(\"bool\", true)'));", {true})
+test:do_catchsql_test("func-81.13", "SELECT TOBOOLEAN(LUA('return ffi.string(\"hello\", 5)'));", {1, "Function 'TOBOOLEAN' returned value of invalid type: expected boolean got string"})
+box.func.TOBOOLEAN:drop()
+
+box.schema.func.create('TOSCALAR', {language = 'Lua', is_deterministic = true,
+ body = 'function(a) return a end',
+ param_list = {'any'}, returns = 'scalar',
+ exports = {'LUA', 'SQL'}})
+test:do_execsql_test("func-82.1", "SELECT TOSCALAR('1');", {'1'})
+test:do_execsql_test("func-82.2", "SELECT TOSCALAR('a');", {'a'})
+test:do_execsql_test("func-82.3", "SELECT TOSCALAR(1);", {1})
+test:do_execsql_test("func-82.4", "SELECT TOSCALAR(-1);", {-1})
+test:do_execsql_test("func-82.5", "SELECT TOSCALAR(-1.1);", {-1.1})
+test:do_execsql_test("func-82.6", "SELECT TOSCALAR(TRUE);", {true})
+test:do_catchsql_test("func-82.7", "SELECT TOSCALAR(NULL);", {1, "Function 'TOSCALAR' returned value of invalid type: expected scalar got nil"})
+test:do_catchsql_test("func-82.8", "SELECT TOSCALAR(LUA('return nil'));", {1, "Function 'TOSCALAR' returned value of invalid type: expected scalar got nil"})
+test:do_catchsql_test("func-82.9", "SELECT TOSCALAR(LUA('return box.NULL'));", {1, "Function 'TOSCALAR' returned value of invalid type: expected scalar got nil"})
+test:do_execsql_test("func-82.10", "SELECT TOSCALAR(LUA('return ffi.new(\"unsigned\", 666)'));", {666})
+test:do_execsql_test("func-82.11", "SELECT TOSCALAR(LUA('return ffi.new(\"int\", -666)'));", {-666})
+test:do_execsql_test("func-82.12", "SELECT TOSCALAR(LUA('return ffi.new(\"double\", -666.666)'));", {-666.666})
+test:do_execsql_test("func-82.14", "SELECT TOSCALAR(LUA('return ffi.new(\"bool\", true)'));", {true})
+test:do_execsql_test("func-82.13", "SELECT TOSCALAR(LUA('return ffi.string(\"hello\", 5)'));", {"hello"})
+box.func.TOSCALAR:drop()
+
test:finish_test()
diff --git a/test/sql/func-recreate.result b/test/sql/func-recreate.result
index 1f6966431..a0a67a106 100644
--- a/test/sql/func-recreate.result
+++ b/test/sql/func-recreate.result
@@ -18,7 +18,7 @@ test_run:cmd("setopt delimiter ';'")
...
box.schema.func.create('WAITFOR', {language = 'Lua',
body = 'function (n) fiber.sleep(n) return n end',
- param_list = {'integer'}, returns = 'integer',
+ param_list = {'number'}, returns = 'number',
exports = {'LUA', 'SQL'}})
test_run:cmd("setopt delimiter ''");
---
@@ -41,7 +41,7 @@ test_run:cmd("setopt delimiter ';'")
...
box.schema.func.create('WAITFOR', {language = 'Lua',
body = 'function (n) fiber.sleep(n) return n end',
- param_list = {'integer'}, returns = 'integer',
+ param_list = {'number'}, returns = 'number',
exports = {'LUA', 'SQL'}})
test_run:cmd("setopt delimiter ''");
---
@@ -50,7 +50,7 @@ ch:get()
---
- metadata:
- name: WAITFOR(0.2)
- type: integer
+ type: number
rows:
- [0.2]
...
@@ -63,7 +63,7 @@ test_run:cmd("setopt delimiter ';'")
...
box.schema.func.create('WAITFOR', {language = 'Lua',
body = 'function (n) fiber.sleep(n) return n end',
- param_list = {'integer'}, returns = 'integer',
+ param_list = {'number'}, returns = 'number',
exports = {'LUA', 'SQL'}})
test_run:cmd("setopt delimiter ''");
---
diff --git a/test/sql/func-recreate.test.lua b/test/sql/func-recreate.test.lua
index 5496baf6e..0b32ea9ae 100644
--- a/test/sql/func-recreate.test.lua
+++ b/test/sql/func-recreate.test.lua
@@ -7,7 +7,7 @@ fiber = require('fiber')
test_run:cmd("setopt delimiter ';'")
box.schema.func.create('WAITFOR', {language = 'Lua',
body = 'function (n) fiber.sleep(n) return n end',
- param_list = {'integer'}, returns = 'integer',
+ param_list = {'number'}, returns = 'number',
exports = {'LUA', 'SQL'}})
test_run:cmd("setopt delimiter ''");
@@ -21,7 +21,7 @@ box.func.WAITFOR:drop()
test_run:cmd("setopt delimiter ';'")
box.schema.func.create('WAITFOR', {language = 'Lua',
body = 'function (n) fiber.sleep(n) return n end',
- param_list = {'integer'}, returns = 'integer',
+ param_list = {'number'}, returns = 'number',
exports = {'LUA', 'SQL'}})
test_run:cmd("setopt delimiter ''");
ch:get()
@@ -30,7 +30,7 @@ box.func.WAITFOR:drop()
test_run:cmd("setopt delimiter ';'")
box.schema.func.create('WAITFOR', {language = 'Lua',
body = 'function (n) fiber.sleep(n) return n end',
- param_list = {'integer'}, returns = 'integer',
+ param_list = {'number'}, returns = 'number',
exports = {'LUA', 'SQL'}})
test_run:cmd("setopt delimiter ''");
--
2.23.0
More information about the Tarantool-patches
mailing list