From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 572E96EC55; Thu, 29 Jul 2021 15:28:36 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 572E96EC55 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1627561716; bh=VoYCRv3P3731rm178AO4x/90rfVO1ofsp6zoW0yNF5s=; h=Date:In-Reply-To:To:References:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=R7/awar2e5u51PEo5TD/7gOk7fjdW2WcCIEyD5Ra/hRHPsTcNPQdqmuwecHNsEdBl Y7x0bwsZwNSOKti/rNjx/xE/SpD6oOUHQQVqAR3zVxT3WUcLQYlwrv6RgOvfXcT7du WPEZNOo7AkhaaYrbTHAAnWdsTY133ActtJPEEqVM= Received: from smtp49.i.mail.ru (smtp49.i.mail.ru [94.100.177.109]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id D0D906EC55 for ; Thu, 29 Jul 2021 15:28:34 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org D0D906EC55 Received: by smtp49.i.mail.ru with esmtpa (envelope-from ) id 1m959N-0002us-Ul; Thu, 29 Jul 2021 15:28:34 +0300 Message-Id: <55E403DE-9F49-42D5-9E9C-7B5207B17483@tarantool.org> Content-Type: multipart/alternative; boundary="Apple-Mail=_D9CD886F-8D0D-46FF-B5EE-B7D4752E69FA" Mime-Version: 1.0 (Mac OS X Mail 14.0 \(3654.100.0.2.22\)) Date: Thu, 29 Jul 2021 15:28:32 +0300 In-Reply-To: <0deaaafed27a864820d91181c5d5f8c0c7bdcf70.1627420835.git.v.shpilevoy@tarantool.org> To: Vladislav Shpilevoy References: <0deaaafed27a864820d91181c5d5f8c0c7bdcf70.1627420835.git.v.shpilevoy@tarantool.org> X-Mailer: Apple Mail (2.3654.100.0.2.22) X-4EC0790: 10 X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD941C43E597735A9C354866C15C72ED952BE56FFA0EFAF5B8C182A05F53808504081EAB7CC68A967BCC74E1DF07CFEF6F86776842317BBF4CB2277F76A60DC9616 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7E8204D72912C702FEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F790063767A3307017EE919D8638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D838949870C5A9D95640BC5291CDE603E1117882F4460429724CE54428C33FAD305F5C1EE8F4F765FC2EE5AD8F952D28FBA471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F446042972877693876707352033AC447995A7AD18CB629EEF1311BF91D2E47CDBA5A96583BA9C0B312567BB231DD303D21008E29813377AFFFEAFD269A417C69337E82CC2E827F84554CEF50127C277FBC8AE2E8BA83251EDC214901ED5E8D9A59859A8B6AC294AFEFA671E80089D37D7C0E48F6C5571747095F342E88FB05168BE4CE3AF X-C1DE0DAB: 0D63561A33F958A542A86E65E26CDA7C539E0FDFD12205BE45DD8084FF89876DD59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA753530422897FB34C3410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D346AD04FDA812EEFF12E082B917D2E56501C4ECE9B82A1007D76FE85B38E2D1432F34BA315940EB43E1D7E09C32AA3244CAA4B5D938BAB163C1DAB6997F6F9EAF1250262A5EE9971B0FACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojPp/mPgZxawESPdMgrh2DDA== X-Mailru-Sender: 3B9A0136629DC912F4AABCEFC589C81E6200AF157719E7CD7D79D36FDA0E064C17AE8485831B59E8AD07DD1419AC565FA614486B47F28B67C5E079CCF3B0523AED31B7EB2E253A9E112434F685709FCF0DA7A0AF5A3A8387 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH 5/5] decimal: introduce and use lua_pushdecimalstr() X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Sergey Ostanevich via Tarantool-patches Reply-To: Sergey Ostanevich Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" --Apple-Mail=_D9CD886F-8D0D-46FF-B5EE-B7D4752E69FA Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=us-ascii Hi! Thanks for the patch! Just a minor on assert in test, LGTM if resolved similar to uuid patch. regards, Sergos > On 28 Jul 2021, at 00:24, Vladislav Shpilevoy = wrote: >=20 > decimal conversion to string in Lua used decimal_str() function. > The function is not safe to use in preemptive context like Lua, > where any attempt to push something onto the Lua stack might > trigger GC, which in turn might invoke any other code. >=20 > It is not safe because uses the static buffer, which is global and > cyclic. Newer allocations can override the old data without any > warning. >=20 > The same problem was fixed for tt_uuid_str() and uuids in > box.info in one of the previous commits. >=20 > The patch adds a new function lua_pushdecimalstr() which does not > use the static buffer. It is now used to push decimals safely on a > Lua stack. >=20 > Follow up #5632 > Follow up #6050 > Closes #6259 > --- > .../unreleased/gh-6259-static-buf-in-lua.md | 5 +++ > src/lua/decimal.c | 14 ++++++- > src/lua/decimal.h | 3 ++ > .../gh-5632-6050-6259-gc-buf-reuse.test.lua | 39 ++++++++++++++++++- > 4 files changed, 59 insertions(+), 2 deletions(-) > create mode 100644 changelogs/unreleased/gh-6259-static-buf-in-lua.md >=20 > diff --git a/changelogs/unreleased/gh-6259-static-buf-in-lua.md = b/changelogs/unreleased/gh-6259-static-buf-in-lua.md > new file mode 100644 > index 000000000..1a2f7705a > --- /dev/null > +++ b/changelogs/unreleased/gh-6259-static-buf-in-lua.md > @@ -0,0 +1,5 @@ > +## bugfix/core > + > + * `box.info.uuid`, `box.info.cluster.uuid`, and `tostring(decimal)` = with any > + decimal number in Lua sometimes could return garbage if `__gc` = handlers are > + used in user's code (gh-6259). > diff --git a/src/lua/decimal.c b/src/lua/decimal.c > index e25b3ec18..a50e4142a 100644 > --- a/src/lua/decimal.c > +++ b/src/lua/decimal.c > @@ -89,6 +89,18 @@ lua_pushdecimal(struct lua_State *L) > return res; > } >=20 > +void > +lua_pushdecimalstr(struct lua_State *L, const decimal_t *dec) > +{ > + /* > + * Do not use a global buffer. It might be overwritten if GC = starts > + * working. > + */ > + char str[DECIMAL_MAX_STR_LEN + 1]; > + decimal_to_string(dec, str); > + lua_pushstring(L, str); > +} > + > /** > * Returns true if a value at a given index is a decimal > * and false otherwise > @@ -376,7 +388,7 @@ ldecimal_tostring(struct lua_State *L) > if (lua_gettop(L) < 1) > return luaL_error(L, "usage: = decimal.tostring(decimal)"); > decimal_t *lhs =3D lua_checkdecimal(L, 1); > - lua_pushstring(L, decimal_str(lhs)); > + lua_pushdecimalstr(L, lhs); > return 1; > } >=20 > diff --git a/src/lua/decimal.h b/src/lua/decimal.h > index b13e59247..59a71c312 100644 > --- a/src/lua/decimal.h > +++ b/src/lua/decimal.h > @@ -44,6 +44,9 @@ struct lua_State; > decimal_t * > lua_pushdecimal(struct lua_State *L); >=20 > +void > +lua_pushdecimalstr(struct lua_State *L, const decimal_t *dec); > + > void > tarantool_lua_decimal_init(struct lua_State *L); >=20 > 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 > index f806ba6b7..45273ed9a 100755 > --- 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 > @@ -13,6 +13,7 @@ local uuid =3D require('uuid') > local uri =3D require('uri') > local json =3D require('json') > local msgpackffi =3D require('msgpackffi') > +local decimal =3D require('decimal') >=20 > local function test_uuid(test) > test:plan(1) > @@ -223,14 +224,50 @@ local function test_info_uuid(test) > test:ok(is_success, 'info uuid in gc') > end >=20 > +local function test_decimal(test) > + test:plan(1) > + > + local gc_count =3D 100 > + local iter_count =3D 1000 > + local is_success =3D true > + > + local d1 =3D decimal.new(1111111111111111111) > + local d2 =3D decimal.new(2222222222222222222) > + > + local function decimal_to_str() > + local str1 =3D tostring(d1) > + local str2 =3D tostring(d2) > + local str3 =3D tostring(d1) > + local str4 =3D tostring(d2) > + if str1 ~=3D str3 or str2 ~=3D str4 then > + is_success =3D false > + assert(false) Same as previos patch with uuid. I refer to the = https://www.tarantool.io/en/doc/latest/reference/reference_lua/tap/#tap-ex= ample = perhaps I miss something? > + end > + end > + > + local function create_gc() > + for _ =3D 1, gc_count do > + ffi.gc(ffi.new('char[1]'), function() decimal_to_str() = end) > + end > + end > + > + for _ =3D 1, iter_count do > + create_gc() > + decimal_to_str() > + end > + > + test:ok(is_success, 'decimal str in gc') > +end > + > box.cfg{} >=20 > local test =3D tap.test('gh-5632-6050-6259-gc-buf-reuse') > -test:plan(5) > +test:plan(6) > test:test('uuid in __gc', test_uuid) > test:test('uri in __gc', test_uri) > test:test('msgpackffi in __gc', test_msgpackffi) > test:test('json in __gc', test_json) > test:test('info uuid in __gc', test_info_uuid) > +test:test('decimal str in __gc', test_decimal) >=20 > os.exit(test:check() and 0 or 1) > --=20 > 2.24.3 (Apple Git-128) >=20 --Apple-Mail=_D9CD886F-8D0D-46FF-B5EE-B7D4752E69FA Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=us-ascii Hi! = Thanks for the patch!

