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

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

[Maria]

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

Test added
---
 src/lua/msgpackffi.lua           | 8 +++++---
 test/app-tap/msgpackffi.test.lua | 6 ++++++
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/src/lua/msgpackffi.lua b/src/lua/msgpackffi.lua
index f7ee44291..18a3cb3d5 100644
--- a/src/lua/msgpackffi.lua
+++ b/src/lua/msgpackffi.lua
@@ -11,6 +11,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 *
@@ -602,11 +603,12 @@ local function decode_unchecked(str, offset)
         local buf = ffi.cast(const_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) then
+        return r, ffi.cast(char_ptr_t, bufp[0]) - buf + 1
+    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(char_ptr_t, bufp[0])
     else
         error("msgpackffi.decode_unchecked(str, offset) -> res, new_offset | "..
               "msgpackffi.decode_unchecked(const char *buf) -> res, new_buf")
diff --git a/test/app-tap/msgpackffi.test.lua b/test/app-tap/msgpackffi.test.lua
index f2a8f254b..3ca09b304 100755
--- a/test/app-tap/msgpackffi.test.lua
+++ b/test/app-tap/msgpackffi.test.lua
@@ -4,6 +4,7 @@ package.path = "lua/?.lua;"..package.path
 
 local tap = require('tap')
 local common = require('serializer_test')
+local buffer = require('buffer')
 
 local function is_map(s)
     local b = string.byte(string.sub(s, 1, 1))
