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