From 97be44aff01d086e3e06428c75cef02f6f3b8f5f Mon Sep 17 00:00:00 2001
From: mechanik20051988 <mechanik20051988@tarantool.org>
Date: Fri, 1 Oct 2021 16:12:04 +0300
Subject: [PATCH] iproto: fix tarantool blindness in case of invalid listen
uri.
In case user enters invalid listen address, tarantool closes previous listen
socket, but bind on invalid address fails also, so tarantool becames blind -
no listening socket at all. This patch fixed this behaviour, now tarantool
still listen old listen address.
Closes #6092
---
...lid-listen-address-make-tarantool-blind.md | 4 +
src/box/iproto.cc | 8 ++
src/box/lua/load_cfg.lua | 33 ++++-
src/lib/core/errinj.h | 1 +
test/box/errinj.result | 1 +
test/box/gh-6092-invalid-listen-uri.result | 120 ++++++++++++++++++
test/box/gh-6092-invalid-listen-uri.test.lua | 46 +++++++
test/box/suite.ini | 2 +-
8 files changed, 213 insertions(+), 2 deletions(-)
create mode 100644 changelogs/unreleased/gh-6092-invalid-listen-address-make-tarantool-blind.md
create mode 100644 test/box/gh-6092-invalid-listen-uri.result
create mode 100644 test/box/gh-6092-invalid-listen-uri.test.lua
diff --git a/changelogs/unreleased/gh-6092-invalid-listen-address-make-tarantool-blind.md b/changelogs/unreleased/gh-6092-invalid-listen-address-make-tarantool-blind.md
new file mode 100644
index 0000000000..25939358ac
--- /dev/null
+++ b/changelogs/unreleased/gh-6092-invalid-listen-address-make-tarantool-blind.md
@@ -0,0 +1,4 @@
+## bugfix/core
+
+* Fixed box.cfg.listen not reverted to the old address
+ in case the new one is invalid (gh-6092).
diff --git a/src/box/iproto.cc b/src/box/iproto.cc
index f68f0ea680..7eef59779e 100644
--- a/src/box/iproto.cc
+++ b/src/box/iproto.cc
@@ -2914,6 +2914,7 @@ iproto_do_cfg_f(struct cbus_call_msg *m)
struct iproto_cfg_msg *cfg_msg = (struct iproto_cfg_msg *) m;
int old;
struct iproto_thread *iproto_thread = cfg_msg->iproto_thread;
+ struct errinj *inj;
try {
switch (cfg_msg->op) {
@@ -2926,6 +2927,13 @@ iproto_do_cfg_f(struct cbus_call_msg *m)
iproto_resume(iproto_thread);
break;
case IPROTO_CFG_LISTEN:
+ inj = errinj(ERRINJ_IPROTO_CFG_LISTEN, ERRINJ_INT);
+ if (inj != NULL && inj->iparam > 0) {
+ inj->iparam--;
+ diag_set(ClientError, ER_INJECTION,
+ "iproto listen");
+ diag_raise();
+ }
if (evio_service_is_active(&iproto_thread->binary)) {
diag_set(ClientError, ER_UNSUPPORTED, "Iproto",
"listen if service already active");
diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua
index d591878e27..13605c4ac9 100644
--- a/src/box/lua/load_cfg.lua
+++ b/src/box/lua/load_cfg.lua
@@ -337,6 +337,22 @@ local dynamic_cfg = {
sql_cache_size = private.cfg_set_sql_cache_size,
}
+-- dynamically settable options, which should be reverted in case
+-- there change fails.
+local dynamic_cfg_revert = {
+ listen = private.cfg_set_listen,
+}
+
+-- Values of dynamically settable options, the revert to which cannot fail.
+-- If trying to change the value of dynamically settable option fails, we
+-- try to rollback to previous value of this option. If rollback is also fails
+-- we rollback to the value, which contains here. This table should contain
+-- such values, that rollback for them can't fails. It's necessary to prevent
+-- inconsistent state.
+local default_cfg_on_revert = {
+ listen = nil,
+}
+
ifdef_feedback = nil -- luacheck: ignore
ifdef_feedback_set_params = nil -- luacheck: ignore
@@ -613,8 +629,23 @@ local function reload_cfg(oldcfg, cfg)
local oldval = oldcfg[key]
if not compare_cfg(val, oldval) then
rawset(oldcfg, key, val)
- if not pcall(dynamic_cfg[key]) then
+ local result, err = pcall(dynamic_cfg[key])
+ if not result then
+ local save_err = err
rawset(oldcfg, key, oldval) -- revert the old value
+ if dynamic_cfg_revert[key] then
+ result, err = pcall(dynamic_cfg_revert[key])
+ if not result then
+ log.error("failed to revert '%s' " ..
+ "configuration option: %s",
+ key, err)
+ -- We set the value from special table, rollback to
+ -- which cannot fail.
+ rawset(oldcfg, key, default_cfg_on_revert[key])
+ assert(pcall(dynamic_cfg_revert[key]))
+ end
+ end
+ box.error.set(save_err)
return box.error() -- re-throw
end
if log_cfg_option[key] ~= nil then
diff --git a/src/lib/core/errinj.h b/src/lib/core/errinj.h
index aafefe4d15..12d2814e35 100644
--- a/src/lib/core/errinj.h
+++ b/src/lib/core/errinj.h
@@ -89,6 +89,7 @@ struct errinj {
_(ERRINJ_HTTP_RESPONSE_ADD_WAIT, ERRINJ_BOOL, {.bparam = false}) \
_(ERRINJ_INDEX_ALLOC, ERRINJ_BOOL, {.bparam = false}) \
_(ERRINJ_INDEX_RESERVE, ERRINJ_BOOL, {.bparam = false})\
+ _(ERRINJ_IPROTO_CFG_LISTEN, ERRINJ_INT, {.iparam = 0}) \
_(ERRINJ_IPROTO_DISABLE_ID, ERRINJ_BOOL, {.bparam = false}) \
_(ERRINJ_IPROTO_FLIP_FEATURE, ERRINJ_INT, {.iparam = -1}) \
_(ERRINJ_IPROTO_SET_VERSION, ERRINJ_INT, {.iparam = -1}) \
diff --git a/test/box/errinj.result b/test/box/errinj.result
index ec3c43e685..529da15781 100644
--- a/test/box/errinj.result
+++ b/test/box/errinj.result
@@ -59,6 +59,7 @@ evals
- ERRINJ_HTTP_RESPONSE_ADD_WAIT: false
- ERRINJ_INDEX_ALLOC: false
- ERRINJ_INDEX_RESERVE: false
+ - ERRINJ_IPROTO_CFG_LISTEN: 0
- ERRINJ_IPROTO_DISABLE_ID: false
- ERRINJ_IPROTO_FLIP_FEATURE: -1
- ERRINJ_IPROTO_SET_VERSION: -1
diff --git a/test/box/gh-6092-invalid-listen-uri.result b/test/box/gh-6092-invalid-listen-uri.result
new file mode 100644
index 0000000000..2c24568521
--- /dev/null
+++ b/test/box/gh-6092-invalid-listen-uri.result
@@ -0,0 +1,120 @@
+test_run = require('test_run').new()
+---
+...
+netbox = require('net.box')
+---
+...
+fio = require('fio')
+---
+...
+errinj = box.error.injection
+---
+...
+-- Check that an invalid listening uri
+-- does not make tarantool blind.
+bad_uri = "baduribaduri:1"
+---
+...
+old_listen = box.cfg.listen
+---
+...
+box.cfg({ listen = bad_uri })
+---
+- error: can't resolve uri for bind, called on fd -1
+...
+conn = netbox.connect(old_listen)
+---
+...
+assert(conn:ping())
+---
+- true
+...
+assert(fio.path.exists(old_listen))
+---
+- true
+...
+assert(box.cfg.listen == old_listen)
+---
+- true
+...
+-- Check that failure in listen does
+-- not make tarantool blind and not
+-- leads to unreleased resources.
+errinj.set("ERRINJ_IPROTO_CFG_LISTEN", 1)
+---
+- ok
+...
+new_listen = old_listen .. "A"
+---
+...
+box.cfg({ listen = new_listen })
+---
+- error: Error injection 'iproto listen'
+...
+test_run:wait_cond(function() return fio.path.exists(old_listen) end)
+---
+- true
+...
+test_run:wait_cond(function() return not fio.path.exists(new_listen) end)
+---
+- true
+...
+conn = netbox.connect(old_listen)
+---
+...
+assert(conn:ping())
+---
+- true
+...
+assert(box.cfg.listen == old_listen)
+---
+- true
+...
+-- Check the error message when listen fails
+-- and reverts to old listen fails also.
+-- We set 'ERRINJ_IPROTO_CFG_LISTEN' to 2, so
+-- listen fails and rollback to old value fails
+-- also. So we rollback to special value (for
+-- box.cfg.listen it's nil), it can't fails.
+errinj.set("ERRINJ_IPROTO_CFG_LISTEN", 2)
+---
+- ok
+...
+new_listen = old_listen .. "A"
+---
+...
+box.cfg({ listen = new_listen })
+---
+- error: Error injection 'iproto listen'
+...
+test_run:wait_cond(function() return not fio.path.exists(old_listen) end)
+---
+- true
+...
+test_run:wait_cond(function() return not fio.path.exists(new_listen) end)
+---
+- true
+...
+conn = netbox.connect(old_listen)
+---
+...
+assert(not conn:ping())
+---
+- true
+...
+errmsg = "failed to revert 'listen' configuration option: " .. \
+ "Error injection 'iproto listen'"
+---
+...
+assert(test_run:grep_log('default', errmsg))
+---
+- 'failed to revert ''listen'' configuration option: Error injection ''iproto listen'''
+...
+assert(box.cfg.listen == nil)
+---
+- true
+...
+assert(box.info.listen == nil)
+---
+- true
+...
diff --git a/test/box/gh-6092-invalid-listen-uri.test.lua b/test/box/gh-6092-invalid-listen-uri.test.lua
new file mode 100644
index 0000000000..382791adfa
--- /dev/null
+++ b/test/box/gh-6092-invalid-listen-uri.test.lua
@@ -0,0 +1,46 @@
+test_run = require('test_run').new()
+netbox = require('net.box')
+fio = require('fio')
+errinj = box.error.injection
+
+-- Check that an invalid listening uri
+-- does not make tarantool blind.
+bad_uri = "baduribaduri:1"
+old_listen = box.cfg.listen
+box.cfg({ listen = bad_uri })
+conn = netbox.connect(old_listen)
+assert(conn:ping())
+assert(fio.path.exists(old_listen))
+assert(box.cfg.listen == old_listen)
+
+-- Check that failure in listen does
+-- not make tarantool blind and not
+-- leads to unreleased resources.
+errinj.set("ERRINJ_IPROTO_CFG_LISTEN", 1)
+new_listen = old_listen .. "A"
+box.cfg({ listen = new_listen })
+test_run:wait_cond(function() return fio.path.exists(old_listen) end)
+test_run:wait_cond(function() return not fio.path.exists(new_listen) end)
+conn = netbox.connect(old_listen)
+assert(conn:ping())
+assert(box.cfg.listen == old_listen)
+
+-- Check the error message when listen fails
+-- and reverts to old listen fails also.
+-- We set 'ERRINJ_IPROTO_CFG_LISTEN' to 2, so
+-- listen fails and rollback to old value fails
+-- also. So we rollback to special value (for
+-- box.cfg.listen it's nil), it can't fails.
+errinj.set("ERRINJ_IPROTO_CFG_LISTEN", 2)
+new_listen = old_listen .. "A"
+box.cfg({ listen = new_listen })
+test_run:wait_cond(function() return not fio.path.exists(old_listen) end)
+test_run:wait_cond(function() return not fio.path.exists(new_listen) end)
+conn = netbox.connect(old_listen)
+assert(not conn:ping())
+errmsg = "failed to revert 'listen' configuration option: " .. \
+ "Error injection 'iproto listen'"
+assert(test_run:grep_log('default', errmsg))
+assert(box.cfg.listen == nil)
+assert(box.info.listen == nil)
+
diff --git a/test/box/suite.ini b/test/box/suite.ini
index 4c164abcaf..a53ba4d41c 100644
--- a/test/box/suite.ini
+++ b/test/box/suite.ini
@@ -5,7 +5,7 @@ script = box.lua
disabled = rtree_errinj.test.lua tuple_bench.test.lua
long_run = huge_field_map_long.test.lua alter-primary-index-tuple-leak-long.test.lua
config = engine.cfg
-release_disabled = errinj.test.lua errinj_index.test.lua rtree_errinj.test.lua upsert_errinj.test.lua iproto_stress.test.lua gh-4648-func-load-unload.test.lua net.box_discard_console_request_gh-6249.test.lua gh-5998-one-tx-for-ddl-errinj.test.lua net.box_closing_without_lost_gh-6338.test.lua net.box_iproto_id.test.lua net.box_error_extension_feature.test.lua
+release_disabled = errinj.test.lua errinj_index.test.lua rtree_errinj.test.lua upsert_errinj.test.lua iproto_stress.test.lua gh-4648-func-load-unload.test.lua net.box_discard_console_request_gh-6249.test.lua gh-5998-one-tx-for-ddl-errinj.test.lua net.box_closing_without_lost_gh-6338.test.lua net.box_iproto_id.test.lua net.box_error_extension_feature.test.lua gh-6092-invalid-listen-uri.test.lua
lua_libs = lua/fifo.lua lua/utils.lua lua/bitset.lua lua/index_random_test.lua lua/push.lua lua/identifier.lua lua/txn_proxy.lua
use_unix_sockets = True
use_unix_sockets_iproto = True
--
GitLab