Re: [Tarantool-patches] [PATCH 5/5] decimal: introduce and use lua_pushdecimalstr()

[Sergey Ostanevich via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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

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                             | 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
> 
> 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 = 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 = require('uuid')
> local uri = require('uri')
> local json = require('json')
> local msgpackffi = require('msgpackffi')
> +local decimal = 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 = 100
> +    local iter_count = 1000
> +    local is_success = true
> +
> +    local d1 = decimal.new(1111111111111111111)
> +    local d2 = decimal.new(2222222222222222222)
> +
> +    local function decimal_to_str()
> +        local str1 = tostring(d1)
> +        local str2 = tostring(d2)
> +        local str3 = tostring(d1)
> +        local str4 = tostring(d2)
> +        if str1 ~= str3 or str2 ~= str4 then
> +            is_success = 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-example <https://www.tarantool.io/en/doc/latest/reference/reference_lua/tap/#tap-example>
perhaps I miss something?

> +        end
> +    end
> +
> +    local function create_gc()
> +        for _ = 1, gc_count do
> +            ffi.gc(ffi.new('char[1]'), function() decimal_to_str() end)
> +        end
> +    end
> +
> +    for _ = 1, iter_count do
> +        create_gc()
> +        decimal_to_str()
> +    end
> +
> +    test:ok(is_success, 'decimal str in gc')
> +end
> +
> box.cfg{}
> 
> local test = 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)
> 


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