Tarantool development patches archive
 help / color / mirror / Atom feed
From: Sergey Ostanevich via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH 5/5] decimal: introduce and use lua_pushdecimalstr()
Date: Thu, 29 Jul 2021 15:28:32 +0300
Message-ID: <55E403DE-9F49-42D5-9E9C-7B5207B17483@tarantool.org> (raw)
In-Reply-To: <0deaaafed27a864820d91181c5d5f8c0c7bdcf70.1627420835.git.v.shpilevoy@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 --]

  reply	other threads:[~2021-07-29 12:28 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-27 21:24 [Tarantool-patches] [PATCH 0/5] Static buf in Lua, part 3 Vladislav Shpilevoy via Tarantool-patches
2021-07-27 21:24 ` [Tarantool-patches] [PATCH 1/5] uuid: introduce and use luaL_pushuuidstr() Vladislav Shpilevoy via Tarantool-patches
2021-07-29 11:30   ` Sergey Ostanevich via Tarantool-patches
2021-07-27 21:24 ` [Tarantool-patches] [PATCH 2/5] info: use luaL_pushuuidstr() for box.info uuids Vladislav Shpilevoy via Tarantool-patches
2021-07-29 11:38   ` Sergey Ostanevich via Tarantool-patches
2021-08-01 15:03     ` Vladislav Shpilevoy via Tarantool-patches
2021-08-01 17:01       ` Sergey Ostanevich via Tarantool-patches
2021-07-27 21:24 ` [Tarantool-patches] [PATCH 3/5] decimal: rename decimal_to_string to decimal_str Vladislav Shpilevoy via Tarantool-patches
2021-07-29 11:41   ` Sergey Ostanevich via Tarantool-patches
2021-08-01 15:03     ` Vladislav Shpilevoy via Tarantool-patches
2021-08-01 17:01       ` Sergey Ostanevich via Tarantool-patches
2021-07-27 21:24 ` [Tarantool-patches] [PATCH 4/5] decimal: introduce decimal_to_string Vladislav Shpilevoy via Tarantool-patches
2021-07-29 11:52   ` Sergey Ostanevich via Tarantool-patches
2021-08-01 15:04     ` Vladislav Shpilevoy via Tarantool-patches
2021-08-01 17:06       ` Sergey Ostanevich via Tarantool-patches
2021-07-27 21:24 ` [Tarantool-patches] [PATCH 5/5] decimal: introduce and use lua_pushdecimalstr() Vladislav Shpilevoy via Tarantool-patches
2021-07-29 12:28   ` Sergey Ostanevich via Tarantool-patches [this message]
2021-08-01 15:04     ` Vladislav Shpilevoy via Tarantool-patches
2021-07-27 21:39 ` [Tarantool-patches] [PATCH 0/5] Static buf in Lua, part 3 Cyrill Gorcunov via Tarantool-patches
2021-08-02 19:45 ` Vladislav Shpilevoy via Tarantool-patches

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=55E403DE-9F49-42D5-9E9C-7B5207B17483@tarantool.org \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=sergos@tarantool.org \
    --cc=v.shpilevoy@tarantool.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Tarantool development patches archive

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://lists.tarantool.org/tarantool-patches/0 tarantool-patches/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 tarantool-patches tarantool-patches/ https://lists.tarantool.org/tarantool-patches \
		tarantool-patches@dev.tarantool.org.
	public-inbox-index tarantool-patches

Example config snippet for mirrors.


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git