Merge pull request #10316 from bfredl/cb_safety

luv callbacks: throw error on deferred methods instead of crashing

14 files changed, 222 insertions, 29 deletions

diff --git a/runtime/doc/if_lua.txt b/runtime/doc/if_lua.txt
index cbc19a63a2..6dcf3f9fa9 100644
--- a/runtime/doc/if_lua.txt
+++ b/runtime/doc/if_lua.txt
@@ -382,20 +382,26 @@ management.  Try this command to see available functions: >
 See http://docs.libuv.org for complete documentation.
 See https://github.com/luvit/luv/tree/master/examples for examples.
 
-Note: it is not safe to directly invoke the Nvim API from `vim.loop`
-callbacks.  This will crash: >
+						*E5560* *lua-loop-callbacks*
+Note: it is not allowed to directly invoke most of the Nvim API from `vim.loop`
+callbacks.  This will result in an error: >
 
     local timer = vim.loop.new_timer()
     timer:start(1000, 0, function()
-      vim.api.nvim_command('sleep 100m')  -- BROKEN, will crash.
+      vim.api.nvim_command('echomsg "test"')
     end)
 
-Instead wrap the API call with |vim.schedule()|. >
+The `vim.schedule_wrap` helper can be used to defer the callback until it
+is safe to execute API methods. >
 
     local timer = vim.loop.new_timer()
-    timer:start(1000, 0, function()
-      vim.schedule(function() vim.api.nvim_command('sleep 100m') end)
-    end)
+    timer:start(1000, 0, vim.schedule_wrap(function()
+      vim.api.nvim_command('echomsg "test"')
+    end))
+
+A subset of the API is available in direct luv callbacks ("fast" callbacks),
+most notably |nvim_get_mode()| and |nvim_input()|.
+
 
 Example: repeating timer
     1. Save this code to a file.
diff --git a/src/nvim/api/private/dispatch.h b/src/nvim/api/private/dispatch.h
index 39aabd708a..bad5a13934 100644
--- a/src/nvim/api/private/dispatch.h
+++ b/src/nvim/api/private/dispatch.h
@@ -11,8 +11,10 @@ typedef Object (*ApiDispatchWrapper)(uint64_t channel_id,
 /// functions of this type.
 typedef struct {
   ApiDispatchWrapper fn;
-  bool async;  // function is always safe to run immediately instead of being
-               // put in a request queue for handling when nvim waits for input.
+  bool fast;  // Function is safe to be executed immediately while running the
+              // uv loop (the loop is run very frequently due to breakcheck).
+              // If "fast" is false, the function is deferred, i e the call will
+              // be put in the event queue, for safe handling later.
 } MsgpackRpcRequestHandler;
 
 #ifdef INCLUDE_GENERATED_DECLARATIONS
diff --git a/src/nvim/api/vim.c b/src/nvim/api/vim.c
index 72eb68b650..8e5650633a 100644
--- a/src/nvim/api/vim.c
+++ b/src/nvim/api/vim.c
@@ -209,7 +209,7 @@ void nvim_feedkeys(String keys, String mode, Boolean escape_csi)
 /// @return Number of bytes actually written (can be fewer than
 ///         requested if the buffer becomes full).
 Integer nvim_input(String keys)
-  FUNC_API_SINCE(1) FUNC_API_ASYNC
+  FUNC_API_SINCE(1) FUNC_API_FAST
 {
   return (Integer)input_enqueue(keys);
 }
@@ -238,7 +238,7 @@ Integer nvim_input(String keys)
 /// @param[out] err Error details, if any
 void nvim_input_mouse(String button, String action, String modifier,
                       Integer grid, Integer row, Integer col, Error *err)
-  FUNC_API_SINCE(6) FUNC_API_ASYNC
+  FUNC_API_SINCE(6) FUNC_API_FAST
 {
   if (button.data == NULL || action.data == NULL) {
     goto error;
@@ -1256,7 +1256,7 @@ Dictionary nvim_get_color_map(void)
 ///
 /// @returns Dictionary { "mode": String, "blocking": Boolean }
 Dictionary nvim_get_mode(void)
-  FUNC_API_SINCE(2) FUNC_API_ASYNC
+  FUNC_API_SINCE(2) FUNC_API_FAST
 {
   Dictionary rv = ARRAY_DICT_INIT;
   char *modestr = get_mode();
@@ -1342,7 +1342,7 @@ Dictionary nvim_get_commands(Dictionary opts, Error *err)
 ///
 /// @returns 2-tuple [{channel-id}, {api-metadata}]
 Array nvim_get_api_info(uint64_t channel_id)
-  FUNC_API_SINCE(1) FUNC_API_ASYNC FUNC_API_REMOTE_ONLY
+  FUNC_API_SINCE(1) FUNC_API_FAST FUNC_API_REMOTE_ONLY
 {
   Array rv = ARRAY_DICT_INIT;
 
@@ -1652,7 +1652,7 @@ typedef kvec_withinit_t(ExprASTConvStackItem, 16) ExprASTConvStack;
 /// @param[out] err Error details, if any
 Dictionary nvim_parse_expression(String expr, String flags, Boolean highlight,
                                  Error *err)
-  FUNC_API_SINCE(4) FUNC_API_ASYNC
+  FUNC_API_SINCE(4) FUNC_API_FAST
 {
   int pflags = 0;
   for (size_t i = 0 ; i < flags.size ; i++) {
diff --git a/src/nvim/func_attr.h b/src/nvim/func_attr.h
index d3b600a40c..14b8158c7f 100644
--- a/src/nvim/func_attr.h
+++ b/src/nvim/func_attr.h
@@ -205,8 +205,8 @@
 #endif
 
 #ifdef DEFINE_FUNC_ATTRIBUTES
-/// Non-deferred API function.
-# define FUNC_API_ASYNC
+/// Fast (non-deferred) API function.
+# define FUNC_API_FAST
 /// Internal C function not exposed in the RPC API.
 # define FUNC_API_NOEXPORT
 /// API function not exposed in VimL/eval.
diff --git a/src/nvim/generators/c_grammar.lua b/src/nvim/generators/c_grammar.lua
index 40bdc3e30c..8d50290ccc 100644
--- a/src/nvim/generators/c_grammar.lua
+++ b/src/nvim/generators/c_grammar.lua
@@ -35,11 +35,11 @@ local c_params = Ct(c_void + c_param_list)
 local c_proto = Ct(
   Cg(c_type, 'return_type') * Cg(c_id, 'name') *
   fill * P('(') * fill * Cg(c_params, 'parameters') * fill * P(')') *
-  Cg(Cc(false), 'async') *
+  Cg(Cc(false), 'fast') *
   (fill * Cg((P('FUNC_API_SINCE(') * C(num ^ 1)) * P(')'), 'since') ^ -1) *
   (fill * Cg((P('FUNC_API_DEPRECATED_SINCE(') * C(num ^ 1)) * P(')'),
               'deprecated_since') ^ -1) *
-  (fill * Cg((P('FUNC_API_ASYNC') * Cc(true)), 'async') ^ -1) *
+  (fill * Cg((P('FUNC_API_FAST') * Cc(true)), 'fast') ^ -1) *
   (fill * Cg((P('FUNC_API_NOEXPORT') * Cc(true)), 'noexport') ^ -1) *
   (fill * Cg((P('FUNC_API_REMOTE_ONLY') * Cc(true)), 'remote_only') ^ -1) *
   (fill * Cg((P('FUNC_API_REMOTE_IMPL') * Cc(true)), 'remote_impl') ^ -1) *
diff --git a/src/nvim/generators/gen_api_dispatch.lua b/src/nvim/generators/gen_api_dispatch.lua
index f52d05a4a5..3ebcd2d1c4 100644
--- a/src/nvim/generators/gen_api_dispatch.lua
+++ b/src/nvim/generators/gen_api_dispatch.lua
@@ -309,7 +309,7 @@ for i = 1, #functions do
                '(String) {.data = "'..fn.name..'", '..
                '.size = sizeof("'..fn.name..'") - 1}, '..
                '(MsgpackRpcRequestHandler) {.fn = handle_'..  (fn.impl_name or fn.name)..
-               ', .async = '..tostring(fn.async)..'});\n')
+               ', .fast = '..tostring(fn.fast)..'});\n')
 
 end
 
@@ -349,6 +349,7 @@ output:write([[
 #include "nvim/api/private/defs.h"
 #include "nvim/api/private/helpers.h"
 #include "nvim/lua/converter.h"
+#include "nvim/lua/executor.h"
 ]])
 include_headers(output, headers)
 output:write('\n')
@@ -372,6 +373,14 @@ local function process_function(fn)
     binding=lua_c_function_name,
     api=fn.name
   }
+
+  if not fn.fast then
+    write_shifted_output(output, string.format([[
+    if (!nlua_is_deferred_safe(lstate)) {
+      return luaL_error(lstate, e_luv_api_disabled, "%s");
+    }
+    ]], fn.name))
+  end
   local cparams = ''
   local free_code = {}
   for j = #fn.parameters,1,-1 do
diff --git a/src/nvim/globals.h b/src/nvim/globals.h
index 0b5fafd7c0..139ffe2144 100644
--- a/src/nvim/globals.h
+++ b/src/nvim/globals.h
@@ -1061,6 +1061,9 @@ EXTERN char_u e_cmdmap_key[] INIT(=N_(
 EXTERN char_u e_api_error[] INIT(=N_(
     "E5555: API call: %s"));
 
+EXTERN char e_luv_api_disabled[] INIT(=N_(
+    "E5560: %s must not be called in a lua loop callback"));
+
 EXTERN char_u e_floatonly[] INIT(=N_(
     "E5601: Cannot close window, only floating window would remain"));
 EXTERN char_u e_floatexchange[] INIT(=N_(
diff --git a/src/nvim/lua/executor.c b/src/nvim/lua/executor.c
index ae872c1540..4fd4e4c4fa 100644
--- a/src/nvim/lua/executor.c
+++ b/src/nvim/lua/executor.c
@@ -33,6 +33,8 @@
 
 #include "luv/luv.h"
 
+static int in_fast_callback = 0;
+
 typedef struct {
   Error err;
   String lua_err_str;
@@ -110,6 +112,50 @@ static int nlua_stricmp(lua_State *const lstate) FUNC_ATTR_NONNULL_ALL
   return 1;
 }
 
+static void nlua_luv_error_event(void **argv)
+{
+  char *error = (char *)argv[0];
+  msg_ext_set_kind("lua_error");
+  emsgf_multiline("Error executing luv callback:\n%s", error);
+  xfree(error);
+}
+
+static int nlua_luv_cfpcall(lua_State *lstate, int nargs, int nresult,
+                            int flags)
+  FUNC_ATTR_NONNULL_ALL
+{
+  int retval;
+
+  // luv callbacks might be executed at any os_breakcheck/line_breakcheck
+  // call, so using the API directly here is not safe.
+  in_fast_callback++;
+
+  int top = lua_gettop(lstate);
+  int status = lua_pcall(lstate, nargs, nresult, 0);
+  if (status) {
+    if (status == LUA_ERRMEM && !(flags & LUVF_CALLBACK_NOEXIT)) {
+      // consider out of memory errors unrecoverable, just like xmalloc()
+      mch_errmsg(e_outofmem);
+      mch_errmsg("\n");
+      preserve_exit();
+    }
+    const char *error = lua_tostring(lstate, -1);
+
+    multiqueue_put(main_loop.events, nlua_luv_error_event,
+                   1, xstrdup(error));
+    lua_pop(lstate, 1);  // error mesage
+    retval = -status;
+  } else {  // LUA_OK
+    if (nresult == LUA_MULTRET) {
+      nresult = lua_gettop(lstate) - top + nargs + 1;
+    }
+    retval = nresult;
+  }
+
+  in_fast_callback--;
+  return retval;
+}
+
 static void nlua_schedule_event(void **argv)
 {
   LuaRef cb = (LuaRef)(ptrdiff_t)argv[0];
@@ -180,8 +226,18 @@ static int nlua_state_init(lua_State *const lstate) FUNC_ATTR_NONNULL_ALL
 
   // vim.loop
   luv_set_loop(lstate, &main_loop.uv);
+  luv_set_callback(lstate, nlua_luv_cfpcall);
   luaopen_luv(lstate);
-  lua_setfield(lstate, -2, "loop");
+  lua_pushvalue(lstate, -1);
+  lua_setfield(lstate, -3, "loop");
+
+  // package.loaded.luv = vim.loop
+  // otherwise luv will be reinitialized when require'luv'
+  lua_getglobal(lstate, "package");
+  lua_getfield(lstate, -1, "loaded");
+  lua_pushvalue(lstate, -3);
+  lua_setfield(lstate, -2, "luv");
+  lua_pop(lstate, 3);
 
   lua_setglobal(lstate, "vim");
   return 0;
@@ -546,6 +602,13 @@ Object executor_exec_lua_cb(LuaRef ref, const char *name, Array args,
   }
 }
 
+/// check if the current execution context is safe for calling deferred API
+/// methods. Luv callbacks are unsafe as they are called inside the uv loop.
+bool nlua_is_deferred_safe(lua_State *lstate)
+{
+  return in_fast_callback == 0;
+}
+
 /// Run lua string
 ///
 /// Used for :lua.
diff --git a/src/nvim/lua/vim.lua b/src/nvim/lua/vim.lua
index 848bccaae6..46c96b455f 100644
--- a/src/nvim/lua/vim.lua
+++ b/src/nvim/lua/vim.lua
@@ -172,11 +172,22 @@ local function __index(t, key)
   end
 end
 
+--- Defers the wrapped callback until when the nvim API is safe to call.
+---
+--- See |vim-loop-callbacks|
+local function schedule_wrap(cb)
+  return (function (...)
+    local args = {...}
+    vim.schedule(function() cb(unpack(args)) end)
+  end)
+end
+
 local module = {
   _update_package_paths = _update_package_paths,
   _os_proc_children = _os_proc_children,
   _os_proc_info = _os_proc_info,
   _system = _system,
+  schedule_wrap = schedule_wrap,
 }
 
 setmetatable(module, {
diff --git a/src/nvim/map.c b/src/nvim/map.c
index 90da27cf9f..cdade5ee71 100644
--- a/src/nvim/map.c
+++ b/src/nvim/map.c
@@ -179,7 +179,7 @@ MAP_IMPL(cstr_t, ptr_t, DEFAULT_INITIALIZER)
 MAP_IMPL(ptr_t, ptr_t, DEFAULT_INITIALIZER)
 MAP_IMPL(uint64_t, ptr_t, DEFAULT_INITIALIZER)
 MAP_IMPL(handle_T, ptr_t, DEFAULT_INITIALIZER)
-#define MSGPACK_HANDLER_INITIALIZER { .fn = NULL, .async = false }
+#define MSGPACK_HANDLER_INITIALIZER { .fn = NULL, .fast = false }
 MAP_IMPL(String, MsgpackRpcRequestHandler, MSGPACK_HANDLER_INITIALIZER)
 #define KVEC_INITIALIZER { .size = 0, .capacity = 0, .items = NULL }
 MAP_IMPL(HlEntry, int, DEFAULT_INITIALIZER)
diff --git a/src/nvim/msgpack_rpc/channel.c b/src/nvim/msgpack_rpc/channel.c
index c1ad3bd829..81c9f1e3f4 100644
--- a/src/nvim/msgpack_rpc/channel.c
+++ b/src/nvim/msgpack_rpc/channel.c
@@ -344,7 +344,7 @@ static void handle_request(Channel *channel, msgpack_object *request)
   evdata->args = args;
   evdata->request_id = request_id;
   channel_incref(channel);
-  if (handler.async) {
+  if (handler.fast) {
     bool is_get_mode = handler.fn == handle_nvim_get_mode;
 
     if (is_get_mode && !input_blocking()) {
diff --git a/test/functional/helpers.lua b/test/functional/helpers.lua
index e7a93238e7..95bff21ff5 100644
--- a/test/functional/helpers.lua
+++ b/test/functional/helpers.lua
@@ -686,6 +686,10 @@ local curbufmeths = create_callindex(curbuf)
 local curwinmeths = create_callindex(curwin)
 local curtabmeths = create_callindex(curtab)
 
+local function exec_lua(code, ...)
+  return meths.execute_lua(code, {...})
+end
+
 local function redir_exec(cmd)
   meths.set_var('__redir_exec_cmd', cmd)
   nvim_command([[
@@ -778,6 +782,7 @@ local module = {
   curwinmeths = curwinmeths,
   eval = nvim_eval,
   exc_exec = exc_exec,
+  exec_lua = exec_lua,
   expect = expect,
   expect_any = expect_any,
   expect_msg_seq = expect_msg_seq,
diff --git a/test/functional/lua/loop_spec.lua b/test/functional/lua/loop_spec.lua
index 8cc54e8c13..cd3b935e1a 100644
--- a/test/functional/lua/loop_spec.lua
+++ b/test/functional/lua/loop_spec.lua
@@ -1,11 +1,15 @@
 -- Test suite for testing interactions with API bindings
 local helpers = require('test.functional.helpers')(after_each)
+local Screen = require('test.functional.ui.screen')
 local funcs = helpers.funcs
 local meths = helpers.meths
 local clear = helpers.clear
 local sleep = helpers.sleep
+local feed = helpers.feed
 local eq = helpers.eq
+local eval = helpers.eval
 local matches = helpers.matches
+local exec_lua = helpers.exec_lua
 
 before_each(clear)
 
@@ -17,7 +21,7 @@ describe('vim.loop', function()
   end)
 
   it('timer', function()
-    meths.execute_lua('vim.api.nvim_set_var("coroutine_cnt", 0)', {})
+    exec_lua('vim.api.nvim_set_var("coroutine_cnt", 0)', {})
 
     local code=[[
       local loop = vim.loop
@@ -27,14 +31,14 @@ describe('vim.loop', function()
         local this = coroutine.running()
         assert(this)
         local timer = loop.new_timer()
-        timer:start(ms, 0, function ()
+        timer:start(ms, 0, vim.schedule_wrap(function ()
           timer:close()
           touch = touch + 1
           coroutine.resume(this)
           touch = touch + 1
           assert(touch==3)
           vim.api.nvim_set_var("coroutine_cnt_1", touch)
-        end)
+        end))
         coroutine.yield()
         touch = touch + 1
         return touch
@@ -47,9 +51,99 @@ describe('vim.loop', function()
     ]]
 
     eq(0, meths.get_var('coroutine_cnt'))
-    meths.execute_lua(code, {})
-    sleep(20)
+    exec_lua(code)
+    sleep(50)
     eq(2, meths.get_var('coroutine_cnt'))
     eq(3, meths.get_var('coroutine_cnt_1'))
   end)
+
+  it('is API safe', function()
+    local screen = Screen.new(50,10)
+    screen:attach()
+    screen:set_default_attr_ids({
+      [1] = {bold = true, foreground = Screen.colors.Blue1},
+      [2] = {bold = true, reverse = true},
+      [3] = {foreground = Screen.colors.Grey100, background = Screen.colors.Red},
+      [4] = {bold = true, foreground = Screen.colors.SeaGreen4},
+      [5] = {bold = true},
+    })
+
+    -- deferred API functions are disabled, as their safety can't be guaranteed
+    exec_lua([[
+      local timer = vim.loop.new_timer()
+      timer:start(20, 0, function ()
+        timer:close()
+        vim.api.nvim_set_var("valid", true)
+        vim.api.nvim_command("echomsg 'howdy'")
+      end)
+    ]])
+
+    screen:expect([[
+                                                        |
+      {1:~                                                 }|
+      {1:~                                                 }|
+      {1:~                                                 }|
+      {1:~                                                 }|
+      {2:                                                  }|
+      {3:Error executing luv callback:}                     |
+      {3:[string "<nvim>"]:4: E5560: nvim_set_var must not }|
+      {3:be called in a lua loop callback}                  |
+      {4:Press ENTER or type command to continue}^           |
+    ]])
+    feed('<cr>')
+    eq(false, eval("get(g:, 'valid', v:false)"))
+
+    -- callbacks can be scheduled to be executed in the main event loop
+    -- where the entire API is available
+    exec_lua([[
+      local timer = vim.loop.new_timer()
+      timer:start(20, 0, vim.schedule_wrap(function ()
+        timer:close()
+        vim.api.nvim_set_var("valid", true)
+        vim.api.nvim_command("echomsg 'howdy'")
+      end))
+    ]])
+
+    screen:expect([[
+      ^                                                  |
+      {1:~                                                 }|
+      {1:~                                                 }|
+      {1:~                                                 }|
+      {1:~                                                 }|
+      {1:~                                                 }|
+      {1:~                                                 }|
+      {1:~                                                 }|
+      {1:~                                                 }|
+      howdy                                             |
+    ]])
+    eq(true, eval("get(g:, 'valid', v:false)"))
+
+    -- fast (not deferred) API functions are allowed to be called directly
+    exec_lua([[
+      local timer = vim.loop.new_timer()
+      timer:start(20, 0, function ()
+        timer:close()
+        -- input is queued for processing after the callback returns
+        vim.api.nvim_input("isneaky")
+        _G.mode = vim.api.nvim_get_mode()
+      end)
+    ]])
+    screen:expect([[
+      sneaky^                                            |
+      {1:~                                                 }|
+      {1:~                                                 }|
+      {1:~                                                 }|
+      {1:~                                                 }|
+      {1:~                                                 }|
+      {1:~                                                 }|
+      {1:~                                                 }|
+      {1:~                                                 }|
+      {5:-- INSERT --}                                      |
+    ]])
+    eq({blocking=false, mode='n'}, exec_lua("return _G.mode"))
+  end)
+
+  it("is equal to require('luv')", function()
+    eq(true, exec_lua("return vim.loop == require('luv')"))
+  end)
 end)
diff --git a/third-party/CMakeLists.txt b/third-party/CMakeLists.txt
index d789e48299..5db8e10a33 100644
--- a/third-party/CMakeLists.txt
+++ b/third-party/CMakeLists.txt
@@ -154,9 +154,9 @@ set(LIBTERMKEY_SHA256 cecbf737f35d18f433c8d7864f63c0f878af41f8bd0255a3ebb16010dc
 set(LIBVTERM_URL https://github.com/neovim/libvterm/archive/b45b648cab73f9667bde7c0c6045b285e22b3ecd.tar.gz)
 set(LIBVTERM_SHA256 37cc123deff29327efa654358c2ebaaf8589da03754ca5adb8ec47be386a0433)
 
-set(LUV_VERSION 1.29.1-2)
+set(LUV_VERSION 1.30.0-0)
 set(LUV_URL https://github.com/luvit/luv/archive/${LUV_VERSION}.tar.gz)
-set(LUV_SHA256 e75d8fd2a14433bb798900a71e45318b3c0b8c2ef2c1c43593482ce95b4999e2)
+set(LUV_SHA256 6e468fa17bf222ca8ce0bfffdbdf947fc897da48643a12955db92f80e2c852f5)
 
 set(LUA_COMPAT53_URL https://github.com/keplerproject/lua-compat-5.3/archive/v0.7.tar.gz)
 set(LUA_COMPAT53_SHA256 bec3a23114a3d9b3218038309657f0f506ad10dfbc03bb54e91da7e5ffdba0a2)