@@ -68,6 +69,11 @@ local function test_other(test, s)
     test:is(#s.encode(-0x8001), 5, "len(encode(-0x8001))")
     test:is(#s.encode(-0x80000000), 5, "len(encode(-0x80000000))")
     test:is(#s.encode(-0x80000001), 9, "len(encode(-0x80000001))")
+    
+    -- gh-3926: decode result cannot be assigned to buffer.rpos
+    local buf = buffer.ibuf()
+    local res, buf.rpos = msgpackffi.decode(buf.rpos, buf.wpos - buf.rpos)
+    test:iscdata(buf.rpos, ‘char *’)
 end
 
 tap.test("msgpackffi", function(test)
-- 
2.20.1 (Apple Git-117)

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

[Alexander Turenko]

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)

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

[Maria Khaydich]

[-- 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 --]

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

[Alexander Turenko]

LGTM for the code.

Requested changes for the commit message, see below.

Please, fix it and proceed to the next reviewer (I suggest to send the
patch to Vlad). Leave me in CC, please.

WBR, Alexander Turenko.

On Wed, Nov 13, 2019 at 02:21:06PM +0300, Maria Khaydich wrote:
> 
> 
> 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>:

I take a glance at commits in [1]. Sorry for the big delay.

The message of the first commit ('msgpackffi.decode can now be assigned
to buf.rpos') now reflects all intermediate changes. It should not:
please, reword the commit messsage to reflect the resulting state of the
patch.

In this particular case there is nothing in messages I left in fixup
commits that is worth to save in the resulting commit message.

The original commit message is the following, I'll comment it:

> msgpackffi.decode can now be assigned to buf.rpos

We usually prefix commit messages with a subsystem that is the subject
of a commit: 'lua' in this case.

I would state what was changed in the header (msgpackffi.decode*()
return value type) and then describe why (to assign to rpos) in the
commit message.

Maybe this is just my personal taste. I don't insist anyway.

>
> Function decode of module msgpackffi was passing
> value of type const unsigned char * to a C function
> that accepts arguments of type const char *.

msgpackffi.decode() returns 'unsigned char *', then a user passes this
result to a function that accepts 'const char *'? Is I interpret it
right? Didn't get this message, to be honest.

>
> Closes #3926

I would reword it like so (just for example, let's use any wording you
comfortable with):

  | lua: don't modify pointer type in msgpackffi.decode*
  |
  | When msgpackffi.decode() and msgpackffi.decode_unchecked() are called
  | with a buffer (cdata<[const] char *>) parameter, they return two values:
  | a decoded value and a new position within the buffer (it is a pointer
  | too).
  |
  | This commit changes a type of the returned pointer: it was
  | cdata<unsigned char *>, but now it is cdata<char *> or cdata<const char
  | *> depending of a function parameter.
  |
  | The primary reason why this change was made is to use
  | msgpackffi.decode*() with 'buffer' module seamlessly: read msgpack from
  | 'rpos' field and then promote 'rpos' to the new position:
  |
  |  | local msgpackffi = require('msgpackffi')
  |  | local buffer = require('buffer')
  |  |
  |  | local buf = buffer.ibuf()
  |  | <write msgpack data to 'buf'>
  |  | local res
  |  | res, buf.rpos = msgpackffi.decode(buf.rpos, buf:size())
  |
  | Before this commit the latter statement fails with the following error:
  |
  | > cannot convert 'const unsigned char *' to 'char *'
  |
  | Closes #3926

[1]: https://github.com/tarantool/tarantool/commits/eljashm/gh-3926-msgpackffi.decode-assignment-to-buf.rpos

Re: [Tarantool-patches] [tarantool-patches] Re: [PATCH] lua: keeping the pointer type in msgpackffi.decode

[Maria Khaydich]

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


Fixed the commit message according to your comments. 
 
>Понедельник, 9 декабря 2019, 2:36 +03:00 от Alexander Turenko < alexander.turenko@tarantool.org >:
>  
>LGTM for the code.
>
>Requested changes for the commit message, see below.
>
>Please, fix it and proceed to the next reviewer (I suggest to send the
>patch to Vlad). Leave me in CC, please.
>
>WBR, Alexander Turenko.
>
>On Wed, Nov 13, 2019 at 02:21:06PM +0300, Maria Khaydich wrote:
>>
>>
>> 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 >:
>
>I take a glance at commits in [1]. Sorry for the big delay.
>
>The message of the first commit ('msgpackffi.decode can now be assigned
>to buf.rpos') now reflects all intermediate changes. It should not:
>please, reword the commit messsage to reflect the resulting state of the
>patch.
>
>In this particular case there is nothing in messages I left in fixup
>commits that is worth to save in the resulting commit message.
>
>The original commit message is the following, I'll comment it:
>
>> msgpackffi.decode can now be assigned to buf.rpos
>
>We usually prefix commit messages with a subsystem that is the subject
>of a commit: 'lua' in this case.
>
>I would state what was changed in the header (msgpackffi.decode*()
>return value type) and then describe why (to assign to rpos) in the
>commit message.
>
>Maybe this is just my personal taste. I don't insist anyway.
>
>>
>> Function decode of module msgpackffi was passing
>> value of type const unsigned char * to a C function
>> that accepts arguments of type const char *.
>
>msgpackffi.decode() returns 'unsigned char *', then a user passes this
>result to a function that accepts 'const char *'? Is I interpret it
>right? Didn't get this message, to be honest.
>
>>
>> Closes #3926
>
>I would reword it like so (just for example, let's use any wording you
>comfortable with):
>
>  | lua: don't modify pointer type in msgpackffi.decode*
>  |
>  | When msgpackffi.decode() and msgpackffi.decode_unchecked() are called
>  | with a buffer (cdata<[const] char *>) parameter, they return two values:
>  | a decoded value and a new position within the buffer (it is a pointer
>  | too).
>  |
>  | This commit changes a type of the returned pointer: it was
>  | cdata<unsigned char *>, but now it is cdata<char *> or cdata<const char
>  | *> depending of a function parameter.
>  |
>  | The primary reason why this change was made is to use
>  | msgpackffi.decode*() with 'buffer' module seamlessly: read msgpack from
>  | 'rpos' field and then promote 'rpos' to the new position:
>  |
>  | | local msgpackffi = require('msgpackffi')
>  | | local buffer = require('buffer')
>  | |
>  | | local buf = buffer.ibuf()
>  | | <write msgpack data to 'buf'>
>  | | local res
>  | | res, buf.rpos = msgpackffi.decode(buf.rpos, buf:size())
>  |
>  | Before this commit the latter statement fails with the following error:
>  |
>  | > cannot convert 'const unsigned char *' to 'char *'
>  |
>  | Closes #3926
>
>[1]:  https://github.com/tarantool/tarantool/commits/eljashm/gh-3926-msgpackffi.decode-assignment-to-buf.rpos
>  
 
 
--
Maria Khaydich
 
 

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

Re: [Tarantool-patches] [tarantool-patches] Re: [PATCH] lua: keeping the pointer type in msgpackffi.decode

[Alexander Turenko]

On Wed, Dec 11, 2019 at 02:15:43PM +0300, Maria Khaydich wrote:
> 
> Fixed the commit message according to your comments. 

 | lua: keeping the pointer type in msgpackffi.decode()

Nit: I would say msgpackffi.decode*() to highlight that
decode_unchecked() is changed too.

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

[Vladislav Shpilevoy]

Hi! Thanks for the patch!

Review of the first commit:

============================================================================

> Author: Maria <marianneliash@gmail.com>
>
>     lua: keeping the pointer type in msgpackffi.decode()
>     
>     Method decode_unchecked returns two values - the one that has
>     been decoded and a pointer to the new position within the buffer
>     given as a parameter. The type of returned pointer used to be
>     cdata<unsigned char *> and it was not possible to assign returned
>     value to buf.rpos due to the following error:
>     
>     > cannot convert 'const unsigned char *' to 'char *'
>     
>     The patch fixes this by making decode_unchecked method return either
>     cdata<char *> or cdata<const char *> depending on the given parameter.
>     
>     Closes #3926
>
> diff --git a/test/app-tap/msgpackffi.test.lua b/test/app-tap/msgpackffi.test.lua
> index 36ac26b7e..e64228e4d 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')

Buffer is never used here. Lets drop it.

> +local ffi = require('ffi')

============================================================================

Review of the second commit:

============================================================================

> Author: Alexander Turenko <alexander.turenko@tarantool.org>
>
>     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/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)

Now "require('ffi')" in the beginning of that file becomes
not used. Please, drop it.

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

[Maria Khaydich]

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


Hello, thank you for the review! 
 
> Buffer is never used here. Lets drop it.
 
Done
 
> Now "require('ffi')" in the beginning of that file becomes not used. Please, drop it.
 
And done 
  
>Суббота, 21 декабря 2019, 1:00 +03:00 от Vladislav Shpilevoy <v.shpilevoy@tarantool.org>:
> 
>
>
>Hi! Thanks for the patch!
>
>Review of the first commit:
>
>============================================================================
>
>> Author: Maria < marianneliash@gmail.com >
>>
>> lua: keeping the pointer type in msgpackffi.decode()
>>
>> Method decode_unchecked returns two values - the one that has
>> been decoded and a pointer to the new position within the buffer
>> given as a parameter. The type of returned pointer used to be
>> cdata<unsigned char *> and it was not possible to assign returned
>> value to buf.rpos due to the following error:
>>
>> > cannot convert 'const unsigned char *' to 'char *'
>>
>> The patch fixes this by making decode_unchecked method return either
>> cdata<char *> or cdata<const char *> depending on the given parameter.
>>
>> Closes #3926
>>
>> diff --git a/test/app-tap/msgpackffi.test.lua b/test/app-tap/msgpackffi.test.lua
>> index 36ac26b7e..e64228e4d 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')
>
>Buffer is never used here. Lets drop it.
>
>> +local ffi = require('ffi')
>
>============================================================================
>
>Review of the second commit:
>
>============================================================================
>
>> Author: Alexander Turenko < alexander.turenko@tarantool.org >
>>
>> 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/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)
>
>Now "require('ffi')" in the beginning of that file becomes
>not used. Please, drop it.
>
>  
 
 
--
Maria Khaydich
 

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

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

[Vladislav Shpilevoy]

Hi! Thanks for the fixes!

LGTM.