Re: [Tarantool-patches] [tarantool-patches] Re: [PATCH] msgpackffi.decode can now be assigned to buf.rpos

["Maria Khaydich" <maria.khaydich@tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 26559 bytes --]



Thank you for the review. Proposed changes look reasonable to me so I’ve squashed your FIXUP commits and also added follow up patch as a separate commit. 
  
>Среда, 6 ноября 2019, 3:44 +03:00 от Alexander Turenko <alexander.turenko@tarantool.org>:
> 
>Note: The patch was changed: now msgpackffi.decode_unchecked() returns a
>pointer of the same cdata type as passed buffer (the same for its
>msgpackffi.decode() alias).
>eljashm/gh-3926-msgpackffi.decode-assignment-to-buf.rpos branch was
>updated, but the email thread was not. I added the new patch below to
>make the discussion consistent.
>
>The review contains two parts: a question about the API and proposed
>changes for code and tests.
>
>API
>---
>
>I digged a bit into the history of msgpack and msgpackffi API to
>understand use cases and decide whether we should always return
>cdata<char *> pointer despite of cdata type of a provided buffer or
>return cdata<[const] char *> depending on the passed buffer.
>
>msgpack module initially accepts only cdata<char *>, so no one bother
>with this question. Then it start to accept const pointers too, while
>the type of returned pointer remains the same: always cdata<char *>.
>
>msgpackffi always accepts both types of pointers, but always returns
>'unsigned char *'. I guess this was not intentional.
>
>I see no reason to don't change the behaviour for both modules to the
>proposed one: return the same pointer type as passed one.
>
>For ones who interested, my sources of info. Here the new API that
>allows to interact with buffers was introduced:
>
>https://github.com/tarantool/tarantool/commit/68c213975a31d0c05b1e5d23e6112417e315f995
>https://github.com/tarantool/tarantool/issues/2755
>
>It was needed to easier use ibuf ('buffer' module) here:
>
>https://github.com/tarantool/dump/commit/5b1e15e057aa4606fedc929aae51724705600638
>
>Here msgpack module starts to accept cdata<const char *> (don't sure
>what the reason was):
>
>https://github.com/tarantool/tarantool/commit/453fff2b215e0c2e2d4c5951fae711329eff7b48
>
>Code
>----
>
>I'll paste the original (updated as I wrote above) patch and changes I
>proposed below the email. I also pushed them onto
>Totktonada/gh-3926-msgpack-decode-retval-ctype branch to ease
>cherry-picking.
>
>Masha, please, look into proposed changes and squash 'FIXUP' commits
>into your patch (if you're agree with them).
>
>Aside of two fixups I made a follow up patch that changes msgpack
>behaviour is the same way as you changed msgpackffi. Please, look into
>it too and if you're agree add it on top of your branch too (but as a
>separate commit).
>
>Then, I think somebody else should look into the patchset. I propose to
>send it to Vlad (because he is the author of 453fff2b). Please, leave me
>in CC when will send the new version(s) of the patchset.
>
>WBR, Alexander Turenko.
>
>----
>
>(The original patch from
>eljashm/gh-3926-msgpackffi.decode-assignment-to-buf.rpos.)
>
>commit 5a8f4504c8f09ff8532dd29da85885bf70abd53b
>Author: Maria < marianneliash@gmail.com >
>Date: Thu Sep 12 16:55:53 2019 +0300
>
>    msgpackffi.decode can now be assigned to buf.rpos
>    
>    Function decode of module msgpackffi was passing
>    value of type const unsigned char * to a C function
>    that accepts arguments of type const char *.
>    
>    Closes #3926
>
>diff --git a/src/lua/msgpackffi.lua b/src/lua/msgpackffi.lua
>index 5331dc75f..34b22fbb2 100644
>--- a/src/lua/msgpackffi.lua
>+++ b/src/lua/msgpackffi.lua
>@@ -10,6 +10,7 @@ local uint16_ptr_t = ffi.typeof('uint16_t *')
> local uint32_ptr_t = ffi.typeof('uint32_t *')
> local uint64_ptr_t = ffi.typeof('uint64_t *')
> local const_char_ptr_t = ffi.typeof('const char *')
>+local char_ptr_t = ffi.typeof('char *')
> 
> ffi.cdef([[
> char *
>@@ -606,13 +607,14 @@ local function decode_unchecked(str, offset)
>         bufp[0] = buf + offset - 1
>         local r = decode_r(bufp)
>         return r, bufp[0] - buf + 1
>- elseif ffi.istype(const_char_ptr_t, str) then
>+ elseif ffi.istype(const_char_ptr_t, str) or
>+ ffi.istype(char_ptr_t, str) then
>         bufp[0] = str
>         local r = decode_r(bufp)
>- return r, bufp[0]
>+ return r, ffi.cast(ffi.typeof(str), bufp[0])
>     else
>         error("msgpackffi.decode_unchecked(str, offset) -> res, new_offset | "..
>- "msgpackffi.decode_unchecked(const char *buf) -> res, new_buf")
>+ "msgpackffi.decode_unchecked([const] char *buf) -> res, new_buf")
>     end
> end
> 
>diff --git a/test/app-tap/msgpackffi.test.lua b/test/app-tap/msgpackffi.test.lua
>index 36ac26b7e..c4276ec43 100755
>--- a/test/app-tap/msgpackffi.test.lua
>+++ b/test/app-tap/msgpackffi.test.lua
>@@ -4,6 +4,8 @@ package.path = "lua/?.lua;"..package.path
> 
> local tap = require('tap')
> local common = require('serializer_test')
>+local buffer = require('buffer')
>+local ffi = require('ffi')
> 
> local function is_map(s)
>     local b = string.byte(string.sub(s, 1, 1))
>@@ -35,6 +37,23 @@ local function test_offsets(test, s)
>     test:ok(not pcall(s.decode, dump, offset), "invalid offset")
> end
> 
>+local function test_types(test, s)
>+ test:plan(2)
>+ -- gh-3926: decode result cannot be assigned to buffer.rpos
>+ local encoded_data = s.encode(0)
>+ local len = encoded_data:len()
>+ local buf = buffer.ibuf()
>+ buf:reserve(len)
>+ local p = buf:alloc(len)
>+ ffi.copy(p, encoded_data, len)
>+ local _, new_buf = s.decode(ffi.cast(p, 'const char *'))
>+ test:iscdata(new_buf, 'const char *', 'cdata const char * type')
>+ _, new_buf = s.decode(p)
>+ test:iscdata(new_buf, 'char *', 'cdata char * type')
>+ buf.rpos = new_buf
>+ buf:recycle()
>+end
>+
> local function test_other(test, s)
>     test:plan(24)
>     local buf = string.char(0x93, 0x6e, 0xcb, 0x42, 0x2b, 0xed, 0x30, 0x47,
>@@ -117,7 +136,7 @@ end
> 
> tap.test("msgpackffi", function(test)
>     local serializer = require('msgpackffi')
>- test:plan(10)
>+ test:plan(11)
>     test:test("unsigned", common.test_unsigned, serializer)
>     test:test("signed", common.test_signed, serializer)
>     test:test("double", common.test_double, serializer)
>@@ -130,4 +149,5 @@ tap.test("msgpackffi", function(test)
>     --test:test("ucdata", common.test_ucdata, serializer)
>     test:test("offsets", test_offsets, serializer)
>     test:test("other", test_other, serializer)
>+ test:test("types", test_types, serializer)
> end)
>
>----
>
>commit 6c95d8c4c04e56288dfeacede0bc2468aa292a6d
>Author: Alexander Turenko < alexander.turenko@tarantool.org >
>Date: Wed Nov 6 01:36:17 2019 +0300
>
>    FIXUP: msgpackffi.decode can now be assigned to buf.rpos
>    
>    Added a note that ffi.istype() ignores the const qualifier and changed
>    the code to make it more clear.
>    
>    Saved an argument cdata type to a local variable to make the logic
>    'return a pointer of the same type as passed one' more clear.
>    
>    No behaviour changes.
>
>diff --git a/src/lua/msgpackffi.lua b/src/lua/msgpackffi.lua
>index 34b22fbb2..bb2747f83 100644
>--- a/src/lua/msgpackffi.lua
>+++ b/src/lua/msgpackffi.lua
>@@ -9,7 +9,6 @@ local uint8_ptr_t = ffi.typeof('uint8_t *')
> local uint16_ptr_t = ffi.typeof('uint16_t *')
> local uint32_ptr_t = ffi.typeof('uint32_t *')
> local uint64_ptr_t = ffi.typeof('uint64_t *')
>-local const_char_ptr_t = ffi.typeof('const char *')
> local char_ptr_t = ffi.typeof('char *')
> 
> ffi.cdef([[
>@@ -603,15 +602,17 @@ end
> local function decode_unchecked(str, offset)
>     if type(str) == "string" then
>         offset = check_offset(offset, #str)
>- local buf = ffi.cast(const_char_ptr_t, str)
>+ local buf = ffi.cast(char_ptr_t, str)
>         bufp[0] = buf + offset - 1
>         local r = decode_r(bufp)
>         return r, bufp[0] - buf + 1
>- elseif ffi.istype(const_char_ptr_t, str) or
>- ffi.istype(char_ptr_t, str) then
>+ elseif ffi.istype(char_ptr_t, str) then
>+ -- Note: ffi.istype() ignores the const qualifier, so both
>+ -- (char *) and (const char *) buffers are valid.
>         bufp[0] = str
>         local r = decode_r(bufp)
>- return r, ffi.cast(ffi.typeof(str), bufp[0])
>+ local ctype = ffi.typeof(str)
>+ return r, ffi.cast(ctype, bufp[0])
>     else
>         error("msgpackffi.decode_unchecked(str, offset) -> res, new_offset | "..
>               "msgpackffi.decode_unchecked([const] char *buf) -> res, new_buf")
>
>----
>
>commit f1dee13991d43c37d626e91601ab50e34ec2303b
>Author: Alexander Turenko < alexander.turenko@tarantool.org >
>Date: Wed Nov 6 01:42:28 2019 +0300
>
>    FIXUP: msgpackffi.decode can now be assigned to buf.rpos
>    
>    Rewritten the test case. There are several reasons to do so.
>    
>    First, the previous test case implementation uses test:iscdata() which
>    is not sufficient to verify whether the const qualifier is applied to a
>    cdata type, because of using ffi.istype() under hood, which ignores
>    'const'.
>    
>    Second, the previous test case was invalid, because of wrong order of
>    ffi.cast() arguments.
>    
>    Third, we don't need a real ibuf object, ffi.cast(ctype, 'a string') is
>    enough. This allows to simplify test cases.
>    
>    Fourth, the test case is rewritten in a declarative manner to reduce
>    code duplication. This also allows to expand it w/o many changes for
>    msgpack.decode() function in the following commit.
>    
>    I left only msgpackffi.decode_unchecked() cases. A bit context is needed
>    to describe why.
>    
>    msgpack.decode() accepts two arguments: a buffer and its size (or a
>    string and an offset, but it is out of scope here).
>    msgpack.decode_unchecked() accepts only a buffer (or, again, a string
>    and an offset) and does not verify whether we perform reads out of the
>    buffer bounds. See #2755 for the formal description of the API.
>    
>    msgpackffi module contains only msgpackffi.decode_unchecked().
>    msgpackffi.decode() is just alias for decode_unchecked: it does not
>    verify a buffer bounds. AFAIU the alias was added to unify
>    testing code for msgpack and msgpackffi modules.
>    
>    Our website has no documentation about msgpackffi. I don't sure whether
>    its API should be considered as public API. However if we'll make the
>    module public, we'll need to implement buffer bound check. And only then
>    state that the API is stable and can be used by users.
>    
>    If we don't consider msgpackffi.decode() as public function for now, it
>    make sense to left it untested when a testing code is not unified
>    between msgpack and msgpackffi modules.
>    
>    However I unified this testing code for both modules in the following
>    commit, so it will be tested anyway: but in the commit where it looks
>    reasonable.
>
>diff --git a/test/app-tap/msgpackffi.test.lua b/test/app-tap/msgpackffi.test.lua
>index c4276ec43..e64228e4d 100755
>--- a/test/app-tap/msgpackffi.test.lua
>+++ b/test/app-tap/msgpackffi.test.lua
>@@ -37,23 +37,6 @@ local function test_offsets(test, s)
>     test:ok(not pcall(s.decode, dump, offset), "invalid offset")
> end
> 
>-local function test_types(test, s)
>- test:plan(2)
>- -- gh-3926: decode result cannot be assigned to buffer.rpos
>- local encoded_data = s.encode(0)
>- local len = encoded_data:len()
>- local buf = buffer.ibuf()
>- buf:reserve(len)
>- local p = buf:alloc(len)
>- ffi.copy(p, encoded_data, len)
>- local _, new_buf = s.decode(ffi.cast(p, 'const char *'))
>- test:iscdata(new_buf, 'const char *', 'cdata const char * type')
>- _, new_buf = s.decode(p)
>- test:iscdata(new_buf, 'char *', 'cdata char * type')
>- buf.rpos = new_buf
>- buf:recycle()
>-end
>-
> local function test_other(test, s)
>     test:plan(24)
>     local buf = string.char(0x93, 0x6e, 0xcb, 0x42, 0x2b, 0xed, 0x30, 0x47,
>@@ -134,6 +117,45 @@ local function test_other(test, s)
>                  encode_max_depth = max_depth})
> end
> 
>+-- gh-3926: Ensure that a returned pointer has the same cdata type
>+-- as passed argument.
>+local function test_decode_buffer(test, s)
>+ local cases = {
>+ {
>+ 'decode_unchecked(cdata<const char *>)',
>+ data = ffi.cast('const char *', '\x93\x01\x02\x03'),
>+ exp_res = {1, 2, 3},
>+ exp_rewind = 4,
>+ },
>+ {
>+ 'decode_unchecked(cdata<char *>)',
>+ data = ffi.cast('char *', '\x93\x01\x02\x03'),
>+ exp_res = {1, 2, 3},
>+ exp_rewind = 4,
>+ },
>+ }
>+
>+ test:plan(#cases)
>+
>+ for _, case in ipairs(cases) do
>+ test:test(case[1], function(test)
>+ test:plan(4)
>+ local res, res_buf = s.decode_unchecked(case.data)
>+ test:is_deeply(res, case.exp_res, 'verify result')
>+ local rewind = res_buf - case.data
>+ test:is(rewind, case.exp_rewind, 'verify resulting buffer')
>+ -- test:iscdata() is not sufficient here, because it
>+ -- ignores 'const' qualifier (because of using
>+ -- ffi.istype()).
>+ test:is(type(res_buf), 'cdata', 'verify resulting buffer type')
>+ local data_ctype = tostring(ffi.typeof(case.data))
>+ local res_buf_ctype = tostring(ffi.typeof(res_buf))
>+ test:is(res_buf_ctype, data_ctype, 'verify resulting buffer ctype')
>+ end)
>+ end
>+end
>+
>+
> tap.test("msgpackffi", function(test)
>     local serializer = require('msgpackffi')
>     test:plan(11)
>@@ -149,5 +171,5 @@ tap.test("msgpackffi", function(test)
>     --test:test("ucdata", common.test_ucdata, serializer)
>     test:test("offsets", test_offsets, serializer)
>     test:test("other", test_other, serializer)
>- test:test("types", test_types, serializer)
>+ test:test("decode_buffer", test_decode_buffer, serializer)
> end)
>
>----
>
>commit cc50b71720a46ed947bbdf71d9e3d6c6ade1372a
>Author: Alexander Turenko < alexander.turenko@tarantool.org >
>Date: Wed Nov 6 02:05:03 2019 +0300
>
>    lua: don't modify pointer type in msgpack.decode*
>    
>    msgpackffi.decode_unchecked([const] char *) returns two values: a
>    decoded result and a new pointer within passed buffer. After #3926 a
>    cdata type of the returned pointer follows a type of passed buffer.
>    
>    This commit modifies behaviour of msgpack module in the same way. The
>    following functions now returns cdata<char *> or cdata<const char *>
>    depending of its argument:
>    
>    * msgpack.decode(cdata<[const] char *>, number)
>    * msgpack.decode_unchecked(cdata<[const] char *>)
>    * msgpack.decode_array_header(cdata<[const] char *>, number)
>    * msgpack.decode_map_header(cdata<[const] char *>, number)
>    
>    Follows up #3926.
>
>diff --git a/src/lua/msgpack.c b/src/lua/msgpack.c
>index ef315b692..edbc15b72 100644
>--- a/src/lua/msgpack.c
>+++ b/src/lua/msgpack.c
>@@ -370,7 +370,8 @@ static int
> lua_msgpack_decode_cdata(lua_State *L, bool check)
> {
>  const char *data;
>- if (luaL_checkconstchar(L, 1, &data) != 0) {
>+ uint32_t cdata_type;
>+ if (luaL_checkconstchar(L, 1, &data, &cdata_type) != 0) {
>  return luaL_error(L, "msgpack.decode: "
>  "a Lua string or 'char *' expected");
>  }
>@@ -386,7 +387,7 @@ lua_msgpack_decode_cdata(lua_State *L, bool check)
>  }
>  struct luaL_serializer *cfg = luaL_checkserializer(L);
>  luamp_decode(L, cfg, &data);
>- *(const char **)luaL_pushcdata(L, CTID_CHAR_PTR) = data;
>+ *(const char **)luaL_pushcdata(L, cdata_type) = data;
>  return 2;
> }
> 
>@@ -468,7 +469,8 @@ lua_ibuf_msgpack_decode(lua_State *L)
>  */
> static int
> verify_decode_header_args(lua_State *L, const char *func_name,
>- const char **data_p, ptrdiff_t *size_p)
>+ const char **data_p, uint32_t *cdata_type_p,
>+ ptrdiff_t *size_p)
> {
>  /* Verify arguments count. */
>  if (lua_gettop(L) != 2)
>@@ -476,7 +478,8 @@ verify_decode_header_args(lua_State *L, const char *func_name,
> 
>  /* Verify ptr type. */
>  const char *data;
>- if (luaL_checkconstchar(L, 1, &data) != 0)
>+ uint32_t cdata_type;
>+ if (luaL_checkconstchar(L, 1, &data, &cdata_type) != 0)
>  return luaL_error(L, "%s: 'char *' expected", func_name);
> 
>  /* Verify size type and value. */
>@@ -486,6 +489,7 @@ verify_decode_header_args(lua_State *L, const char *func_name,
> 
>  *data_p = data;
>  *size_p = size;
>+ *cdata_type_p = cdata_type;
> 
>  return 0;
> }
>@@ -499,8 +503,9 @@ lua_decode_array_header(lua_State *L)
> {
>  const char *func_name = "msgpack.decode_array_header";
>  const char *data;
>+ uint32_t cdata_type;
>  ptrdiff_t size;
>- verify_decode_header_args(L, func_name, &data, &size);
>+ verify_decode_header_args(L, func_name, &data, &cdata_type, &size);
> 
>  if (mp_typeof(*data) != MP_ARRAY)
>  return luaL_error(L, "%s: unexpected msgpack type", func_name);
>@@ -511,7 +516,7 @@ lua_decode_array_header(lua_State *L)
>  uint32_t len = mp_decode_array(&data);
> 
>  lua_pushinteger(L, len);
>- *(const char **) luaL_pushcdata(L, CTID_CHAR_PTR) = data;
>+ *(const char **) luaL_pushcdata(L, cdata_type) = data;
>  return 2;
> }
> 
>@@ -524,8 +529,9 @@ lua_decode_map_header(lua_State *L)
> {
>  const char *func_name = "msgpack.decode_map_header";
>  const char *data;
>+ uint32_t cdata_type;
>  ptrdiff_t size;
>- verify_decode_header_args(L, func_name, &data, &size);
>+ verify_decode_header_args(L, func_name, &data, &cdata_type, &size);
> 
>  if (mp_typeof(*data) != MP_MAP)
>  return luaL_error(L, "%s: unexpected msgpack type", func_name);
>@@ -536,7 +542,7 @@ lua_decode_map_header(lua_State *L)
>  uint32_t len = mp_decode_map(&data);
> 
>  lua_pushinteger(L, len);
>- *(const char **) luaL_pushcdata(L, CTID_CHAR_PTR) = data;
>+ *(const char **) luaL_pushcdata(L, cdata_type) = data;
>  return 2;
> }
> 
>diff --git a/src/lua/utils.c b/src/lua/utils.c
>index 7f7bf4776..8cb9920e0 100644
>--- a/src/lua/utils.c
>+++ b/src/lua/utils.c
>@@ -43,8 +43,8 @@ int luaL_array_metatable_ref = LUA_REFNIL;
> 
> static uint32_t CTID_STRUCT_IBUF;
> static uint32_t CTID_STRUCT_IBUF_PTR;
>-uint32_t CTID_CHAR_PTR;
>-uint32_t CTID_CONST_CHAR_PTR;
>+static uint32_t CTID_CHAR_PTR;
>+static uint32_t CTID_CONST_CHAR_PTR;
> uint32_t CTID_DECIMAL;
> 
> 
>@@ -1135,7 +1135,8 @@ luaL_checkibuf(struct lua_State *L, int idx)
> }
> 
> int
>-luaL_checkconstchar(struct lua_State *L, int idx, const char **res)
>+luaL_checkconstchar(struct lua_State *L, int idx, const char **res,
>+ uint32_t *cdata_type_p)
> {
>  if (lua_type(L, idx) != LUA_TCDATA)
>  return -1;
>@@ -1144,6 +1145,7 @@ luaL_checkconstchar(struct lua_State *L, int idx, const char **res)
>  if (cdata_type != CTID_CHAR_PTR && cdata_type != CTID_CONST_CHAR_PTR)
>  return -1;
>  *res = cdata != NULL ? *(const char **) cdata : NULL;
>+ *cdata_type_p = cdata_type;
>  return 0;
> }
> 
>diff --git a/src/lua/utils.h b/src/lua/utils.h
>index d9fb0704f..0b3672769 100644
>--- a/src/lua/utils.h
>+++ b/src/lua/utils.h
>@@ -71,9 +71,6 @@ struct ibuf;
> extern struct lua_State *tarantool_L;
> extern struct ibuf *tarantool_lua_ibuf;
> 
>-extern uint32_t CTID_CONST_CHAR_PTR;
>-extern uint32_t CTID_CHAR_PTR;
>-
> /** \cond public */
> 
> /**
>@@ -635,7 +632,8 @@ luaL_checkibuf(struct lua_State *L, int idx);
>  * char pointer.
>  */
> int
>-luaL_checkconstchar(struct lua_State *L, int idx, const char **res);
>+luaL_checkconstchar(struct lua_State *L, int idx, const char **res,
>+ uint32_t *cdata_type_p);
> 
> /* {{{ Helper functions to interact with a Lua iterator from C */
> 
>diff --git a/test/app-tap/lua/serializer_test.lua b/test/app-tap/lua/serializer_test.lua
>index 1609f768f..b3e9facc0 100644
>--- a/test/app-tap/lua/serializer_test.lua
>+++ b/test/app-tap/lua/serializer_test.lua
>@@ -433,6 +433,64 @@ local function test_depth(test, s)
>     s.cfg({encode_deep_as_nil = deep_as_nil, encode_max_depth = max_depth})
> end
> 
>+-- gh-3926: Ensure that a returned pointer has the same cdata type
>+-- as passed argument.
>+--
>+-- The test case is applicable for msgpack and msgpackffi.
>+local function test_decode_buffer(test, s)
>+ local cases = {
>+ {
>+ 'decode(cdata<const char *>, number)',
>+ func = s.decode,
>+ args = {ffi.cast('const char *', '\x93\x01\x02\x03'), 4},
>+ exp_res = {1, 2, 3},
>+ exp_rewind = 4,
>+ },
>+ {
>+ 'decode(cdata<char *>, number)',
>+ func = s.decode,
>+ args = {ffi.cast('char *', '\x93\x01\x02\x03'), 4},
>+ exp_res = {1, 2, 3},
>+ exp_rewind = 4,
>+ },
>+ {
>+ 'decode_unchecked(cdata<const char *>)',
>+ func = s.decode_unchecked,
>+ args = {ffi.cast('const char *', '\x93\x01\x02\x03')},
>+ exp_res = {1, 2, 3},
>+ exp_rewind = 4,
>+ },
>+ {
>+ 'decode_unchecked(cdata<char *>)',
>+ func = s.decode_unchecked,
>+ args = {ffi.cast('char *', '\x93\x01\x02\x03')},
>+ exp_res = {1, 2, 3},
>+ exp_rewind = 4,
>+ },
>+ }
>+
>+ test:plan(#cases)
>+
>+ for _, case in ipairs(cases) do
>+ test:test(case[1], function(test)
>+ test:plan(4)
>+ local args_len = table.maxn(case.args)
>+ local res, res_buf = case.func(unpack(case.args, 1, args_len))
>+ test:is_deeply(res, case.exp_res, 'verify result')
>+ local buf = case.args[1]
>+ local rewind = res_buf - buf
>+ test:is(rewind, case.exp_rewind, 'verify resulting buffer')
>+ -- test:iscdata() is not sufficient here, because it
>+ -- ignores 'const' qualifier (because of using
>+ -- ffi.istype()).
>+ test:is(type(res_buf), 'cdata', 'verify resulting buffer type')
>+ local buf_ctype = tostring(ffi.typeof(buf))
>+ local res_buf_ctype = tostring(ffi.typeof(res_buf))
>+ test:is(res_buf_ctype, buf_ctype, 'verify resulting buffer ctype')
>+ end)
>+ end
>+end
>+
> return {
>     test_unsigned = test_unsigned;
>     test_signed = test_signed;
>@@ -444,4 +502,5 @@ return {
>     test_ucdata = test_ucdata;
>     test_decimal = test_decimal;
>     test_depth = test_depth;
>+ test_decode_buffer = test_decode_buffer;
> }
>diff --git a/test/app-tap/msgpack.test.lua b/test/app-tap/msgpack.test.lua
>index 752f107a8..1df9d2372 100755
>--- a/test/app-tap/msgpack.test.lua
>+++ b/test/app-tap/msgpack.test.lua
>@@ -136,6 +136,27 @@ local function test_decode_array_map_header(test, s)
>             size = 4,
>             exp_err = end_of_buffer_err,
>         },
>+ -- gh-3926: Ensure that a returned pointer has the same
>+ -- cdata type as passed argument.
>+ --
>+ -- cdata<char *> arguments are passed in the cases above,
>+ -- so only cdata<const char *> argument is checked here.
>+ {
>+ 'fixarray (const char *)',
>+ func = s.decode_array_header,
>+ data = ffi.cast('const char *', '\x94'),
>+ size = 1,
>+ exp_len = 4,
>+ exp_rewind = 1,
>+ },
>+ {
>+ 'fixmap (const char *)',
>+ func = s.decode_map_header,
>+ data = ffi.cast('const char *', '\x84'),
>+ size = 1,
>+ exp_len = 4,
>+ exp_rewind = 1,
>+ },
>     }
> 
>     local bad_api_cases = {
>@@ -206,9 +227,14 @@ local function test_decode_array_map_header(test, s)
>         else
>             local len, new_buf = case.func(case.data, case.size)
>             local rewind = new_buf - case.data
>+
>+ -- gh-3926: Verify cdata type of a returned buffer.
>+ local data_ctype = tostring(ffi.typeof(case.data))
>+ local new_buf_ctype = tostring(ffi.typeof(new_buf))
>+
>             local description = ('good; %s'):format(case[1])
>- test:is_deeply({len, rewind}, {case.exp_len, case.exp_rewind},
>- description)
>+ test:is_deeply({len, rewind, new_buf_ctype}, {case.exp_len,
>+ case.exp_rewind, data_ctype}, description)
>         end
>     end
> 
>@@ -231,7 +257,7 @@ end
> 
> tap.test("msgpack", function(test)
>     local serializer = require('msgpack')
>- test:plan(12)
>+ test:plan(13)
>     test:test("unsigned", common.test_unsigned, serializer)
>     test:test("signed", common.test_signed, serializer)
>     test:test("double", common.test_double, serializer)
>@@ -244,4 +270,5 @@ tap.test("msgpack", function(test)
>     test:test("offsets", test_offsets, serializer)
>     test:test("misc", test_misc, serializer)
>     test:test("decode_array_map", test_decode_array_map_header, serializer)
>+ test:test("decode_buffer", common.test_decode_buffer, serializer)
> end)
>diff --git a/test/app-tap/msgpackffi.test.lua b/test/app-tap/msgpackffi.test.lua
>index e64228e4d..be6906e67 100755
>--- a/test/app-tap/msgpackffi.test.lua
>+++ b/test/app-tap/msgpackffi.test.lua
>@@ -117,45 +117,6 @@ local function test_other(test, s)
>                  encode_max_depth = max_depth})
> end
> 
>--- gh-3926: Ensure that a returned pointer has the same cdata type
>--- as passed argument.
>-local function test_decode_buffer(test, s)
>- local cases = {
>- {
>- 'decode_unchecked(cdata<const char *>)',
>- data = ffi.cast('const char *', '\x93\x01\x02\x03'),
>- exp_res = {1, 2, 3},
>- exp_rewind = 4,
>- },
>- {
>- 'decode_unchecked(cdata<char *>)',
>- data = ffi.cast('char *', '\x93\x01\x02\x03'),
>- exp_res = {1, 2, 3},
>- exp_rewind = 4,
>- },
>- }
>-
>- test:plan(#cases)
>-
>- for _, case in ipairs(cases) do
>- test:test(case[1], function(test)
>- test:plan(4)
>- local res, res_buf = s.decode_unchecked(case.data)
>- test:is_deeply(res, case.exp_res, 'verify result')
>- local rewind = res_buf - case.data
>- test:is(rewind, case.exp_rewind, 'verify resulting buffer')
>- -- test:iscdata() is not sufficient here, because it
>- -- ignores 'const' qualifier (because of using
>- -- ffi.istype()).
>- test:is(type(res_buf), 'cdata', 'verify resulting buffer type')
>- local data_ctype = tostring(ffi.typeof(case.data))
>- local res_buf_ctype = tostring(ffi.typeof(res_buf))
>- test:is(res_buf_ctype, data_ctype, 'verify resulting buffer ctype')
>- end)
>- end
>-end
>-
>-
> tap.test("msgpackffi", function(test)
>     local serializer = require('msgpackffi')
>     test:plan(11)
>@@ -171,5 +132,5 @@ tap.test("msgpackffi", function(test)
>     --test:test("ucdata", common.test_ucdata, serializer)
>     test:test("offsets", test_offsets, serializer)
>     test:test("other", test_other, serializer)
>- test:test("decode_buffer", test_decode_buffer, serializer)
>+ test:test("decode_buffer", common.test_decode_buffer, serializer)
> end)
>  
 
 
--
Maria Khaydich
 

[-- Attachment #2: Type: text/html, Size: 33785 bytes --]