From d7da59b4a33c9547da68431a22ee256e50a3df62 Mon Sep 17 00:00:00 2001
From: Nikita Zheleztsov <n.zheleztsov@proton.me>
Date: Wed, 20 Jul 2022 12:00:04 +0300
Subject: [PATCH] net.box: add checking of options
Currently net.box's methods doesn't check types of passed options.
This can lead to Lua's internal errors, which are not self-explaining.
Let's add this functionality and raise errors with meaningful messages
in case of incorrect options.
Closes #6063
Closes #6530
NO_DOC=Not a visible change
---
.../unreleased/gh-6530-netbox-opts-check.md | 5 +
src/box/lua/net_box.lua | 95 +++++++--
src/box/lua/schema.lua | 19 +-
.../gh_6530_net_box_opts_check_test.lua | 184 ++++++++++++++++++
test/box-luatest/net_box_test.lua | 2 +-
5 files changed, 288 insertions(+), 17 deletions(-)
create mode 100644 changelogs/unreleased/gh-6530-netbox-opts-check.md
create mode 100644 test/box-luatest/gh_6530_net_box_opts_check_test.lua
diff --git a/changelogs/unreleased/gh-6530-netbox-opts-check.md b/changelogs/unreleased/gh-6530-netbox-opts-check.md
new file mode 100644
index 0000000000..72d46a09e7
--- /dev/null
+++ b/changelogs/unreleased/gh-6530-netbox-opts-check.md
@@ -0,0 +1,5 @@
+## bugfix/core
+
+* Added type checking for options in net.box's remote queries and
+ connect method. Now graceful errors are thrown in case of incorrect
+ options (gh-6063, gh-6530).
diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
index 5092956df6..570108fa53 100644
--- a/src/box/lua/net_box.lua
+++ b/src/box/lua/net_box.lua
@@ -16,6 +16,9 @@ local check_select_opts = box.internal.check_select_opts
local check_index_arg = box.internal.check_index_arg
local check_space_arg = box.internal.check_space_arg
local check_primary_index = box.internal.check_primary_index
+local check_param_table = box.internal.check_param_table
+
+local ibuf_t = ffi.typeof('struct ibuf')
local TIMEOUT_INFINITY = 500 * 365 * 86400
@@ -55,6 +58,38 @@ local IPROTO_FEATURE_NAMES = {
[3] = 'watchers',
}
+local REQUEST_OPTION_TYPES = {
+ is_async = "boolean",
+ iterator = "string",
+ limit = "number",
+ offset = "number",
+ on_push = "function",
+ on_push_ctx = "any",
+ return_raw = "boolean",
+ skip_header = "boolean",
+ timeout = "number",
+ buffer = function(buf)
+ if not ffi.istype(ibuf_t, buf) then
+ return false, "struct ibuf"
+ end
+ return true
+ end,
+}
+
+local CONNECT_OPTION_TYPES = {
+ user = "string",
+ password = "string",
+ wait_connected = "number, boolean",
+ reconnect_after = "number",
+ call_16 = "boolean",
+ console = "boolean",
+ connect_timeout = "number",
+ fetch_schema = "boolean",
+ required_protocol_version = "number",
+ required_protocol_features = "table",
+ _disable_graceful_shutdown = "boolean",
+}
+
-- Given an array of IPROTO feature ids, returns a map {feature_name: bool}.
local function iproto_features_resolve(feature_ids)
local features = {}
@@ -436,7 +471,9 @@ end
-- @retval Net.box object.
--
local function connect(...)
- return new_sm(parse_connect_params(...))
+ local uri, opts = parse_connect_params(...)
+ check_param_table(opts, CONNECT_OPTION_TYPES)
+ return new_sm(uri, opts)
end
local function check_remote_arg(remote, method)
@@ -469,6 +506,7 @@ end
local function stream_begin(stream, txn_opts, netbox_opts)
check_remote_arg(stream, 'begin')
+ check_param_table(netbox_opts, REQUEST_OPTION_TYPES)
local timeout
local txn_isolation
if txn_opts then
@@ -494,6 +532,7 @@ end
local function stream_commit(stream, opts)
check_remote_arg(stream, 'commit')
+ check_param_table(opts, REQUEST_OPTION_TYPES)
local res = stream:_request(M_COMMIT, opts, nil, stream._stream_id)
if opts and opts.is_async then
return res
@@ -502,6 +541,7 @@ end
local function stream_rollback(stream, opts)
check_remote_arg(stream, 'rollback')
+ check_param_table(opts, REQUEST_OPTION_TYPES)
local res = stream:_request(M_ROLLBACK, opts, nil, stream._stream_id)
if opts and opts.is_async then
return res
@@ -707,7 +747,12 @@ function remote_methods:wait_connected(timeout)
return self:wait_state('active', timeout)
end
-function remote_methods:_request(method, opts, format, stream_id, ...)
+--
+-- Make a request, which throws an exception in case of critical errors
+-- (e.g. wrong API usage) and returns nil,err if there's connection related
+-- issues.
+--
+function remote_methods:_request_impl(method, opts, format, stream_id, ...)
local transport = self._transport
local on_push, on_push_ctx, buffer, skip_header, return_raw, deadline
-- Extract options, set defaults, check if the request is
@@ -720,14 +765,10 @@ function remote_methods:_request(method, opts, format, stream_id, ...)
if opts.on_push or opts.on_push_ctx then
error('To handle pushes in an async request use future:pairs()')
end
- local res, err =
- transport:perform_async_request(buffer, skip_header, return_raw,
- table.insert, {}, format,
- stream_id, method, ...)
- if err then
- box.error(err)
- end
- return res
+ return transport:perform_async_request(buffer, skip_header,
+ return_raw, table.insert,
+ {}, format, stream_id,
+ method, ...)
end
if opts.timeout then
deadline = fiber_clock() + opts.timeout
@@ -749,9 +790,6 @@ function remote_methods:_request(method, opts, format, stream_id, ...)
local res, err = transport:perform_request(timeout, buffer, skip_header,
return_raw, on_push, on_push_ctx,
format, stream_id, method, ...)
- if err then
- box.error(err)
- end
-- Try to wait until a schema is reloaded if needed.
-- Regardless of reloading result, the main response is
-- returned, since it does not depend on any schema things.
@@ -759,10 +797,19 @@ function remote_methods:_request(method, opts, format, stream_id, ...)
timeout = deadline and max(0, deadline - fiber_clock())
self:wait_state('active', timeout)
end
+ return res, err
+end
+
+function remote_methods:_request(method, opts, format, stream_id, ...)
+ local res, err = self:_request_impl(method, opts, format, stream_id, ...)
+ if err then
+ box.error(err)
+ end
return res
end
function remote_methods:_inject(str, opts)
+ check_param_table(opts, REQUEST_OPTION_TYPES)
return self:_request(M_INJECT, opts, nil, nil, str)
end
@@ -772,7 +819,12 @@ end
function remote_methods:ping(opts)
check_remote_arg(self, 'ping')
- return (pcall(self._request, self, M_PING, opts, nil, self._stream_id))
+ check_param_table(opts, REQUEST_OPTION_TYPES)
+ if opts and opts.is_async then
+ error("conn:ping() doesn't support `is_async` argument")
+ end
+ local _, err = self:_request_impl(M_PING, opts, nil, self._stream_id)
+ return err == nil
end
function remote_methods:reload_schema()
@@ -790,6 +842,7 @@ end
function remote_methods:call(func_name, args, opts)
check_remote_arg(self, 'call')
check_call_args(args)
+ check_param_table(opts, REQUEST_OPTION_TYPES)
args = args or {}
local res = self:_request(M_CALL_17, opts, nil, self._stream_id,
tostring(func_name), args)
@@ -809,6 +862,7 @@ end
function remote_methods:eval(code, args, opts)
check_remote_arg(self, 'eval')
check_eval_args(args)
+ check_param_table(opts, REQUEST_OPTION_TYPES)
args = args or {}
local res = self:_request(M_EVAL, opts, nil, self._stream_id, code, args)
if type(res) ~= 'table' or opts and opts.is_async then
@@ -822,6 +876,7 @@ function remote_methods:execute(query, parameters, sql_opts, netbox_opts)
if sql_opts ~= nil then
box.error(box.error.UNSUPPORTED, "execute", "options")
end
+ check_param_table(netbox_opts, REQUEST_OPTION_TYPES)
return self:_request(M_EXECUTE, netbox_opts, nil, self._stream_id,
query, parameters or {}, sql_opts or {})
end
@@ -834,6 +889,7 @@ function remote_methods:prepare(query, parameters, sql_opts, netbox_opts) -- lua
if sql_opts ~= nil then
box.error(box.error.UNSUPPORTED, "prepare", "options")
end
+ check_param_table(netbox_opts, REQUEST_OPTION_TYPES)
return self:_request(M_PREPARE, netbox_opts, nil, self._stream_id, query)
end
@@ -846,6 +902,7 @@ function remote_methods:unprepare(query, parameters, sql_opts, netbox_opts)
if sql_opts ~= nil then
box.error(box.error.UNSUPPORTED, "unprepare", "options")
end
+ check_param_table(netbox_opts, REQUEST_OPTION_TYPES)
return self:_request(M_UNPREPARE, netbox_opts, nil, self._stream_id,
query, parameters or {}, sql_opts or {})
end
@@ -984,12 +1041,14 @@ space_metatable = function(remote)
function methods:insert(tuple, opts)
check_space_arg(self, 'insert')
+ check_param_table(opts, REQUEST_OPTION_TYPES)
return remote:_request(M_INSERT, opts, self._format_cdata,
self._stream_id, self.id, tuple)
end
function methods:replace(tuple, opts)
check_space_arg(self, 'replace')
+ check_param_table(opts, REQUEST_OPTION_TYPES)
return remote:_request(M_REPLACE, opts, self._format_cdata,
self._stream_id, self.id, tuple)
end
@@ -1011,6 +1070,7 @@ space_metatable = function(remote)
function methods:upsert(key, oplist, opts)
check_space_arg(self, 'upsert')
+ check_param_table(opts, REQUEST_OPTION_TYPES)
return nothing_or_data(remote:_request(M_UPSERT, opts, nil,
self._stream_id, self.id,
key, oplist))
@@ -1037,6 +1097,7 @@ index_metatable = function(remote)
function methods:select(key, opts)
check_index_arg(self, 'select')
+ check_param_table(opts, REQUEST_OPTION_TYPES)
local key_is_nil = (key == nil or
(type(key) == 'table' and #key == 0))
local iterator, offset, limit = check_select_opts(opts, key_is_nil)
@@ -1047,6 +1108,7 @@ index_metatable = function(remote)
function methods:get(key, opts)
check_index_arg(self, 'get')
+ check_param_table(opts, REQUEST_OPTION_TYPES)
if opts and opts.buffer then
error("index:get() doesn't support `buffer` argument")
end
@@ -1059,6 +1121,7 @@ index_metatable = function(remote)
function methods:min(key, opts)
check_index_arg(self, 'min')
+ check_param_table(opts, REQUEST_OPTION_TYPES)
if opts and opts.buffer then
error("index:min() doesn't support `buffer` argument")
end
@@ -1071,6 +1134,7 @@ index_metatable = function(remote)
function methods:max(key, opts)
check_index_arg(self, 'max')
+ check_param_table(opts, REQUEST_OPTION_TYPES)
if opts and opts.buffer then
error("index:max() doesn't support `buffer` argument")
end
@@ -1083,6 +1147,7 @@ index_metatable = function(remote)
function methods:count(key, opts)
check_index_arg(self, 'count')
+ check_param_table(opts, REQUEST_OPTION_TYPES)
if opts and opts.buffer then
error("index:count() doesn't support `buffer` argument")
end
@@ -1094,6 +1159,7 @@ index_metatable = function(remote)
function methods:delete(key, opts)
check_index_arg(self, 'delete')
+ check_param_table(opts, REQUEST_OPTION_TYPES)
return nothing_or_data(remote:_request(M_DELETE, opts,
self.space._format_cdata,
self._stream_id, self.space.id,
@@ -1102,6 +1168,7 @@ index_metatable = function(remote)
function methods:update(key, oplist, opts)
check_index_arg(self, 'update')
+ check_param_table(opts, REQUEST_OPTION_TYPES)
return nothing_or_data(remote:_request(M_UPDATE, opts,
self.space._format_cdata,
self._stream_id, self.space.id,
diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
index 38e881c2ac..1c658d996e 100644
--- a/src/box/lua/schema.lua
+++ b/src/box/lua/schema.lua
@@ -248,7 +248,9 @@ end
@param table - table with parameters
@param template - table with expected types of expected parameters
type could be comma separated string with lua types (number, string etc),
- or 'any' if any type is allowed
+ or 'any' if any type is allowed. Instead of this string, function, which will
+ be used to check if the parameter is correct, can be used too. It should
+ accept option as an argument and return either true or false + expected_type.
The function checks following:
1)that parameters table is a table (or nil)
2)all keys in parameters are present in template
@@ -259,7 +261,13 @@ end
@example
check_param_table(options, { user = 'string',
port = 'string, number',
- data = 'any'} )
+ data = 'any',
+ addr = function(opt)
+ if not ffi.istype(addr_t, buf) then
+ return false, "struct addr"
+ end
+ return true
+ end} )
--]]
local function check_param_table(table, template)
if table == nil then
@@ -277,6 +285,13 @@ local function check_param_table(table, template)
if template[k] == nil then
box.error(box.error.ILLEGAL_PARAMS,
"unexpected option '" .. k .. "'")
+ elseif type(template[k]) == 'function' then
+ local res, expected_type = template[k](v)
+ if not res then
+ box.error(box.error.ILLEGAL_PARAMS,
+ "options parameter '" .. k ..
+ "' should be of type " .. expected_type)
+ end
elseif template[k] == 'any' then -- luacheck: ignore
-- any type is ok
elseif (string.find(template[k], ',') == nil) then
diff --git a/test/box-luatest/gh_6530_net_box_opts_check_test.lua b/test/box-luatest/gh_6530_net_box_opts_check_test.lua
new file mode 100644
index 0000000000..084f80237d
--- /dev/null
+++ b/test/box-luatest/gh_6530_net_box_opts_check_test.lua
@@ -0,0 +1,184 @@
+local server = require('test.luatest_helpers.server')
+local buffer = require('buffer')
+local net = require('net.box')
+local t = require('luatest')
+local g = t.group()
+
+g.before_all = function()
+ g.server = server:new{alias = 'default'}
+ g.server:start()
+ g.server:exec(function()
+ local s = box.schema.space.create('test')
+ s:format({
+ {name = 'id', type = 'unsigned'},
+ {name = 'data', type = 'string'},
+ })
+ s:create_index('primary', {parts = {'id'}})
+ box.schema.user.grant('guest', 'read,write', 'space', 'test')
+ end)
+
+ g.conn = net.connect(g.server.net_box_uri)
+end
+
+g.after_all = function()
+ g.server:exec(function()
+ if box.space.test then
+ box.space.test:drop()
+ end
+ end)
+
+ g.conn:close()
+ g.server:drop()
+end
+
+g.test_netbox_connect_opts = function()
+ t.assert_error_msg_contains("unexpected option 'some_opt'", function()
+ net.connect(g.server.net_box_uri, {some_opt = 1})
+ end)
+ t.assert_error_msg_contains("parameter 'user' should be of type string", function()
+ net.connect(g.server.net_box_uri, {user = true})
+ end)
+ t.assert_error_msg_contains("parameter 'password' should be of type string", function()
+ net.connect(g.server.net_box_uri, {password = true})
+ end)
+ t.assert_error_msg_contains("parameter 'wait_connected' should be one of types: number, boolean", function()
+ net.connect(g.server.net_box_uri, {wait_connected = 'string'})
+ end)
+ t.assert_error_msg_contains("parameter 'reconnect_after' should be of type number", function()
+ net.connect(g.server.net_box_uri, {reconnect_after = true})
+ end)
+ t.assert_error_msg_contains("parameter 'call_16' should be of type boolean", function()
+ net.connect(g.server.net_box_uri, {call_16 = 1})
+ end)
+ t.assert_error_msg_contains("parameter 'console' should be of type boolean", function()
+ net.connect(g.server.net_box_uri, {console = 1})
+ end)
+ t.assert_error_msg_contains("parameter 'connect_timeout' should be of type number", function()
+ net.connect(g.server.net_box_uri, {connect_timeout = 'string'})
+ end)
+ t.assert_error_msg_contains("parameter 'required_protocol_version' should be of type number", function()
+ net.connect(g.server.net_box_uri, {required_protocol_version = 'string'})
+ end)
+ t.assert_error_msg_contains("parameter 'required_protocol_features' should be of type table", function()
+ net.connect(g.server.net_box_uri, {required_protocol_features = 1})
+ end)
+end
+
+g.test_netbox_select_opts = function()
+ t.assert_error_msg_contains("options should be a table", function()
+ g.conn.space.test:select({1}, 123)
+ end)
+ t.assert_error_msg_contains("parameter 'timeout' should be of type number", function()
+ g.conn.space.test:select({1}, {timeout = true})
+ end)
+ t.assert_error_msg_contains("parameter 'buffer' should be of type struct ibuf", function()
+ g.conn.space.test:select({1}, {buffer = true})
+ end)
+ t.assert_error_msg_contains("parameter 'is_async' should be of type boolean", function()
+ g.conn.space.test:select({1}, {is_async = 1})
+ end)
+ t.assert_error_msg_contains("parameter 'on_push' should be of type function", function()
+ g.conn.space.test:select({1}, {on_push = 1})
+ end)
+ t.assert_error_msg_contains("parameter 'return_raw' should be of type boolean", function()
+ g.conn.space.test:select({1}, {return_raw = 1})
+ end)
+ t.assert_error_msg_contains("in an async request use future:pairs()", function()
+ g.conn.space.test:select({1}, {is_async = true, on_push_ctx = true })
+ end)
+end
+
+g.test_netbox_all_methods_opts = function()
+ t.assert_error_msg_contains("options should be a table", function()
+ g.conn:ping(123)
+ end)
+
+ t.assert_error_msg_contains("options should be a table", function()
+ g.conn.space.test:get({1}, 123)
+ end)
+ t.assert_error_msg_contains("options should be a table", function()
+ g.conn.space.test:insert({1, 'A'}, 123)
+ end)
+ t.assert_error_msg_contains("options should be a table", function()
+ g.conn.space.test:replace({1, 'A'}, 123)
+ end)
+ t.assert_error_msg_contains("options should be a table", function()
+ g.conn.space.test:update(1, {'!', 1, 'A'}, 123)
+ end)
+ t.assert_error_msg_contains("options should be a table", function()
+ g.conn.space.test:upsert(1, {'!', 1, 'A'}, 123)
+ end)
+ t.assert_error_msg_contains("options should be a table", function()
+ g.conn.space.test:delete({1}, 123)
+ end)
+
+ t.assert_error_msg_contains("options should be a table", function()
+ g.conn.space.test.index.primary:select({1}, 123)
+ end)
+ t.assert_error_msg_contains("options should be a table", function()
+ g.conn.space.test.index.primary:get({1}, 123)
+ end)
+ t.assert_error_msg_contains("options should be a table", function()
+ g.conn.space.test.index.primary:min({1}, 123)
+ end)
+ t.assert_error_msg_contains("options should be a table", function()
+ g.conn.space.test.index.primary:max({1}, 123)
+ end)
+ t.assert_error_msg_contains("options should be a table", function()
+ g.conn.space.test.index.primary:count({1}, 123)
+ end)
+ t.assert_error_msg_contains("options should be a table", function()
+ g.conn.space.test.index.primary:delete({1}, 123)
+ end)
+ t.assert_error_msg_contains("options should be a table", function()
+ g.conn.space.test.index.primary:update(1, {'!', 1, 'A'}, 123)
+ end)
+
+ t.assert_error_msg_contains("options should be a table", function()
+ g.conn:eval('return {nil,5}', {}, 123)
+ end)
+ t.assert_error_msg_contains("options should be a table", function()
+ g.conn:call('f2',{1,'B'}, 123)
+ end)
+
+ local stream = g.conn:new_stream()
+ t.assert_error_msg_contains("options should be a table", function()
+ stream:begin({}, 123)
+ end)
+ t.assert_error_msg_contains("options should be a table", function()
+ stream:commit(123)
+ end)
+ t.assert_error_msg_contains("options should be a table", function()
+ stream:rollback(123)
+ end)
+
+ t.assert_error_msg_contains("options should be a table", function()
+ g.conn:execute('', {}, nil, 123)
+ end)
+ t.assert_error_msg_contains("options should be a table", function()
+ g.conn:prepare('', {}, nil, 123)
+ end)
+ t.assert_error_msg_contains("options should be a table", function()
+ g.conn:unprepare(1, {}, nil, 123)
+ end)
+end
+
+g.test_netbox_not_supported_opts = function()
+ t.assert_error_msg_contains("doesn't support", function()
+ g.conn:ping({is_async = true})
+ end)
+
+ local ibuf = buffer.ibuf()
+ t.assert_error_msg_contains("doesn't support", function()
+ g.conn.space.test.index.primary:get({1}, {buffer = ibuf})
+ end)
+ t.assert_error_msg_contains("doesn't support", function()
+ g.conn.space.test.index.primary:min({1}, {buffer = ibuf})
+ end)
+ t.assert_error_msg_contains("doesn't support", function()
+ g.conn.space.test.index.primary:max({1}, {buffer = ibuf})
+ end)
+ t.assert_error_msg_contains("doesn't support", function()
+ g.conn.space.test.index.primary:count({1}, {buffer = ibuf})
+ end)
+end
diff --git a/test/box-luatest/net_box_test.lua b/test/box-luatest/net_box_test.lua
index 023b45ce09..d15f503f5d 100644
--- a/test/box-luatest/net_box_test.lua
+++ b/test/box-luatest/net_box_test.lua
@@ -307,7 +307,7 @@ g.test_box_error = function()
t.assert_error_msg_equals(
"Illegal parameters, Netbox text protocol support was dropped, "..
"please use require('console').connect() instead",
- net.connect, g.server.net_box_uri, {console = 123})
+ net.connect, g.server.net_box_uri, {console = true})
local c = net.connect(g.server.net_box_uri)
t.assert_error_msg_equals(
--
GitLab