Just a minor on assert in test, LGTM if resolved similar to = uuid patch.


regards,
Sergos

On 28 Jul 2021, at 00:24, = Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:

decimal conversion to string in Lua used decimal_str() = function.
The function is not safe to use in preemptive = context like Lua,
where any attempt to push something onto = the Lua stack might
trigger GC, which in turn might invoke = any other code.

It is not safe because uses = the static buffer, which is global and
cyclic. Newer = allocations can override the old data without any
warning.

The same problem was = fixed for tt_uuid_str() and uuids in
box.info in one of the previous = commits.

The patch adds a new function = lua_pushdecimalstr() which does not
use the static buffer. = It is now used to push decimals safely on a
Lua stack.

Follow up #5632
Follow up = #6050
Closes #6259
---
= .../unreleased/gh-6259-static-buf-in-lua.md   |  5 +++
src/lua/decimal.c =             &n= bsp;           &nbs= p;   | 14 ++++++-
src/lua/decimal.h =             &n= bsp;           &nbs= p;   |  3 ++
= .../gh-5632-6050-6259-gc-buf-reuse.test.lua   | 39 = ++++++++++++++++++-
4 files changed, 59 insertions(+), 2 = deletions(-)
create mode 100644 = changelogs/unreleased/gh-6259-static-buf-in-lua.md

