Tarantool development patches archive
 help / color / mirror / Atom feed
From: Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: tarantool-patches@dev.tarantool.org, gorcunov@gmail.com,
	sergos@tarantool.org
Subject: [Tarantool-patches] [PATCH 5/5] decimal: introduce and use lua_pushdecimalstr()
Date: Tue, 27 Jul 2021 23:24:15 +0200
Message-ID: <0deaaafed27a864820d91181c5d5f8c0c7bdcf70.1627420835.git.v.shpilevoy@tarantool.org> (raw)
In-Reply-To: <cover.1627420835.git.v.shpilevoy@tarantool.org>

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)
+        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)


  parent reply	other threads:[~2021-07-27 21:26 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 ` Vladislav Shpilevoy via Tarantool-patches [this message]
2021-07-29 12:28   ` [Tarantool-patches] [PATCH 5/5] decimal: introduce and use lua_pushdecimalstr() Sergey Ostanevich via Tarantool-patches
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=0deaaafed27a864820d91181c5d5f8c0c7bdcf70.1627420835.git.v.shpilevoy@tarantool.org \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=gorcunov@gmail.com \
    --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