functests: Test for error conditions

During testing found the following bugs: 1. msgpack-gen.lua script is completely unprepared for Float values either in return type or in arguments. Specifically: 1. At the time of writing relevant code FLOAT_OBJ did not exist as well as FLOATING_OBJ, but it would be used by msgpack-gen.lua should return type be Float. I added FLOATING_OBJ macros later because did not know that msgpack-gen.lua uses these _OBJ macros, otherwise it would be FLOAT_OBJ. 2. msgpack-gen.lua should use .data.floating in place of .data.float. But it did not expect that .data subattribute may have name different from lowercased type name. 2. vim_replace_termcodes returned its argument as-is if it receives an empty string (as well as _vim_id*() functions did). But if something in returned argument lives in an allocated memory such action will cause double free: once when freeing arguments, then when freeing return value. It did not cause problems yet because msgpack bindings return empty string as {NULL, 0} and nothing was actually allocated. 3. New code in msgpack-gen.lua popped arguments in reversed order, making lua bindings’ signatures be different from API ones.
2025-09-24 20:18:32 +00:00 · 2016-07-16 02:26:04 +03:00
parent 7a013e93e0
commit ba2f615cd4
8 changed files with 138 additions and 50 deletions
--- a/scripts/genmsgpack.lua
+++ b/scripts/genmsgpack.lua
@@ -216,6 +216,14 @@ local function real_type(type)
  return rv
 end

+local function attr_name(rt)
+  if rt == 'Float' then
+    return 'floating'
+  else
+    return rt:lower()
+  end
+end
+
 -- start the handler functions. Visit each function metadata to build the
 -- handler function with code generated for validating arguments and calling to
 -- the real API.
@@ -253,7 +261,7 @@ for i = 1, #functions do
          output:write('\n    '..converted..' = (handle_T)args.items['..(j - 1)..'].data.integer;')
        else
          output:write('\n  if (args.items['..(j - 1)..'].type == kObjectType'..rt..') {')
-          output:write('\n    '..converted..' = args.items['..(j - 1)..'].data.'..rt:lower()..';')
+          output:write('\n    '..converted..' = args.items['..(j - 1)..'].data.'..attr_name(rt)..';')
        end
        if rt:match('^Buffer$') or rt:match('^Window$') or rt:match('^Tabpage$') or rt:match('^Boolean$') then
          -- accept nonnegative integers for Booleans, Buffers, Windows and Tabpages
@@ -368,6 +376,7 @@ output:write([[

 #include "nvim/func_attr.h"
 #include "nvim/api/private/defs.h"
+#include "nvim/api/private/helpers.h"
 #include "nvim/viml/executor/converter.h"
 ]])
 include_headers(output, headers)
@@ -382,27 +391,35 @@ local function process_function(fn)
  static int %s(lua_State *lstate)
  {
    Error err = {.set = false};
-  ]], lua_c_function_name))
+    if (lua_gettop(lstate) != %i) {
+      api_set_error(&err, Validation, "Expected %i argument%s");
+      lua_pushstring(lstate, err.msg);
+      return lua_error(lstate);
+    }
+  ]], lua_c_function_name, #fn.parameters, #fn.parameters,
+      (#fn.parameters == 1) and '' or 's'))
  lua_c_functions[#lua_c_functions + 1] = {
    binding=lua_c_function_name,
    api=fn.name
  }
-  cparams = ''
-  for j, param in ipairs(fn.parameters) do
+  local cparams = ''
+  local free_code = {}
+  for j = #fn.parameters,1,-1 do
+    param = fn.parameters[j]
    cparam = string.format('arg%u', j)
-    if param[1]:match('^ArrayOf') then
-      param_type = 'Array'
-    else
-      param_type = param[1]
-    end
+    param_type = real_type(param[1])
+    lc_param_type = param_type:lower()
    write_shifted_output(output, string.format([[
-    %s %s = nlua_pop_%s(lstate, &err);
+    const %s %s = nlua_pop_%s(lstate, &err);
+
    if (err.set) {
+      %s
      lua_pushstring(lstate, err.msg);
      return lua_error(lstate);
    }
-    ]], param[1], cparam, param_type))
-    cparams = cparams .. cparam .. ', '
+    ]], param[1], cparam, param_type, table.concat(free_code, '\n      ')))
+    free_code[#free_code + 1] = ('api_free_%s(%s);'):format(lc_param_type, cparam)
+    cparams = cparam .. ', ' .. cparams
  end
  if fn.receives_channel_id then
    cparams = 'INTERNAL_CALL, ' .. cparams
@@ -412,7 +429,7 @@ local function process_function(fn)
  else
    cparams = cparams:gsub(', $', '')
  end
-  local name = fn.impl_name or fn.name
+  free_at_exit_code = table.concat(free_code, '\n    ')
  if fn.return_type ~= 'void' then
    if fn.return_type:match('^ArrayOf') then
      return_type = 'Array'
@@ -420,23 +437,27 @@ local function process_function(fn)
      return_type = fn.return_type
    end
    write_shifted_output(output, string.format([[
-    %s ret = %s(%s);
+    const %s ret = %s(%s);
+    %s
    if (err.set) {
      lua_pushstring(lstate, err.msg);
      return lua_error(lstate);
    }
    nlua_push_%s(lstate, ret);
+    api_free_%s(ret);
    return 1;
-    ]], fn.return_type, name, cparams, return_type))
+    ]], fn.return_type, fn.name, cparams, free_at_exit_code, return_type,
+        return_type:lower()))
  else
    write_shifted_output(output, string.format([[
    %s(%s);
+    %s
    if (err.set) {
      lua_pushstring(lstate, err.msg);
      return lua_error(lstate);
    }
    return 0;
-    ]], name, cparams))
+    ]], fn.name, cparams, free_at_exit_code))
  end
  write_shifted_output(output, [[
  }
@@ -457,11 +478,12 @@ void nlua_add_api_functions(lua_State *lstate)
 ]], #lua_c_functions))
 for _, func in ipairs(lua_c_functions) do
  output:write(string.format([[
+
  lua_pushcfunction(lstate, &%s);
-  lua_setfield(lstate, -2, "%s");
-  ]], func.binding, func.api))
+  lua_setfield(lstate, -2, "%s");]], func.binding, func.api))
 end
 output:write([[
+
  lua_setfield(lstate, -2, "api");
 }
 ]])
