<html><head><meta http-equiv="Content-Type" content="text/html; charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class="">Hi! Thanks for the patch!<div class=""><br class=""></div><div class="">Just a minor on assert in test, LGTM if resolved similar to uuid patch.</div><div class=""><br class=""></div><div class=""><br class=""></div><div class="">regards,</div><div class="">Sergos</div><div class=""><br class=""><div><blockquote type="cite" class=""><div class="">On 28 Jul 2021, at 00:24, Vladislav Shpilevoy <<a href="mailto:v.shpilevoy@tarantool.org" class="">v.shpilevoy@tarantool.org</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div class="">decimal conversion to string in Lua used decimal_str() function.<br class="">The function is not safe to use in preemptive context like Lua,<br class="">where any attempt to push something onto the Lua stack might<br class="">trigger GC, which in turn might invoke any other code.<br class=""><br class="">It is not safe because uses the static buffer, which is global and<br class="">cyclic. Newer allocations can override the old data without any<br class="">warning.<br class=""><br class="">The same problem was fixed for tt_uuid_str() and uuids in<br class=""><a href="http://box.info" class="">box.info</a> in one of the previous commits.<br class=""><br class="">The patch adds a new function lua_pushdecimalstr() which does not<br class="">use the static buffer. It is now used to push decimals safely on a<br class="">Lua stack.<br class=""><br class="">Follow up #5632<br class="">Follow up #6050<br class="">Closes #6259<br class="">---<br class=""> .../unreleased/gh-6259-static-buf-in-lua.md | 5 +++<br class=""> src/lua/decimal.c | 14 ++++++-<br class=""> src/lua/decimal.h | 3 ++<br class=""> .../gh-5632-6050-6259-gc-buf-reuse.test.lua | 39 ++++++++++++++++++-<br class=""> 4 files changed, 59 insertions(+), 2 deletions(-)<br class=""> create mode 100644 changelogs/unreleased/gh-6259-static-buf-in-lua.md<br class=""><br class="">diff --git a/changelogs/unreleased/gh-6259-static-buf-in-lua.md b/changelogs/unreleased/gh-6259-static-buf-in-lua.md<br class="">new file mode 100644<br class="">index 000000000..1a2f7705a<br class="">--- /dev/null<br class="">+++ b/changelogs/unreleased/gh-6259-static-buf-in-lua.md<br class="">@@ -0,0 +1,5 @@<br class="">+## bugfix/core<br class="">+<br class="">+ * `box.info.uuid`, `box.info.cluster.uuid`, and `tostring(decimal)` with any<br class="">+ decimal number in Lua sometimes could return garbage if `__gc` handlers are<br class="">+ used in user's code (gh-6259).<br class="">diff --git a/src/lua/decimal.c b/src/lua/decimal.c<br class="">index e25b3ec18..a50e4142a 100644<br class="">--- a/src/lua/decimal.c<br class="">+++ b/src/lua/decimal.c<br class="">@@ -89,6 +89,18 @@ lua_pushdecimal(struct lua_State *L)<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>return res;<br class=""> }<br class=""><br class="">+void<br class="">+lua_pushdecimalstr(struct lua_State *L, const decimal_t *dec)<br class="">+{<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>/*<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span> * Do not use a global buffer. It might be overwritten if GC starts<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span> * working.<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span> */<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>char str[DECIMAL_MAX_STR_LEN + 1];<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>decimal_to_string(dec, str);<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>lua_pushstring(L, str);<br class="">+}<br class="">+<br class=""> /**<br class=""> * Returns true if a value at a given index is a decimal<br class=""> * and false otherwise<br class="">@@ -376,7 +388,7 @@ ldecimal_tostring(struct lua_State *L)<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>if (lua_gettop(L) < 1)<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>return luaL_error(L, "usage: decimal.tostring(decimal)");<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>decimal_t *lhs = lua_checkdecimal(L, 1);<br class="">-<span class="Apple-tab-span" style="white-space:pre"> </span>lua_pushstring(L, decimal_str(lhs));<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>lua_pushdecimalstr(L, lhs);<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>return 1;<br class=""> }<br class=""><br class="">diff --git a/src/lua/decimal.h b/src/lua/decimal.h<br class="">index b13e59247..59a71c312 100644<br class="">--- a/src/lua/decimal.h<br class="">+++ b/src/lua/decimal.h<br class="">@@ -44,6 +44,9 @@ struct lua_State;<br class=""> decimal_t *<br class=""> lua_pushdecimal(struct lua_State *L);<br class=""><br class="">+void<br class="">+lua_pushdecimalstr(struct lua_State *L, const decimal_t *dec);<br class="">+<br class=""> void<br class=""> tarantool_lua_decimal_init(struct lua_State *L);<br class=""><br class="">diff --git a/test/app-tap/gh-5632-6050-6259-gc-buf-reuse.test.lua b/test/app-tap/gh-5632-6050-6259-gc-buf-reuse.test.lua<br class="">index f806ba6b7..45273ed9a 100755<br class="">--- a/test/app-tap/gh-5632-6050-6259-gc-buf-reuse.test.lua<br class="">+++ b/test/app-tap/gh-5632-6050-6259-gc-buf-reuse.test.lua<br class="">@@ -13,6 +13,7 @@ local uuid = require('uuid')<br class=""> local uri = require('uri')<br class=""> local json = require('json')<br class=""> local msgpackffi = require('msgpackffi')<br class="">+local decimal = require('decimal')<br class=""><br class=""> local function test_uuid(test)<br class=""> test:plan(1)<br class="">@@ -223,14 +224,50 @@ local function test_info_uuid(test)<br class=""> test:ok(is_success, 'info uuid in gc')<br class=""> end<br class=""><br class="">+local function test_decimal(test)<br class="">+ test:plan(1)<br class="">+<br class="">+ local gc_count = 100<br class="">+ local iter_count = 1000<br class="">+ local is_success = true<br class="">+<br class="">+ local d1 = decimal.new(1111111111111111111)<br class="">+ local d2 = decimal.new(2222222222222222222)<br class="">+<br class="">+ local function decimal_to_str()<br class="">+ local str1 = tostring(d1)<br class="">+ local str2 = tostring(d2)<br class="">+ local str3 = tostring(d1)<br class="">+ local str4 = tostring(d2)<br class="">+ if str1 ~= str3 or str2 ~= str4 then<br class="">+ is_success = false<br class="">+ assert(false)<br class=""></div></div></blockquote><div><br class=""></div>Same as previos patch with uuid. I refer to the</div><div><a href="https://www.tarantool.io/en/doc/latest/reference/reference_lua/tap/#tap-example" class="">https://www.tarantool.io/en/doc/latest/reference/reference_lua/tap/#tap-example</a></div><div>perhaps I miss something?</div><div><br class=""><blockquote type="cite" class=""><div class=""><div class="">+ end<br class="">+ end<br class="">+<br class="">+ local function create_gc()<br class="">+ for _ = 1, gc_count do<br class="">+ ffi.gc(ffi.new('char[1]'), function() decimal_to_str() end)<br class="">+ end<br class="">+ end<br class="">+<br class="">+ for _ = 1, iter_count do<br class="">+ create_gc()<br class="">+ decimal_to_str()<br class="">+ end<br class="">+<br class="">+ test:ok(is_success, 'decimal str in gc')<br class="">+end<br class="">+<br class=""> box.cfg{}<br class=""><br class=""> local test = tap.test('gh-5632-6050-6259-gc-buf-reuse')<br class="">-test:plan(5)<br class="">+test:plan(6)<br class=""> test:test('uuid in __gc', test_uuid)<br class=""> test:test('uri in __gc', test_uri)<br class=""> test:test('msgpackffi in __gc', test_msgpackffi)<br class=""> test:test('json in __gc', test_json)<br class=""> test:test('info uuid in __gc', test_info_uuid)<br class="">+test:test('decimal str in __gc', test_decimal)<br class=""><br class=""> os.exit(test:check() and 0 or 1)<br class="">-- <br class="">2.24.3 (Apple Git-128)<br class=""><br class=""></div></div></blockquote></div><br class=""></div></body></html>