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 0000000000000000000000000000000000000000..25939358ac06f619add3b19d1b18ff33d50938ce --- /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 f68f0ea680cde03829533fb51c5766d68904524a..7eef59779eb9dd9b91f1b5c636c40b78b223d1b4 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 d591878e27a0aea63f3e73b34ebb849a2640b7e4..13605c4ac9969e4c59bd8faa239891706c8fd6d0 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 aafefe4d1590a0fead59fd1e322f5bcfa7242a44..12d2814e358fa6094db79a3ff6b8441d30d8d22e 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 ec3c43e685cfd6c48efa0d3b4e0092fa890b7386..529da1578147b5fe52e9fdccce39ad0e2b2397e2 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 0000000000000000000000000000000000000000..2c245685213794955c8bfec1f07372af4e0be75e --- /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 0000000000000000000000000000000000000000..382791adfae0ca92fa18d3ca9cfad92b6803e4c2 --- /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 4c164abcaff8bdf2f67be6fab0db45c737639e67..a53ba4d41c901b84ae20a85df44ea3d36f5a2fd5 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