--- a/src/nvim/api/private/helpers.c
+++ b/src/nvim/api/private/helpers.c
@@ -351,7 +351,7 @@ void set_option_to(void *to, int type, String name, Object value, Error *err)
 #define TYPVAL_ENCODE_CONV_UNSIGNED_NUMBER TYPVAL_ENCODE_CONV_NUMBER

 #define TYPVAL_ENCODE_CONV_FLOAT(tv, flt) \
-    kv_push(edata->stack, FLOATING_OBJ((Float)(flt)))
+    kv_push(edata->stack, FLOAT_OBJ((Float)(flt)))

 #define TYPVAL_ENCODE_CONV_STRING(tv, str, len) \
    do { \
--- a/src/nvim/api/private/helpers.h
+++ b/src/nvim/api/private/helpers.h
@@ -27,7 +27,7 @@
    .type = kObjectTypeInteger, \
    .data.integer = i })

-#define FLOATING_OBJ(f) ((Object) { \
+#define FLOAT_OBJ(f) ((Object) { \
    .type = kObjectTypeFloat, \
    .data.floating = f })

--- a/src/nvim/api/vim.c
+++ b/src/nvim/api/vim.c
@@ -139,7 +139,7 @@ String nvim_replace_termcodes(String str, Boolean from_part, Boolean do_lt,
 {
  if (str.size == 0) {
    // Empty string
-    return str;
+    return (String) { .data = NULL, .size = 0 };
  }

  char *ptr = NULL;
@@ -843,7 +843,7 @@ static void write_msg(String message, bool to_err)
 /// @return its argument.
 Object _vim_id(Object obj)
 {
-  return obj;
+  return copy_object(obj);
 }

 /// Returns array given as argument
@@ -856,7 +856,7 @@ Object _vim_id(Object obj)
 /// @return its argument.
 Array _vim_id_array(Array arr)
 {
-  return arr;
+  return copy_object(ARRAY_OBJ(arr)).data.array;
 }

 /// Returns dictionary given as argument
@@ -869,5 +869,18 @@ Array _vim_id_array(Array arr)
 /// @return its argument.
 Dictionary _vim_id_dictionary(Dictionary dct)
 {
-  return dct;
+  return copy_object(DICTIONARY_OBJ(dct)).data.dictionary;
+}
+
+/// Returns floating-point value given as argument
+///
+/// This API function is used for testing. One should not rely on its presence
+/// in plugins.
+///
+/// @param[in]  flt  Value to return.
+///
+/// @return its argument.
+Float _vim_id_float(Float flt)
+{
+  return flt;
 }
--- a/src/nvim/msgpack_rpc/helpers.c
+++ b/src/nvim/msgpack_rpc/helpers.c
@@ -117,7 +117,7 @@ bool msgpack_rpc_to_object(const msgpack_object *const obj, Object *const arg)
      case MSGPACK_OBJECT_FLOAT: {
        STATIC_ASSERT(sizeof(Float) == sizeof(cur.mobj->via.f64),
                      "Msgpack floating-point size does not match API integer");
-        *cur.aobj = FLOATING_OBJ(cur.mobj->via.f64);
+        *cur.aobj = FLOAT_OBJ(cur.mobj->via.f64);
        break;
      }
 #define STR_CASE(type, attr, obj, dest, conv) \
--- a/src/nvim/viml/executor/converter.c
+++ b/src/nvim/viml/executor/converter.c
@@ -724,16 +724,15 @@ void nlua_push_Object(lua_State *lstate, const Object obj)
 String nlua_pop_String(lua_State *lstate, Error *err)
  FUNC_ATTR_NONNULL_ALL FUNC_ATTR_WARN_UNUSED_RESULT
 {
+  if (lua_type(lstate, -1) != LUA_TSTRING) {
+    lua_pop(lstate, 1);
+    api_set_error(err, Validation, "Expected lua string");
+    return (String) { .size = 0, .data = NULL };
+  }
  String ret;

  ret.data = (char *)lua_tolstring(lstate, -1, &(ret.size));
-
-  if (ret.data == NULL) {
-    lua_pop(lstate, 1);
-    set_api_error("Expected lua string", err);
-    return (String) { .size = 0, .data = NULL };
-  }
-
+  assert(ret.data != NULL);
  ret.data = xmemdupz(ret.data, ret.size);
  lua_pop(lstate, 1);

@@ -746,17 +745,19 @@ String nlua_pop_String(lua_State *lstate, Error *err)
 Integer nlua_pop_Integer(lua_State *lstate, Error *err)
  FUNC_ATTR_NONNULL_ALL FUNC_ATTR_WARN_UNUSED_RESULT
 {
-  Integer ret = 0;
-
-  if (!lua_isnumber(lstate, -1)) {
+  if (lua_type(lstate, -1) != LUA_TNUMBER) {
    lua_pop(lstate, 1);
-    set_api_error("Expected lua integer", err);
-    return ret;
+    api_set_error(err, Validation, "Expected lua number");
+    return 0;
  }
-  ret = (Integer)lua_tonumber(lstate, -1);
+  const lua_Number n = lua_tonumber(lstate, -1);
  lua_pop(lstate, 1);
-
-  return ret;
+  if (n > (lua_Number)API_INTEGER_MAX || n < (lua_Number)API_INTEGER_MIN
+      || ((lua_Number)((Integer)n)) != n) {
+    api_set_error(err, Exception, "Number is not integral");
+    return 0;
+  }
+  return (Integer)n;
 }

 /// Convert lua value to boolean
@@ -765,7 +766,7 @@ Integer nlua_pop_Integer(lua_State *lstate, Error *err)
 Boolean nlua_pop_Boolean(lua_State *lstate, Error *err)
  FUNC_ATTR_NONNULL_ALL FUNC_ATTR_WARN_UNUSED_RESULT
 {
-  Boolean ret = lua_toboolean(lstate, -1);
+  const Boolean ret = lua_toboolean(lstate, -1);
  lua_pop(lstate, 1);
  return ret;
 }
@@ -784,7 +785,7 @@ static inline LuaTableProps nlua_check_type(lua_State *const lstate,
 {
  if (lua_type(lstate, -1) != LUA_TTABLE) {
    if (err) {
-      set_api_error("Expected lua table", err);
+      api_set_error(err, Validation, "Expected lua table");
    }
    return (LuaTableProps) { .type = kObjectTypeNil };
  }
@@ -797,7 +798,7 @@ static inline LuaTableProps nlua_check_type(lua_State *const lstate,

  if (table_props.type != type) {
    if (err) {
-      set_api_error("Unexpected type", err);
+      api_set_error(err, Validation, "Unexpected type");
    }
  }

@@ -1050,7 +1051,7 @@ Object nlua_pop_Object(lua_State *const lstate, Error *const err)
        const lua_Number n = lua_tonumber(lstate, -1);
        if (n > (lua_Number)API_INTEGER_MAX || n < (lua_Number)API_INTEGER_MIN
            || ((lua_Number)((Integer)n)) != n) {
-          *cur.obj = FLOATING_OBJ((Float)n);
+          *cur.obj = FLOAT_OBJ((Float)n);
        } else {
          *cur.obj = INTEGER_OBJ((Integer)n);
        }
@@ -1094,7 +1095,7 @@ Object nlua_pop_Object(lua_State *const lstate, Error *const err)
            break;
          }
          case kObjectTypeFloat: {
-            *cur.obj = FLOATING_OBJ((Float)table_props.val);
+            *cur.obj = FLOAT_OBJ((Float)table_props.val);
            break;
          }
          case kObjectTypeNil: {
--- a/test/functional/api/vim_spec.lua
+++ b/test/functional/api/vim_spec.lua
@@ -219,6 +219,17 @@ describe('api', function()
      eq('\128\253\44', helpers.nvim('replace_termcodes',
                                     '<LeftMouse>', true, true, true))
    end)
+
+    it('does not crash when transforming an empty string', function()
+      -- Actually does not test anything, because current code will use NULL for
+      -- an empty string.
+      --
+      -- Problem here is that if String argument has .data in allocated memory
+      -- then `return str` in vim_replace_termcodes body will make Neovim free
+      -- `str.data` twice: once when freeing arguments, then when freeing return
+      -- value.
+      eq('', meths.replace_termcodes('', true, true, true))
+    end)
  end)

  describe('nvim_feedkeys', function()
--- a/test/functional/lua_spec.lua
+++ b/test/functional/lua_spec.lua
@@ -276,12 +276,53 @@ describe('luaeval() function', function()
  end)

  it('errors out correctly when working with API', function()
-    eq(0, exc_exec([[call luaeval("vim.api.id")]]))
+    -- Conversion errors
+    eq('Vim(call):E5108: Error while calling lua chunk for luaeval(): Cannot convert given lua type',
+       exc_exec([[call luaeval("vim.api._vim_id(vim.api._vim_id)")]]))
+    eq('Vim(call):E5108: Error while calling lua chunk for luaeval(): Cannot convert given lua table',
+       exc_exec([[call luaeval("vim.api._vim_id({1, foo=42})")]]))
+    eq('Vim(call):E5108: Error while calling lua chunk for luaeval(): Cannot convert given lua type',
+       exc_exec([[call luaeval("vim.api._vim_id({42, vim.api._vim_id})")]]))
+    -- Errors in number of arguments
+    eq('Vim(call):E5108: Error while calling lua chunk for luaeval(): Expected 1 argument',
+       exc_exec([[call luaeval("vim.api._vim_id()")]]))
+    eq('Vim(call):E5108: Error while calling lua chunk for luaeval(): Expected 1 argument',
+       exc_exec([[call luaeval("vim.api._vim_id(1, 2)")]]))
+    eq('Vim(call):E5108: Error while calling lua chunk for luaeval(): Expected 2 arguments',
+       exc_exec([[call luaeval("vim.api.vim_set_var(1, 2, 3)")]]))
+    -- Error in argument types
+    eq('Vim(call):E5108: Error while calling lua chunk for luaeval(): Expected lua string',
+       exc_exec([[call luaeval("vim.api.vim_set_var(1, 2)")]]))
+
+    eq('Vim(call):E5108: Error while calling lua chunk for luaeval(): Expected lua number',
+       exc_exec([[call luaeval("vim.api.buffer_get_line(0, 'test')")]]))
+    eq('Vim(call):E5108: Error while calling lua chunk for luaeval(): Number is not integral',
+       exc_exec([[call luaeval("vim.api.buffer_get_line(0, 1.5)")]]))
+
+    eq('Vim(call):E5108: Error while calling lua chunk for luaeval(): Expected lua table',
+       exc_exec([[call luaeval("vim.api._vim_id_float('test')")]]))
+    eq('Vim(call):E5108: Error while calling lua chunk for luaeval(): Unexpected type',
+       exc_exec([[call luaeval("vim.api._vim_id_float({[vim.type_idx]=vim.types.dictionary})")]]))
+
+    eq('Vim(call):E5108: Error while calling lua chunk for luaeval(): Expected lua table',
+       exc_exec([[call luaeval("vim.api._vim_id_array(1)")]]))
+    eq('Vim(call):E5108: Error while calling lua chunk for luaeval(): Unexpected type',
+       exc_exec([[call luaeval("vim.api._vim_id_array({[vim.type_idx]=vim.types.dictionary})")]]))
+
+    eq('Vim(call):E5108: Error while calling lua chunk for luaeval(): Expected lua table',
+       exc_exec([[call luaeval("vim.api._vim_id_dictionary(1)")]]))
+    eq('Vim(call):E5108: Error while calling lua chunk for luaeval(): Unexpected type',
+       exc_exec([[call luaeval("vim.api._vim_id_dictionary({[vim.type_idx]=vim.types.array})")]]))
+    -- TODO: check for errors with Tabpage argument
+    -- TODO: check for errors with Window argument
+    -- TODO: check for errors with Buffer argument
+  end)
+
+  it('accepts any value as API Boolean', function()
+    eq('', funcs.luaeval('vim.api.vim_replace_termcodes("", vim, false, nil)'))
+    eq('', funcs.luaeval('vim.api.vim_replace_termcodes("", 0, 1.5, "test")'))
+    eq('', funcs.luaeval('vim.api.vim_replace_termcodes("", true, {}, {[vim.type_idx]=vim.types.array})'))
  end)

  -- TODO: check buffer/window/etc.
-  -- TODO: check what happens when it errors out on second list item
-  -- TODO: check what happens if API function receives wrong number of
-  -- arguments.
-  -- TODO: check what happens if API function receives wrong argument types.
 end)