diff --git = a/changelogs/unreleased/gh-6259-static-buf-in-lua.md = b/changelogs/unreleased/gh-6259-static-buf-in-lua.md
new = file mode 100644
index 000000000..1a2f7705a
--- /dev/null
+++ = b/changelogs/unreleased/gh-6259-static-buf-in-lua.md
@@ = -0,0 +1,5 @@
+## bugfix/core
+
+= * `box.info.uuid`, `box.info.cluster.uuid`, and `tostring(decimal)` = with any
+   decimal number in Lua sometimes = could return garbage if `__gc` handlers are
+ =   used in user's code (gh-6259).
diff --git = a/src/lua/decimal.c b/src/lua/decimal.c
index = e25b3ec18..a50e4142a 100644
--- a/src/lua/decimal.c
+++ b/src/lua/decimal.c
@@ -89,6 +89,18 @@ = lua_pushdecimal(struct lua_State *L)
return = res;
}

+void
+lua_pushdecimalstr(struct lua_State *L, const decimal_t = *dec)
+{
+ /*
+ * Do not = use a global buffer. It might be overwritten if GC starts
+ = * working.
+ */
+ char = str[DECIMAL_MAX_STR_LEN + 1];
+ = decimal_to_string(dec, str);
+ = lua_pushstring(L, str);
+}
+
/**
 * Returns true if a value at a = given index is a decimal
 * and false otherwise
@@ -376,7 +388,7 @@ ldecimal_tostring(struct lua_State *L)
= if (lua_gettop(L) < 1)
return = luaL_error(L, "usage: decimal.tostring(decimal)");
decimal_t = *lhs =3D lua_checkdecimal(L, 1);
- = lua_pushstring(L, decimal_str(lhs));
+ = lua_pushdecimalstr(L, lhs);
return = 1;
}

diff --git = a/src/lua/decimal.h b/src/lua/decimal.h
index = b13e59247..59a71c312 100644
--- a/src/lua/decimal.h
+++ b/src/lua/decimal.h
@@ -44,6 +44,9 @@ = struct lua_State;
decimal_t *
= lua_pushdecimal(struct lua_State *L);

+void
+lua_pushdecimalstr(struct lua_State *L, = const decimal_t *dec);
+
void
= tarantool_lua_decimal_init(struct lua_State *L);

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
index= f806ba6b7..45273ed9a 100755
--- = 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
@@ = -13,6 +13,7 @@ local uuid =3D require('uuid')
local uri =3D= require('uri')
local json =3D require('json')
local msgpackffi =3D require('msgpackffi')
+local decimal =3D require('decimal')

local function test_uuid(test)
=     test:plan(1)
@@ -223,14 +224,50 @@ = local function test_info_uuid(test)
=     test:ok(is_success, 'info uuid in gc')
end

+local function = test_decimal(test)
+    test:plan(1)
+
+    local gc_count =3D 100
+    local iter_count =3D 1000
+ =    local is_success =3D true
+
+    local d1 =3D = decimal.new(1111111111111111111)
+    local = d2 =3D decimal.new(2222222222222222222)
+
+ =    local function decimal_to_str()
+ =        local str1 =3D tostring(d1)
+        local str2 =3D = tostring(d2)
+ =        local str3 =3D tostring(d1)
+        local str4 =3D = tostring(d2)
+ =        if str1 ~=3D str3 or str2 ~=3D = str4 then
+ =            is_succe= ss =3D false
+ =            assert(f= alse)

Same as previos patch with uuid. I refer to = the
perhaps I miss something?

+        end
+=    end
+
+ =    local function create_gc()
+ =        for _ =3D 1, gc_count do
+ =            ffi.gc(f= fi.new('char[1]'), function() decimal_to_str() end)
+ =        end
+ =    end
+
+ =    for _ =3D 1, iter_count do
+ =        create_gc()
+ =        decimal_to_str()
+=    end
+
+ =    test:ok(is_success, 'decimal str in gc')
+end
+
box.cfg{}
local test =3D = tap.test('gh-5632-6050-6259-gc-buf-reuse')
-test:plan(5)
+test:plan(6)
test:test('uuid in __gc', = test_uuid)
test:test('uri in __gc', test_uri)
test:test('msgpackffi in __gc', test_msgpackffi)
test:test('json in __gc', test_json)
= test:test('info uuid in __gc', test_info_uuid)
+test:test('decimal str in __gc', test_decimal)

os.exit(test:check() and 0 or 1)
--
2.24.3 (Apple Git-128)


= --Apple-Mail=_D9CD886F-8D0D-46FF-B5EE-B7D4752E69FA--