From 96128a5076b7e45fc01163151401a9e2acdff565 Mon Sep 17 00:00:00 2001 From: "Justin M. Keyes" Date: Mon, 2 Sep 2024 15:57:07 +0200 Subject: [PATCH] feat(startup): validate --listen address Problem: `nvim --listen` does not error on EADDRINUSE. #30123 Solution: Now that `$NVIM_LISTEN_ADDRESS` is deprecated and input *only* (instead of the old, ambiguous situation where it was both an input *and* an output), we can be fail fast instead of trying to "recover". This reverts the "recovery" behavior of 704ba4151e7f67999510ee0ac19fdabb595d530c, but that was basically a workaround for the fragility of `$NVIM_LISTEN_ADDRESS`. --- runtime/doc/news.txt | 15 ++--- src/nvim/api/ui.c | 8 +-- src/nvim/main.c | 14 +++-- src/nvim/msgpack_rpc/server.c | 31 +++++++---- test/functional/options/defaults_spec.lua | 25 ++++++--- test/functional/vimscript/server_spec.lua | 67 +++++++++++++++-------- 6 files changed, 101 insertions(+), 59 deletions(-) diff --git a/runtime/doc/news.txt b/runtime/doc/news.txt index 3d1ea8548f..dacb27e320 100644 --- a/runtime/doc/news.txt +++ b/runtime/doc/news.txt @@ -53,12 +53,6 @@ EDITOR documented and skips help buffers if run from a non-help buffer, otherwise it moves to another help buffer. -VIM SCRIPT - -• |v:msgpack_types| has the type "binary" removed. |msgpackparse()| no longer - treats BIN, STR and FIXSTR as separate types. Any of these is returned as a - string if possible, or a |blob| if the value contained embedded NUL:s. - EVENTS • TODO @@ -98,6 +92,12 @@ TUI • TODO +VIMSCRIPT + +• |v:msgpack_types| has the type "binary" removed. |msgpackparse()| no longer + treats BIN, STR and FIXSTR as separate types. Any of these is returned as a + string if possible, or a |blob| if the value contained embedded NUL:s. + ============================================================================== NEW FEATURES *news-features* @@ -162,7 +162,8 @@ PLUGINS STARTUP -• TODO +• Nvim will fail if the |--listen| or |$NVIM_LISTEN_ADDRESS| address is + invalid, instead of silently skipping an invalid address. TERMINAL diff --git a/src/nvim/api/ui.c b/src/nvim/api/ui.c index a99d97acb8..5dc373acdc 100644 --- a/src/nvim/api/ui.c +++ b/src/nvim/api/ui.c @@ -93,15 +93,15 @@ void remote_ui_free_all_mem(void) } #endif -/// Wait until ui has connected on stdio channel if only_stdio -/// is true, otherwise any channel. +/// Wait until UI has connected. +/// +/// @param only_stdio UI is expected to connect on stdio. void remote_ui_wait_for_attach(bool only_stdio) { if (only_stdio) { Channel *channel = find_channel(CHAN_STDIO); if (!channel) { - // this function should only be called in --embed mode, stdio channel - // can be assumed. + // `only_stdio` implies --embed mode, thus stdio channel can be assumed. abort(); } diff --git a/src/nvim/main.c b/src/nvim/main.c index 6b90a13e1e..a45ee81c81 100644 --- a/src/nvim/main.c +++ b/src/nvim/main.c @@ -332,12 +332,6 @@ int main(int argc, char **argv) #endif bool use_builtin_ui = (has_term && !headless_mode && !embedded_mode && !silent_mode); - // don't bind the server yet, if we are using builtin ui. - // This will be done when nvim server has been forked from the ui process - if (!use_builtin_ui) { - server_init(params.listen_addr); - } - if (params.remote) { remote_request(¶ms, params.remote, params.server_addr, argc, argv, use_builtin_ui); @@ -355,11 +349,19 @@ int main(int argc, char **argv) ui_client_channel_id = rv; } + // NORETURN: Start builtin UI client. if (ui_client_channel_id) { time_finish(); ui_client_run(remote_ui); // NORETURN } assert(!ui_client_channel_id && !use_builtin_ui); + // Nvim server... + + int listen_rv = server_init(params.listen_addr); + if (listen_rv != 0) { + mainerr("Failed to --listen", listen_rv < 0 + ? os_strerror(listen_rv) : (listen_rv == 1 ? "empty address" : NULL)); + } TIME_MSG("expanding arguments"); diff --git a/src/nvim/msgpack_rpc/server.c b/src/nvim/msgpack_rpc/server.c index 24bd343a34..ae34829181 100644 --- a/src/nvim/msgpack_rpc/server.c +++ b/src/nvim/msgpack_rpc/server.c @@ -28,27 +28,32 @@ static garray_T watchers = GA_EMPTY_INIT_VALUE; #endif /// Initializes the module -bool server_init(const char *listen_addr) +/// +/// @returns 0: success, 1: validation error, 2: already listening, -errno: failed to bind/listen. +int server_init(const char *listen_addr) { + bool must_free = false; ga_init(&watchers, sizeof(SocketWatcher *), 1); // $NVIM_LISTEN_ADDRESS (deprecated) - if (!listen_addr && os_env_exists(ENV_LISTEN)) { + if ((!listen_addr || listen_addr[0] == '\0') && os_env_exists(ENV_LISTEN)) { listen_addr = os_getenv(ENV_LISTEN); } - int rv = listen_addr ? server_start(listen_addr) : 1; - if (0 != rv) { + if (!listen_addr || listen_addr[0] == '\0') { listen_addr = server_address_new(NULL); - if (!listen_addr) { - return false; - } - rv = server_start(listen_addr); - xfree((char *)listen_addr); + must_free = true; } + if (!listen_addr) { + abort(); // Cannot happen. + } + + int rv = server_start(listen_addr); + if (os_env_exists(ENV_LISTEN)) { - // Unset $NVIM_LISTEN_ADDRESS, it's a liability hereafter. + // Unset $NVIM_LISTEN_ADDRESS, it's a liability hereafter. It is "input only", it must not be + // leaked to child jobs or :terminal. os_unsetenv(ENV_LISTEN); } @@ -57,7 +62,11 @@ bool server_init(const char *listen_addr) ELOG("test log message"); } - return rv == 0; + if (must_free) { + xfree((char *)listen_addr); + } + + return rv; } /// Teardown a single server diff --git a/test/functional/options/defaults_spec.lua b/test/functional/options/defaults_spec.lua index f61139d92d..0faced5149 100644 --- a/test/functional/options/defaults_spec.lua +++ b/test/functional/options/defaults_spec.lua @@ -247,6 +247,7 @@ describe('startup defaults', function() } }) eq('Xtest-logpath', eval('$NVIM_LOG_FILE')) end) + it('defaults to stdpath("log")/log if empty', function() eq(true, mkdir(xdgdir) and mkdir(xdgstatedir)) clear({ @@ -257,6 +258,7 @@ describe('startup defaults', function() }) eq(xdgstatedir .. '/log', string.gsub(eval('$NVIM_LOG_FILE'), '\\', '/')) end) + it('defaults to stdpath("log")/log if invalid', function() eq(true, mkdir(xdgdir) and mkdir(xdgstatedir)) clear({ @@ -266,6 +268,8 @@ describe('startup defaults', function() }, }) eq(xdgstatedir .. '/log', string.gsub(eval('$NVIM_LOG_FILE'), '\\', '/')) + -- Avoid "failed to open $NVIM_LOG_FILE" noise in test output. + expect_exit(command, 'qall!') end) end) end) @@ -339,9 +343,11 @@ describe('XDG defaults', function() local state_dir = is_os('win') and 'nvim-data' or 'nvim' local root_path = is_os('win') and 'C:' or '' - describe('with too long XDG variables', function() + describe('with too long XDG vars', function() before_each(function() clear({ + -- Ensure valid --listen address despite broken XDG vars (else Nvim won't start). + args = { '--listen', is_os('win') and '' or t.tmpname(false) }, args_rm = { 'runtimepath' }, env = { NVIM_LOG_FILE = testlog, @@ -361,6 +367,9 @@ describe('XDG defaults', function() it('are correctly set', function() if not is_os('win') then + -- Broken XDG vars cause serverstart() to fail (except on Windows, where servernames are not + -- informed by $XDG_STATE_HOME). + t.matches('Failed to start server: no such file or directory', t.pcall_err(fn.serverstart)) assert_log('Failed to start server: no such file or directory: /X/X/X', testlog, 10) end @@ -522,9 +531,11 @@ describe('XDG defaults', function() end) end) - describe('with XDG variables that can be expanded', function() + describe('with expandable XDG vars', function() before_each(function() clear({ + -- Ensure valid --listen address despite broken XDG vars (else Nvim won't start). + args = { '--listen', is_os('win') and '' or t.tmpname(false) }, args_rm = { 'runtimepath' }, env = { NVIM_LOG_FILE = testlog, @@ -544,6 +555,9 @@ describe('XDG defaults', function() it('are not expanded', function() if not is_os('win') then + -- Broken XDG vars cause serverstart() to fail (except on Windows, where servernames are not + -- informed by $XDG_STATE_HOME). + t.matches('Failed to start server: no such file or directory', t.pcall_err(fn.serverstart)) assert_log( 'Failed to start server: no such file or directory: %$XDG_RUNTIME_DIR%/', testlog, @@ -895,7 +909,7 @@ describe('stdpath()', function() assert_alive() -- Check for crash. #8393 end) - it('reacts to $NVIM_APPNAME', function() + it('supports $NVIM_APPNAME', function() local appname = 'NVIM_APPNAME_TEST' .. ('_'):rep(106) clear({ env = { NVIM_APPNAME = appname, NVIM_LOG_FILE = testlog } }) eq(appname, fn.fnamemodify(fn.stdpath('config'), ':t')) @@ -916,7 +930,7 @@ describe('stdpath()', function() local function test_appname(testAppname, expected_exitcode) local lua_code = string.format( [[ - local child = vim.fn.jobstart({ vim.v.progpath, '--clean', '--headless', '+qall!' }, { env = { NVIM_APPNAME = %q } }) + local child = vim.fn.jobstart({ vim.v.progpath, '--clean', '--headless', '--listen', 'x', '+qall!' }, { env = { NVIM_APPNAME = %q } }) return vim.fn.jobwait({ child }, %d)[1] ]], alter_slashes(testAppname), @@ -935,9 +949,6 @@ describe('stdpath()', function() -- Valid appnames: test_appname('a/b', 0) test_appname('a/b\\c', 0) - if not is_os('win') then - assert_log('Failed to start server: no such file or directory:', testlog) - end end) describe('returns a String', function() diff --git a/test/functional/vimscript/server_spec.lua b/test/functional/vimscript/server_spec.lua index 4b0dc087f6..836d841f69 100644 --- a/test/functional/vimscript/server_spec.lua +++ b/test/functional/vimscript/server_spec.lua @@ -4,7 +4,6 @@ local n = require('test.functional.testnvim')() local assert_log = t.assert_log local eq, neq, eval = t.eq, t.neq, n.eval local clear, fn, api = n.clear, n.fn, n.api -local ok = t.ok local matches = t.matches local pcall_err = t.pcall_err local check_close = n.check_close @@ -49,15 +48,6 @@ describe('server', function() eq('', eval('$NVIM_LISTEN_ADDRESS')) end) - it('sets new v:servername if $NVIM_LISTEN_ADDRESS is invalid', function() - clear({ env = { NVIM_LISTEN_ADDRESS = '.' } }) - -- Cleared on startup. - eq('', eval('$NVIM_LISTEN_ADDRESS')) - local servers = fn.serverlist() - eq(1, #servers) - ok(string.len(servers[1]) > 4) -- "~/.local/state/nvim…/…" or "\\.\pipe\…" - end) - it('sets v:servername at startup or if all servers were stopped', function() clear() local initial_server = api.nvim_get_vvar('servername') @@ -89,20 +79,26 @@ describe('server', function() end) it('serverstop() returns false for invalid input', function() - clear { env = { - NVIM_LOG_FILE = testlog, - NVIM_LISTEN_ADDRESS = '.', - } } + clear { + args_rm = { '--listen' }, + env = { + NVIM_LOG_FILE = testlog, + NVIM_LISTEN_ADDRESS = '', + }, + } eq(0, eval("serverstop('')")) eq(0, eval("serverstop('bogus-socket-name')")) assert_log('Not listening on bogus%-socket%-name', testlog, 10) end) it('parses endpoints', function() - clear { env = { - NVIM_LOG_FILE = testlog, - NVIM_LISTEN_ADDRESS = '.', - } } + clear { + args_rm = { '--listen' }, + env = { + NVIM_LOG_FILE = testlog, + NVIM_LISTEN_ADDRESS = '', + }, + } clear_serverlist() eq({}, fn.serverlist()) @@ -178,18 +174,41 @@ end) describe('startup --listen', function() it('validates', function() clear() - local cmd = { unpack(n.nvim_argv) } - table.insert(cmd, '--listen') - matches('nvim.*: Argument missing after: "%-%-listen"', fn.system(cmd)) - cmd = { unpack(n.nvim_argv) } - table.insert(cmd, '--listen2') - matches('nvim.*: Garbage after option argument: "%-%-listen2"', fn.system(cmd)) + -- Tests args with and without "--headless". + local function _test(args, expected) + -- XXX: clear v:shell_error, sigh... + fn.system({ n.nvim_prog, '-es', '+qall!' }) + assert(0 == eval('v:shell_error')) + local cmd = vim.list_extend({ unpack(n.nvim_argv) }, vim.list_extend({ '--headless' }, args)) + local output = fn.system(cmd) + assert(0 ~= eval('v:shell_error')) + -- TODO(justinmk): output not properly captured on Windows? + if is_os('win') then + return + end + matches(expected, output) + cmd = vim.list_extend({ unpack(n.nvim_argv) }, args) + matches(expected, fn.system(cmd)) + end + + _test({ '--listen' }, 'nvim.*: Argument missing after: "%-%-listen"') + _test({ '--listen2' }, 'nvim.*: Garbage after option argument: "%-%-listen2"') + _test({ '--listen', n.eval('v:servername') }, 'nvim.*: Failed to %-%-listen: ".* already .*"') + _test({ '--listen', '/' }, 'nvim.*: Failed to %-%-listen: ".*"') + _test( + { '--listen', 'https://example.com' }, + ('nvim.*: Failed to %%-%%-listen: "%s"'):format( + (is_os('mac') or is_os('win')) and 'unknown node or service' + or 'service not available for socket type' + ) + ) end) it('sets v:servername, overrides $NVIM_LISTEN_ADDRESS', function() local addr = (is_os('win') and [[\\.\pipe\Xtest-listen-pipe]] or './Xtest-listen-pipe') clear({ env = { NVIM_LISTEN_ADDRESS = './Xtest-env-pipe' }, args = { '--listen', addr } }) + eq('', eval('$NVIM_LISTEN_ADDRESS')) -- Cleared on startup. eq(addr, api.nvim_get_vvar('servername')) -- Address without slashes is a "name" which is appended to a generated path. #8519