From: Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: tarantool-patches@dev.tarantool.org, kyukhin@tarantool.org
Subject: [Tarantool-patches] [PATCH 09/15] uuid: drop tt_uuid_str() from Lua
Date: Wed, 24 Mar 2021 22:24:35 +0100 [thread overview]
Message-ID: <5ccc9c039d4c3708fda9760ccc53643d0066c0f4.1616620860.git.v.shpilevoy@tarantool.org> (raw)
In-Reply-To: <cover.1616620860.git.v.shpilevoy@tarantool.org>
The function converts struct tt_uuid * to a string. The string is
allocated on the static buffer, which can't be used in Lua due to
unpredictable GC behaviour. It can start working any moment even
if tt_uuid_str() has returned, but its result wasn't passed to
ffi.string() yet. Then the buffer might be overwritten.
Lua uuid now uses tt_uuid_to_string() which does the same but
takes the buffer pointer. The buffer is stored in an ffi stash,
because it is x4 times faster than ffi.new('char[37]') (where 37
is length of a UUID string + terminating 0) (2.4 ns vs 0.8 ns).
After this patch UUID is supposed to be fully compatible with Lua
GC handlers.
Part of #5632
(cherry picked from commit acf8745ed8fef47e6d1f1c31708c7c9d6324d2f3)
---
| 1 +
src/lua/uuid.lua | 13 ++++--
test/app-tap/gh-5632-gc-buf-reuse.test.lua | 49 ++++++++++++++++++++++
test/app/uuid.result | 2 +-
4 files changed, 61 insertions(+), 4 deletions(-)
create mode 100755 test/app-tap/gh-5632-gc-buf-reuse.test.lua
--git a/extra/exports b/extra/exports
index 91094206d..b27b22a7c 100644
--- a/extra/exports
+++ b/extra/exports
@@ -51,6 +51,7 @@ tt_uuid_is_equal
tt_uuid_is_nil
tt_uuid_bswap
tt_uuid_from_string
+tt_uuid_to_string
log_level
log_format
uri_parse
diff --git a/src/lua/uuid.lua b/src/lua/uuid.lua
index 46b35075a..3efb7f66b 100644
--- a/src/lua/uuid.lua
+++ b/src/lua/uuid.lua
@@ -26,8 +26,6 @@ bool
tt_uuid_is_nil(const struct tt_uuid *uu);
bool
tt_uuid_is_equal(const struct tt_uuid *lhs, const struct tt_uuid *rhs);
-char *
-tt_uuid_str(const struct tt_uuid *uu);
extern const struct tt_uuid uuid_nil;
]]
@@ -38,11 +36,20 @@ local uuid_stash = buffer.ffi_stash_new(uuid_t)
local uuid_stash_take = uuid_stash.take
local uuid_stash_put = uuid_stash.put
+local uuid_str_stash =
+ buffer.ffi_stash_new(string.format('char[%s]', UUID_STR_LEN + 1))
+local uuid_str_stash_take = uuid_str_stash.take
+local uuid_str_stash_put = uuid_str_stash.put
+
local uuid_tostring = function(uu)
if not ffi.istype(uuid_t, uu) then
return error('Usage: uuid:str()')
end
- return ffi.string(builtin.tt_uuid_str(uu), UUID_STR_LEN)
+ local strbuf = uuid_str_stash_take()
+ builtin.tt_uuid_to_string(uu, strbuf)
+ uu = ffi.string(strbuf, UUID_STR_LEN)
+ uuid_str_stash_put(strbuf)
+ return uu
end
local uuid_fromstr = function(str)
diff --git a/test/app-tap/gh-5632-gc-buf-reuse.test.lua b/test/app-tap/gh-5632-gc-buf-reuse.test.lua
new file mode 100755
index 000000000..b09b1bf3e
--- /dev/null
+++ b/test/app-tap/gh-5632-gc-buf-reuse.test.lua
@@ -0,0 +1,49 @@
+#!/usr/bin/env tarantool
+
+--
+-- gh-5632: Lua code should not use any global buffers or objects without
+-- proper ownership protection. Otherwise these items might be suddenly reused
+-- during Lua GC which happens almost at any moment. That might lead to data
+-- corruption.
+--
+
+local tap = require('tap')
+local ffi = require('ffi')
+local uuid = require('uuid')
+
+local function test_uuid(test)
+ test:plan(1)
+
+ local gc_count = 100
+ local iter_count = 1000
+ local is_success = true
+
+ local function uuid_to_str()
+ local uu = uuid.new()
+ local str1 = uu:str()
+ local str2 = uu:str()
+ if str1 ~= str2 then
+ is_success = false
+ assert(false)
+ end
+ end
+
+ local function create_gc()
+ for _ = 1, gc_count do
+ ffi.gc(ffi.new('char[1]'), function() uuid_to_str() end)
+ end
+ end
+
+ for _ = 1, iter_count do
+ create_gc()
+ uuid_to_str()
+ end
+
+ test:ok(is_success, 'uuid in gc')
+end
+
+local test = tap.test('gh-5632-gc-buf-reuse')
+test:plan(1)
+test:test('uuid in __gc', test_uuid)
+
+os.exit(test:check() and 0 or 1)
diff --git a/test/app/uuid.result b/test/app/uuid.result
index 1da8b3e58..b252c497d 100644
--- a/test/app/uuid.result
+++ b/test/app/uuid.result
@@ -106,7 +106,7 @@ uu.node[5]
-- invalid values
uuid.fromstr(nil)
---
-- error: 'builtin/uuid.lua:50: fromstr(str)'
+- error: 'builtin/uuid.lua:57: fromstr(str)'
...
uuid.fromstr('')
---
--
2.24.3 (Apple Git-128)
next prev parent reply other threads:[~2021-03-24 21:32 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-24 21:24 [Tarantool-patches] [PATCH 00/15] Cord buffer, static alloc, and Lua GC bug for 1.10 Vladislav Shpilevoy via Tarantool-patches
2021-03-24 21:24 ` [Tarantool-patches] [PATCH 01/15] fio: don't use shared buffer in pread() Vladislav Shpilevoy via Tarantool-patches
2021-03-24 21:24 ` [Tarantool-patches] [PATCH 10/15] uri: replace static_alloc with ffi stash and ibuf Vladislav Shpilevoy via Tarantool-patches
2021-03-24 21:24 ` [Tarantool-patches] [PATCH 11/15] lua: use lua_pushfstring() instead of tt_sprintf() Vladislav Shpilevoy via Tarantool-patches
2021-03-24 21:24 ` [Tarantool-patches] [PATCH 12/15] sio: rework sio_strfaddr() Vladislav Shpilevoy via Tarantool-patches
2021-03-24 21:24 ` [Tarantool-patches] [PATCH 13/15] sio: increase SERVICE_NAME_MAXLEN size Vladislav Shpilevoy via Tarantool-patches
2021-03-24 21:24 ` [Tarantool-patches] [PATCH 14/15] sio: introduce and use sio_snprintf() Vladislav Shpilevoy via Tarantool-patches
2021-03-24 21:24 ` [Tarantool-patches] [PATCH 15/15] buffer: remove Lua registers Vladislav Shpilevoy via Tarantool-patches
2021-03-24 21:24 ` [Tarantool-patches] [PATCH 02/15] test: don't use IBUF_SHARED in the tests Vladislav Shpilevoy via Tarantool-patches
2021-03-24 21:24 ` [Tarantool-patches] [PATCH 03/15] tuple: pass global ibuf explicitly where possible Vladislav Shpilevoy via Tarantool-patches
2021-03-24 21:24 ` [Tarantool-patches] [PATCH 04/15] iconv: take errno before reseting the context Vladislav Shpilevoy via Tarantool-patches
2021-03-24 21:24 ` [Tarantool-patches] [PATCH 05/15] cord_buf: introduce cord_buf API Vladislav Shpilevoy via Tarantool-patches
2021-03-24 21:24 ` [Tarantool-patches] [PATCH 06/15] cord_buf: introduce ownership management Vladislav Shpilevoy via Tarantool-patches
2021-03-24 21:24 ` [Tarantool-patches] [PATCH 07/15] buffer: implement ffi stash Vladislav Shpilevoy via Tarantool-patches
2021-03-24 21:24 ` [Tarantool-patches] [PATCH 08/15] uuid: replace static_alloc with " Vladislav Shpilevoy via Tarantool-patches
2021-03-24 21:24 ` Vladislav Shpilevoy via Tarantool-patches [this message]
2021-03-29 15:41 ` [Tarantool-patches] [PATCH 00/15] Cord buffer, static alloc, and Lua GC bug for 1.10 Kirill Yukhin 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=5ccc9c039d4c3708fda9760ccc53643d0066c0f4.1616620860.git.v.shpilevoy@tarantool.org \
--to=tarantool-patches@dev.tarantool.org \
--cc=kyukhin@tarantool.org \
--cc=v.shpilevoy@tarantool.org \
--subject='Re: [Tarantool-patches] [PATCH 09/15] uuid: drop tt_uuid_str() from Lua' \
/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
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox