Merge pull request #10316 from bfredl/cb_safety

luv callbacks: throw error on deferred methods instead of crashing

10 files changed, 103 insertions, 15 deletions

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()) {