* [Tarantool-patches] [PATCH 01/16] fio: don't use shared buffer in pread()
2021-03-20 0:42 [Tarantool-patches] [PATCH 00/16] Cord buffer, static alloc, and Lua GC bug Vladislav Shpilevoy via Tarantool-patches
@ 2021-03-20 0:42 ` Vladislav Shpilevoy via Tarantool-patches
2021-03-22 7:19 ` Cyrill Gorcunov via Tarantool-patches
2021-03-20 0:42 ` [Tarantool-patches] [PATCH 10/16] uri: replace static_alloc with ffi stash and ibuf Vladislav Shpilevoy via Tarantool-patches
` (20 subsequent siblings)
21 siblings, 1 reply; 32+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-03-20 0:42 UTC (permalink / raw)
To: tarantool-patches, gorcunov, sergepetrenko
fio:pread() used buffer.IBUF_SHARED, which might be reused after a
yield. As a result, if pread() was called from 2 different fibers
or in parallel with something else using IBUF_SHARED, it would
turn the buffer into garbage for all parallel usages.
The same problem existed for read(), and was fixed in
c7c24f841322528c17714482d150b769d0afcddb ("fio: Fix race condition
in fio.read"). But apparently pread() was missed.
What is worse, the original commit's test passed even without the
fix from that commit. Because it didn't check the results of
read()s called from 2 fibers.
The patch fixes pread() and adds a test covering both read() and
pread(). The old test from the original commit is dropped.
Follow up #3187
---
src/lua/fio.lua | 3 +-
test/app/fio.result | 160 ++++++++++++++++++++++++------------------
test/app/fio.test.lua | 67 ++++++++++--------
3 files changed, 129 insertions(+), 101 deletions(-)
diff --git a/src/lua/fio.lua b/src/lua/fio.lua
index 954c44cdf..5db8bdea3 100644
--- a/src/lua/fio.lua
+++ b/src/lua/fio.lua
@@ -101,8 +101,7 @@ fio_methods.pread = function(self, buf, size, offset)
if not ffi.istype(const_char_ptr_t, buf) then
offset = size
size = buf
- tmpbuf = buffer.IBUF_SHARED
- tmpbuf:reset()
+ tmpbuf = buffer.ibuf()
buf = tmpbuf:reserve(size)
end
local res, err = internal.pread(self.fh, buf, size, offset)
diff --git a/test/app/fio.result b/test/app/fio.result
index 20cfe345d..8b5716658 100644
--- a/test/app/fio.result
+++ b/test/app/fio.result
@@ -1380,76 +1380,6 @@ fio.unlink(tmpfile)
---
- true
...
-tmp1 = fio.pathjoin(tmpdir, "tmp1")
----
-...
-tmp2= fio.pathjoin(tmpdir, "tmp2")
----
-...
-test_run:cmd("setopt delimiter ';'")
----
-- true
-...
-function write_file(name, odd)
- local fh = fio.open(name, { 'O_RDWR', 'O_TRUNC', 'O_CREAT' }, tonumber('0777', 8))
- if odd then
- fh:write(string.rep('1', 100))
- else
- fh:write(string.rep('2', 100))
- end
- fh:write(name)
- fh:seek(0)
- return fh
-end;
----
-...
-test_run:cmd("setopt delimiter ''");
----
-- true
-...
-fh1 = write_file(tmp1)
----
-...
-fh2 = write_file(tmp2)
----
-...
-fiber = require('fiber')
----
-...
-digest = require('digest')
----
-...
-str = fh1:read()
----
-...
-fh1:seek(0)
----
-- 0
-...
-hash = digest.crc32(str)
----
-...
-ch = fiber.channel(1)
----
-...
-f1 = fiber.create(function() str = fh1:read() ch:put(digest.crc32(str)) end)
----
-...
-f2 = fiber.create(function() str = fh2:read() end)
----
-...
-ch:get() == hash
----
-- true
-...
-fio.unlink(tmp1)
----
-- true
-...
-fio.unlink(tmp2)
----
-- true
-...
fio.rmdir(tmpdir)
---
- true
@@ -1602,3 +1532,93 @@ tmpdir = nil
os.setenv('TMPDIR', old_tmpdir)
---
...
+--
+-- read() and pread() should not use a shared buffer so as not to clash with
+-- other fibers during yield.
+--
+rights = tonumber('0777', 8)
+---
+...
+flags = {'O_RDWR', 'O_TRUNC', 'O_CREAT'}
+---
+...
+tmpdir = fio.tempdir()
+---
+...
+file1 = fio.pathjoin(tmpdir, 'file1')
+---
+...
+file2 = fio.pathjoin(tmpdir, 'file2')
+---
+...
+fd1 = fio.open(file1, flags, rights)
+---
+...
+fd2 = fio.open(file2, flags, rights)
+---
+...
+fd1:write('1'), fd1:seek(0)
+---
+- true
+- 0
+...
+fd2:write('2'), fd2:seek(0)
+---
+- true
+- 0
+...
+res1, res2 = nil
+---
+...
+do \
+ fiber.new(function() res1 = fd1:read() end) \
+ fiber.new(function() res2 = fd2:read() end) \
+end
+---
+...
+_ = test_run:wait_cond(function() return res1 and res2 end)
+---
+...
+assert(res1 == '1')
+---
+- true
+...
+assert(res2 == '2')
+---
+- true
+...
+--
+-- The same with pread().
+--
+res1, res2 = nil
+---
+...
+do \
+ fiber.new(function() res1 = fd1:pread(1, 0) end) \
+ fiber.new(function() res2 = fd2:pread(1, 0) end) \
+end
+---
+...
+_ = test_run:wait_cond(function() return res1 and res2 end)
+---
+...
+assert(res1 == '1')
+---
+- true
+...
+assert(res2 == '2')
+---
+- true
+...
+fd1:close()
+---
+- true
+...
+fd2:close()
+---
+- true
+...
+fio.rmtree(tmpdir)
+---
+- true
+...
diff --git a/test/app/fio.test.lua b/test/app/fio.test.lua
index d1a20e8e5..085d72110 100644
--- a/test/app/fio.test.lua
+++ b/test/app/fio.test.lua
@@ -442,35 +442,6 @@ fio.path.lexists(link)
fio.unlink(link)
fio.unlink(tmpfile)
-tmp1 = fio.pathjoin(tmpdir, "tmp1")
-tmp2= fio.pathjoin(tmpdir, "tmp2")
-test_run:cmd("setopt delimiter ';'")
-function write_file(name, odd)
- local fh = fio.open(name, { 'O_RDWR', 'O_TRUNC', 'O_CREAT' }, tonumber('0777', 8))
- if odd then
- fh:write(string.rep('1', 100))
- else
- fh:write(string.rep('2', 100))
- end
- fh:write(name)
- fh:seek(0)
- return fh
-end;
-test_run:cmd("setopt delimiter ''");
-fh1 = write_file(tmp1)
-fh2 = write_file(tmp2)
-fiber = require('fiber')
-digest = require('digest')
-str = fh1:read()
-fh1:seek(0)
-hash = digest.crc32(str)
-ch = fiber.channel(1)
-f1 = fiber.create(function() str = fh1:read() ch:put(digest.crc32(str)) end)
-f2 = fiber.create(function() str = fh2:read() end)
-ch:get() == hash
-
-fio.unlink(tmp1)
-fio.unlink(tmp2)
fio.rmdir(tmpdir)
--
@@ -526,3 +497,41 @@ fio.tempdir()
tmpdir = nil
os.setenv('TMPDIR', old_tmpdir)
+
+--
+-- read() and pread() should not use a shared buffer so as not to clash with
+-- other fibers during yield.
+--
+rights = tonumber('0777', 8)
+flags = {'O_RDWR', 'O_TRUNC', 'O_CREAT'}
+tmpdir = fio.tempdir()
+file1 = fio.pathjoin(tmpdir, 'file1')
+file2 = fio.pathjoin(tmpdir, 'file2')
+fd1 = fio.open(file1, flags, rights)
+fd2 = fio.open(file2, flags, rights)
+fd1:write('1'), fd1:seek(0)
+fd2:write('2'), fd2:seek(0)
+
+res1, res2 = nil
+do \
+ fiber.new(function() res1 = fd1:read() end) \
+ fiber.new(function() res2 = fd2:read() end) \
+end
+_ = test_run:wait_cond(function() return res1 and res2 end)
+assert(res1 == '1')
+assert(res2 == '2')
+--
+-- The same with pread().
+--
+res1, res2 = nil
+do \
+ fiber.new(function() res1 = fd1:pread(1, 0) end) \
+ fiber.new(function() res2 = fd2:pread(1, 0) end) \
+end
+_ = test_run:wait_cond(function() return res1 and res2 end)
+assert(res1 == '1')
+assert(res2 == '2')
+
+fd1:close()
+fd2:close()
+fio.rmtree(tmpdir)
--
2.24.3 (Apple Git-128)
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [Tarantool-patches] [PATCH 01/16] fio: don't use shared buffer in pread()
2021-03-20 0:42 ` [Tarantool-patches] [PATCH 01/16] fio: don't use shared buffer in pread() Vladislav Shpilevoy via Tarantool-patches
@ 2021-03-22 7:19 ` Cyrill Gorcunov via Tarantool-patches
0 siblings, 0 replies; 32+ messages in thread
From: Cyrill Gorcunov via Tarantool-patches @ 2021-03-22 7:19 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
On Sat, Mar 20, 2021 at 01:42:31AM +0100, Vladislav Shpilevoy wrote:
> fio:pread() used buffer.IBUF_SHARED, which might be reused after a
> yield. As a result, if pread() was called from 2 different fibers
> or in parallel with something else using IBUF_SHARED, it would
> turn the buffer into garbage for all parallel usages.
>
> The same problem existed for read(), and was fixed in
> c7c24f841322528c17714482d150b769d0afcddb ("fio: Fix race condition
> in fio.read"). But apparently pread() was missed.
>
> What is worse, the original commit's test passed even without the
> fix from that commit. Because it didn't check the results of
> read()s called from 2 fibers.
>
> The patch fixes pread() and adds a test covering both read() and
> pread(). The old test from the original commit is dropped.
Ack as obvious
^ permalink raw reply [flat|nested] 32+ messages in thread
* [Tarantool-patches] [PATCH 10/16] uri: replace static_alloc with ffi stash and ibuf
2021-03-20 0:42 [Tarantool-patches] [PATCH 00/16] Cord buffer, static alloc, and Lua GC bug Vladislav Shpilevoy via Tarantool-patches
2021-03-20 0:42 ` [Tarantool-patches] [PATCH 01/16] fio: don't use shared buffer in pread() Vladislav Shpilevoy via Tarantool-patches
@ 2021-03-20 0:42 ` Vladislav Shpilevoy via Tarantool-patches
2021-03-20 0:42 ` [Tarantool-patches] [PATCH 11/16] buffer: remove static_alloc() from Lua Vladislav Shpilevoy via Tarantool-patches
` (19 subsequent siblings)
21 siblings, 0 replies; 32+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-03-20 0:42 UTC (permalink / raw)
To: tarantool-patches, gorcunov, sergepetrenko
static_alloc() appears not to be safe to use in Lua, because it
does not provide any ownership protection for the returned values.
The problem appears when something is allocated, then Lua GC
starts, and some __gc handlers might also use static_alloc(). In
Lua and in C - both lead to the buffer being corrupted in its
original usage place.
The patch is a part of activity of getting rid of static_alloc()
in Lua. It removes it from uri Lua module and makes it use the
new FFI stash feature, which helps to cache frequently used and
heavy to allocate FFI values.
In one place static_alloc() was used for an actual buffer - it was
replaced with cord_ibuf which is equally fast when preallocated.
ffi.new() for temporary struct uri is not used, because
- It produces a new GC object;
- ffi.new('struct uri') costs around 20ns while FFI stash
costs around 0.8ns. The hack with 'struct uri[1]' does not help
because size of uri is > 128 bytes;
- Without JIT ffi.new() costs about the same as the stash, not
better as well;
The patch makes uri perf a bit better in the places where
static_alloc() was used, because its cost was around 7ns for one
allocation.
---
src/lua/uri.lua | 21 ++++++--
test/app-tap/gh-5632-gc-buf-reuse.test.lua | 60 +++++++++++++++++++++-
2 files changed, 75 insertions(+), 6 deletions(-)
diff --git a/src/lua/uri.lua b/src/lua/uri.lua
index e08e20675..98f4e02ec 100644
--- a/src/lua/uri.lua
+++ b/src/lua/uri.lua
@@ -1,7 +1,7 @@
-- uri.lua (internal file)
local ffi = require('ffi')
-local static_alloc = require('buffer').static_alloc
+local buffer = require('buffer')
ffi.cdef[[
struct uri {
@@ -32,13 +32,19 @@ uri_format(char *str, size_t len, struct uri *uri, bool write_password);
]]
local builtin = ffi.C;
+local uri_stash = buffer.ffi_stash_new('struct uri')
+local uri_stash_take = uri_stash.take
+local uri_stash_put = uri_stash.put
+local cord_ibuf_take = buffer.internal.cord_ibuf_take
+local cord_ibuf_put = buffer.internal.cord_ibuf_put
local function parse(str)
if str == nil then
error("Usage: uri.parse(string)")
end
- local uribuf = static_alloc('struct uri')
+ local uribuf = uri_stash_take()
if builtin.uri_parse(uribuf, str) ~= 0 then
+ uri_stash_put(uribuf)
return nil
end
local result = {}
@@ -55,11 +61,12 @@ local function parse(str)
elseif uribuf.host_hint == 3 then
result.unix = result.service
end
+ uri_stash_put(uribuf)
return result
end
local function format(uri, write_password)
- local uribuf = static_alloc('struct uri')
+ local uribuf = uri_stash_take()
uribuf.scheme = uri.scheme
uribuf.scheme_len = string.len(uri.scheme or '')
uribuf.login = uri.login
@@ -76,9 +83,13 @@ local function format(uri, write_password)
uribuf.query_len = string.len(uri.query or '')
uribuf.fragment = uri.fragment
uribuf.fragment_len = string.len(uri.fragment or '')
- local str = static_alloc('char', 1024)
+ local ibuf = cord_ibuf_take()
+ local str = ibuf:alloc(1024)
local len = builtin.uri_format(str, 1024, uribuf, write_password and 1 or 0)
- return ffi.string(str, len)
+ uri_stash_put(uribuf)
+ str = ffi.string(str, len)
+ cord_ibuf_put(ibuf)
+ return str
end
return {
diff --git a/test/app-tap/gh-5632-gc-buf-reuse.test.lua b/test/app-tap/gh-5632-gc-buf-reuse.test.lua
index 8fe662d3f..bfa86615f 100755
--- a/test/app-tap/gh-5632-gc-buf-reuse.test.lua
+++ b/test/app-tap/gh-5632-gc-buf-reuse.test.lua
@@ -10,6 +10,7 @@
local tap = require('tap')
local ffi = require('ffi')
local uuid = require('uuid')
+local uri = require('uri')
local function test_uuid(test)
test:plan(1)
@@ -42,8 +43,65 @@ local function test_uuid(test)
test:ok(is_success, 'uuid in gc')
end
+local function test_uri(test)
+ test:plan(1)
+
+ local gc_count = 100
+ local iter_count = 1000
+ local port = 1
+ local ip = 1
+ local login = 1
+ local pass = 1
+ local is_success = true
+
+ local function uri_parse()
+ local loc_ip = ip
+ local loc_port = port
+ local loc_pass = pass
+ local loc_login = login
+
+ ip = ip + 1
+ port = port + 1
+ pass = pass + 1
+ login = login + 1
+ if port > 60000 then
+ port = 1
+ end
+ if ip > 255 then
+ ip = 1
+ end
+
+ loc_ip = string.format('127.0.0.%s', loc_ip)
+ loc_port = tostring(loc_port)
+ loc_pass = string.format('password%s', loc_pass)
+ loc_login = string.format('login%s', loc_login)
+ local host = string.format('%s:%s@%s:%s', loc_login, loc_pass,
+ loc_ip, loc_port)
+ local u = uri.parse(host)
+ if u.host ~= loc_ip or u.login ~= loc_login or u.service ~= loc_port or
+ u.password ~= loc_pass then
+ is_success = false
+ assert(false)
+ end
+ end
+
+ local function create_gc()
+ for i = 1, gc_count do
+ ffi.gc(ffi.new('char[1]'), uri_parse)
+ end
+ end
+
+ for i = 1, iter_count do
+ create_gc()
+ uri_parse()
+ end
+
+ test:ok(is_success, 'uri in gc')
+end
+
local test = tap.test('gh-5632-gc-buf-reuse')
-test:plan(1)
+test:plan(2)
test:test('uuid in __gc', test_uuid)
+test:test('uri in __gc', test_uri)
os.exit(test:check() and 0 or 1)
--
2.24.3 (Apple Git-128)
^ permalink raw reply [flat|nested] 32+ messages in thread* [Tarantool-patches] [PATCH 11/16] buffer: remove static_alloc() from Lua
2021-03-20 0:42 [Tarantool-patches] [PATCH 00/16] Cord buffer, static alloc, and Lua GC bug Vladislav Shpilevoy via Tarantool-patches
2021-03-20 0:42 ` [Tarantool-patches] [PATCH 01/16] fio: don't use shared buffer in pread() Vladislav Shpilevoy via Tarantool-patches
2021-03-20 0:42 ` [Tarantool-patches] [PATCH 10/16] uri: replace static_alloc with ffi stash and ibuf Vladislav Shpilevoy via Tarantool-patches
@ 2021-03-20 0:42 ` Vladislav Shpilevoy via Tarantool-patches
2021-03-20 0:42 ` [Tarantool-patches] [PATCH 12/16] lua: use lua_pushfstring() instead of tt_sprintf() Vladislav Shpilevoy via Tarantool-patches
` (18 subsequent siblings)
21 siblings, 0 replies; 32+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-03-20 0:42 UTC (permalink / raw)
To: tarantool-patches, gorcunov, sergepetrenko
Static_alloc() uses a fixed size circular BSS memory buffer. It is
often used in C when need to allocate something of a size smaller
than the static buffer temporarily. And it was thought that it
might be also useful in Lua when backed up by ffi.new() for large
allocations.
It was useful, and faster than ffi.new() on sizes > 128 and less
than the static buffer size, but it wasn't correct to use it. By
the same reason why IBUF_SHARED global variable should not have
been used as is. Because without a proper ownership the buffer
might be reused in some unexpected way.
Just like with IBUF_SHARED, the static buffer could be reused
during Lua GC in one of __gc handlers. Essentially, at any moment
on almost any line of a Lua script.
IBUF_SHARED was fixed by proper ownership implementation, but it
is not possible with the static buffer. Because there is no such a
thing like a static buffer object which can be owned, and even if
there would be, cost of its support wouldn't be much better than
for the new cord_ibuf API. That would make the static buffer close
to pointless.
This patch eliminates static_alloc() from Lua, and uses cord_ibuf
instead almost everywhere except a couple of places where
ffi.new() is good enough.
Part of #5632
---
src/CMakeLists.txt | 1 -
src/box/lua/schema.lua | 7 ++++--
src/exports.h | 1 -
src/lua/buffer.c | 42 --------------------------------
src/lua/buffer.lua | 32 ------------------------
src/lua/crypto.lua | 10 ++++++--
src/lua/digest.lua | 25 +++++++++++++------
src/lua/fio.lua | 9 +++++--
src/lua/socket.lua | 20 ++++++++++-----
src/lua/string.lua | 18 ++++++++++----
test/app-tap/module_api.test.lua | 3 ++-
test/app-tap/uri.test.lua | 23 +----------------
test/app/buffer.result | 17 -------------
test/app/buffer.test.lua | 9 -------
14 files changed, 68 insertions(+), 149 deletions(-)
delete mode 100644 src/lua/buffer.c
diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
index fb528b666..19839b6af 100644
--- a/src/CMakeLists.txt
+++ b/src/CMakeLists.txt
@@ -131,7 +131,6 @@ set (server_sources
lua/utf8.c
lua/info.c
lua/string.c
- lua/buffer.c
lua/swim.c
lua/decimal.c
${lua_sources}
diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
index cc5a3451a..b345b3050 100644
--- a/src/box/lua/schema.lua
+++ b/src/box/lua/schema.lua
@@ -2725,9 +2725,12 @@ box.schema.user = {}
box.schema.user.password = function(password)
local BUF_SIZE = 128
- local buf = buffer.static_alloc('char', BUF_SIZE)
+ local ibuf = cord_ibuf_take()
+ local buf = ibuf:alloc(BUF_SIZE)
builtin.password_prepare(password, #password, buf, BUF_SIZE)
- return ffi.string(buf)
+ buf = ffi.string(buf)
+ cord_ibuf_put(ibuf)
+ return buf
end
local function chpasswd(uid, new_password)
diff --git a/src/exports.h b/src/exports.h
index a90b9406a..41357636a 100644
--- a/src/exports.h
+++ b/src/exports.h
@@ -329,7 +329,6 @@ EXPORT(lua_setmetatable)
EXPORT(lua_settable)
EXPORT(lua_settop)
EXPORT(lua_setupvalue)
-EXPORT(lua_static_aligned_alloc)
EXPORT(lua_status)
EXPORT(lua_toboolean)
EXPORT(lua_tocfunction)
diff --git a/src/lua/buffer.c b/src/lua/buffer.c
deleted file mode 100644
index 5fd349261..000000000
--- a/src/lua/buffer.c
+++ /dev/null
@@ -1,42 +0,0 @@
-/*
- * Copyright 2010-2019, Tarantool AUTHORS, please see AUTHORS file.
- *
- * Redistribution and use in source and binary forms, with or
- * without modification, are permitted provided that the following
- * conditions are met:
- *
- * 1. Redistributions of source code must retain the above
- * copyright notice, this list of conditions and the
- * following disclaimer.
- *
- * 2. Redistributions in binary form must reproduce the above
- * copyright notice, this list of conditions and the following
- * disclaimer in the documentation and/or other materials
- * provided with the distribution.
- *
- * THIS SOFTWARE IS PROVIDED BY <COPYRIGHT HOLDER> ``AS IS'' AND
- * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
- * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
- * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
- * <COPYRIGHT HOLDER> OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
- * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
- * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
- * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
- * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
- * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
- * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
- * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
- * SUCH DAMAGE.
- */
-#include "small/static.h"
-
-/**
- * Static inline functions like in static.h can't be exported.
- * Here they are given a symbol to export.
- */
-
-void *
-lua_static_aligned_alloc(size_t size, size_t alignment)
-{
- return static_aligned_alloc(size, alignment);
-}
diff --git a/src/lua/buffer.lua b/src/lua/buffer.lua
index 9bbd1d98d..4d0bef91a 100644
--- a/src/lua/buffer.lua
+++ b/src/lua/buffer.lua
@@ -42,9 +42,6 @@ ibuf_reinit(struct ibuf *ibuf);
void *
ibuf_reserve_slow(struct ibuf *ibuf, size_t size);
-void *
-lua_static_aligned_alloc(size_t size, size_t alignment);
-
/**
* Register is a buffer to use with FFI functions, which usually
* operate with pointers to scalar values like int, char, size_t,
@@ -263,34 +260,6 @@ local function ffi_stash_new(c_type)
}
end
---
--- NOTE: ffi.new() with inlined size <= 128 works even faster
--- than this allocator. If your size is a constant <= 128 -
--- use ffi.new(). This function is faster than malloc,
--- ffi.new(> 128), and ffi.new() with any size not known in
--- advance like this:
---
--- local size = <any expression>
--- ffi.new('char[?]', size)
---
--- Allocate a chunk of static BSS memory, or use ordinal ffi.new,
--- when too big size.
--- @param type C type - a struct, a basic type, etc. Should be a
--- string: 'int', 'char *', 'struct tuple', etc.
--- @param size Optional argument, number of elements of @a type to
--- allocate. 1 by default.
--- @return Cdata pointer to @a type.
---
-local function static_alloc(type, size)
- size = size or 1
- local bsize = size * ffi.sizeof(type)
- local ptr = builtin.lua_static_aligned_alloc(bsize, ffi.alignof(type))
- if ptr ~= nil then
- return ffi.cast(type..' *', ptr)
- end
- return ffi.new(type..'[?]', size)
-end
-
--
-- Sometimes it is wanted to use several temporary registers at
-- once. For example, when a C function takes 2 C pointers. Then
@@ -344,7 +313,6 @@ return {
internal = internal,
ibuf = ibuf_new;
READAHEAD = READAHEAD;
- static_alloc = static_alloc,
-- Keep reference.
reg_array = reg_array,
reg1 = reg_array[0],
diff --git a/src/lua/crypto.lua b/src/lua/crypto.lua
index f31708e29..5a219cf30 100644
--- a/src/lua/crypto.lua
+++ b/src/lua/crypto.lua
@@ -3,6 +3,8 @@
local ffi = require('ffi')
local buffer = require('buffer')
local reg = buffer.reg1
+local cord_ibuf_take = buffer.internal.cord_ibuf_take
+local cord_ibuf_put = buffer.internal.cord_ibuf_put
ffi.cdef[[
/* from openssl/err.h */
@@ -204,11 +206,15 @@ local function hmac_final(self)
return error('HMAC not initialized')
end
self.initialized = false
- local buf = buffer.static_alloc('char', 64)
+ local ibuf = cord_ibuf_take()
+ local buf = ibuf:alloc(64)
if ffi.C.HMAC_Final(self.ctx, buf, reg.ai) ~= 1 then
+ cord_ibuf_put(ibuf)
return error('Can\'t finalize HMAC: ' .. openssl_err_str())
end
- return ffi.string(buf, reg.ai[0])
+ buf = ffi.string(buf, reg.ai[0])
+ cord_ibuf_put(ibuf)
+ return buf
end
local function hmac_free(self)
diff --git a/src/lua/digest.lua b/src/lua/digest.lua
index 464cc6525..8c901ff41 100644
--- a/src/lua/digest.lua
+++ b/src/lua/digest.lua
@@ -3,7 +3,9 @@
local ffi = require('ffi')
local crypto = require('crypto')
local bit = require('bit')
-local static_alloc = require('buffer').static_alloc
+local buffer = require('buffer')
+local cord_ibuf_take = buffer.internal.cord_ibuf_take
+local cord_ibuf_put = buffer.internal.cord_ibuf_put
ffi.cdef[[
/* internal implementation */
@@ -180,9 +182,12 @@ local m = {
end
local blen = #bin
local slen = ffi.C.base64_bufsize(blen, mask)
- local str = static_alloc('char', slen)
+ local ibuf = cord_ibuf_take()
+ local str = ibuf:alloc(slen)
local len = ffi.C.base64_encode(bin, blen, str, slen, mask)
- return ffi.string(str, len)
+ str = ffi.string(str, len)
+ cord_ibuf_put(ibuf)
+ return str
end,
base64_decode = function(str)
@@ -191,9 +196,12 @@ local m = {
end
local slen = #str
local blen = math.ceil(slen * 3 / 4)
- local bin = static_alloc('char', blen)
+ local ibuf = cord_ibuf_take()
+ local bin = ibuf:alloc(blen)
local len = ffi.C.base64_decode(str, slen, bin, blen)
- return ffi.string(bin, len)
+ bin = ffi.string(bin, len)
+ cord_ibuf_put(ibuf)
+ return bin
end,
crc32 = CRC32,
@@ -229,9 +237,12 @@ local m = {
if n == nil then
error('Usage: digest.urandom(len)')
end
- local buf = static_alloc('char', n)
+ local ibuf = cord_ibuf_take()
+ local buf = ibuf:alloc(n)
ffi.C.random_bytes(buf, n)
- return ffi.string(buf, n)
+ buf = ffi.string(buf, n)
+ cord_ibuf_put(ibuf)
+ return buf
end,
murmur = PMurHash,
diff --git a/src/lua/fio.lua b/src/lua/fio.lua
index 5db8bdea3..da580fb04 100644
--- a/src/lua/fio.lua
+++ b/src/lua/fio.lua
@@ -6,6 +6,8 @@ local buffer = require('buffer')
local fiber = require('fiber')
local errno = require('errno')
local schedule_task = fiber._internal.schedule_task
+local cord_ibuf_take = buffer.internal.cord_ibuf_take
+local cord_ibuf_put = buffer.internal.cord_ibuf_put
ffi.cdef[[
int umask(int mask);
@@ -302,9 +304,12 @@ fio.dirname = function(path)
-- Can't just cast path to char * - on Linux dirname modifies
-- its argument.
local bsize = #path + 1
- local cpath = buffer.static_alloc('char', bsize)
+ local ibuf = cord_ibuf_take()
+ local cpath = ibuf:alloc(bsize)
ffi.copy(cpath, ffi.cast('const char *', path), bsize)
- return ffi.string(ffi.C.dirname(cpath))
+ path = ffi.string(ffi.C.dirname(cpath))
+ cord_ibuf_put(ibuf)
+ return path
end
fio.umask = function(umask)
diff --git a/src/lua/socket.lua b/src/lua/socket.lua
index 7c24b5655..3b2b13e19 100644
--- a/src/lua/socket.lua
+++ b/src/lua/socket.lua
@@ -12,8 +12,8 @@ local log = require('log')
local buffer = require('buffer')
local reg1 = buffer.reg1
local reg2 = buffer.reg2
-local static_alloc = buffer.static_alloc
local cord_ibuf_take = buffer.internal.cord_ibuf_take
+local cord_ibuf_put = buffer.internal.cord_ibuf_put
local cord_ibuf_drop = buffer.internal.cord_ibuf_drop
local format = string.format
@@ -540,15 +540,19 @@ local function socket_getsockopt(self, level, name)
end
if info.type == 2 then
- local value = static_alloc('char', 256)
+ local ibuf = cord_ibuf_take()
+ local value = ibuf:alloc(256)
local len = reg1.as
len[0] = 256
local res = ffi.C.getsockopt(fd, level, info.iname, value, len)
if res < 0 then
self._errno = boxerrno()
+ cord_ibuf_put(ibuf)
return nil
end
- return ffi.string(value, tonumber(len[0]))
+ value = ffi.string(value, tonumber(len[0]))
+ cord_ibuf_put(ibuf)
+ return value
end
if name == 'SO_LINGER' then
@@ -564,7 +568,7 @@ local function socket_linger(self, active, timeout)
local info = internal.SO_OPT[level].SO_LINGER
self._errno = nil
if active == nil then
- local value = static_alloc('linger_t')
+ local value = ffi.new('linger_t[1]')
local len = reg1.as
len[0] = ffi.sizeof('linger_t')
local res = ffi.C.getsockopt(fd, level, info.iname, value, len)
@@ -858,14 +862,18 @@ local function socket_recv(self, size, flags)
end
self._errno = nil
- local buf = static_alloc('char', size)
+ local ibuf = cord_ibuf_take()
+ local buf = ibuf:alloc(size)
local res = ffi.C.recv(fd, buf, size, iflags)
if res == -1 then
self._errno = boxerrno()
+ cord_ibuf_put(ibuf)
return nil
end
- return ffi.string(buf, res)
+ buf = ffi.string(buf, res)
+ cord_ibuf_put(ibuf)
+ return buf
end
local function socket_recvfrom(self, size, flags)
diff --git a/src/lua/string.lua b/src/lua/string.lua
index d3a846645..105110b52 100644
--- a/src/lua/string.lua
+++ b/src/lua/string.lua
@@ -1,6 +1,7 @@
local ffi = require('ffi')
local buffer = require('buffer')
-local static_alloc = buffer.static_alloc
+local cord_ibuf_take = buffer.internal.cord_ibuf_take
+local cord_ibuf_put = buffer.internal.cord_ibuf_put
ffi.cdef[[
const char *
@@ -291,13 +292,16 @@ local function string_hex(inp)
error(err_string_arg:format(1, 'string.hex', 'string', type(inp)), 2)
end
local len = inp:len() * 2
- local res = static_alloc('char', len + 1)
+ local ibuf = cord_ibuf_take()
+ local res = ibuf:alloc(len + 1)
local uinp = ffi.cast('const unsigned char *', inp)
for i = 0, inp:len() - 1 do
ffi.C.snprintf(res + i * 2, 3, "%02x", ffi.cast('unsigned', uinp[i]))
end
- return ffi.string(res, len)
+ res = ffi.string(res, len)
+ cord_ibuf_put(ibuf)
+ return res
end
local hexadecimal_chars = {
@@ -334,17 +338,21 @@ local function string_fromhex(inp)
end
local len = inp:len() / 2
local casted_inp = ffi.cast('const char *', inp)
- local res = static_alloc('char', len)
+ local ibuf = cord_ibuf_take()
+ local res = ibuf:alloc(len)
for i = 0, len - 1 do
local first = hexadecimals_mapping[casted_inp[i * 2]]
local second = hexadecimals_mapping[casted_inp[i * 2 + 1]]
if first == nil or second == nil then
+ cord_ibuf_put(ibuf)
error(err_string_arg:format(1, 'string.fromhex', 'hex string',
'non hex chars'), 2)
end
res[i] = first * 16 + second
end
- return ffi.string(res, len)
+ res = ffi.string(res, len)
+ cord_ibuf_put(ibuf)
+ return res
end
local function string_strip(inp, chars)
diff --git a/test/app-tap/module_api.test.lua b/test/app-tap/module_api.test.lua
index 28e33a02d..f0a864e02 100755
--- a/test/app-tap/module_api.test.lua
+++ b/test/app-tap/module_api.test.lua
@@ -2,6 +2,7 @@
local ffi = require('ffi')
local fio = require('fio')
+local ffi = require('ffi')
box.cfg{log = "tarantool.log"}
-- Use BUILDDIR passed from test-run or cwd when run w/o
@@ -43,7 +44,7 @@ local function test_buffers(test, module)
test:plan(9)
local buffer = require('buffer')
- local bufalloc = buffer.static_alloc("char", 128)
+ local bufalloc = ffi.new('char[?]', 128)
local ibuf = buffer.ibuf()
local pbuf = ibuf:alloc(128)
local ibuf_ptr = ffi.cast('struct ibuf *', ibuf)
diff --git a/test/app-tap/uri.test.lua b/test/app-tap/uri.test.lua
index dcddfb958..b1c21569f 100755
--- a/test/app-tap/uri.test.lua
+++ b/test/app-tap/uri.test.lua
@@ -3,7 +3,6 @@
local tap = require('tap')
local uri = require('uri')
local ffi = require('ffi')
-local static_alloc = require('buffer').static_alloc
local function test_parse(test)
-- Tests for uri.parse() Lua bindings.
@@ -66,28 +65,8 @@ local function test_format(test)
test:is(uri.format(u, true), "user:password@localhost", "password kept")
end
-local function test_static_alloc(test)
- -- gh-4779 uri.format returns junk output.
- -- As static allocator is also used in several places
- -- we should properly handle situation when output
- -- is zero-length string.
- -- Here we allocate the whole buffer,
- -- fill it with some "junk" bytes and
- -- check that result doesn't contain any of them.
- local buffer_size = 12288
- local buf = static_alloc('char', buffer_size)
- ffi.fill(buf, 512, string.byte('x'))
-
- local iterations = 10
- test:plan(iterations)
- for _ = 1, iterations do
- test:is(uri.format({}), '')
- end
-end
-
tap.test("uri", function(test)
- test:plan(3)
+ test:plan(2)
test:test("parse", test_parse)
test:test("format", test_format)
- test:test("static_alloc", test_static_alloc)
end)
diff --git a/test/app/buffer.result b/test/app/buffer.result
index beccc5a87..0934ae58f 100644
--- a/test/app/buffer.result
+++ b/test/app/buffer.result
@@ -34,20 +34,3 @@ reg1.u16
---
- 200
...
--- Alignment.
-_ = buffer.static_alloc('char') -- This makes buffer pos unaligned.
----
-...
-p = buffer.static_alloc('int')
----
-...
-ffi.cast('int', p) % ffi.alignof('int') == 0 -- But next alloc is aligned.
----
-- true
-...
--- Able to allocate bigger than static buffer - such allocations
--- are on the heap.
-type(buffer.static_alloc('char', 13000))
----
-- cdata
-...
diff --git a/test/app/buffer.test.lua b/test/app/buffer.test.lua
index a1c380680..1873e189e 100644
--- a/test/app/buffer.test.lua
+++ b/test/app/buffer.test.lua
@@ -12,12 +12,3 @@ u16[0]
u16[0] = 200
ffi.copy(reg1, u16, 2)
reg1.u16
-
--- Alignment.
-_ = buffer.static_alloc('char') -- This makes buffer pos unaligned.
-p = buffer.static_alloc('int')
-ffi.cast('int', p) % ffi.alignof('int') == 0 -- But next alloc is aligned.
-
--- Able to allocate bigger than static buffer - such allocations
--- are on the heap.
-type(buffer.static_alloc('char', 13000))
--
2.24.3 (Apple Git-128)
^ permalink raw reply [flat|nested] 32+ messages in thread* [Tarantool-patches] [PATCH 12/16] lua: use lua_pushfstring() instead of tt_sprintf()
2021-03-20 0:42 [Tarantool-patches] [PATCH 00/16] Cord buffer, static alloc, and Lua GC bug Vladislav Shpilevoy via Tarantool-patches
` (2 preceding siblings ...)
2021-03-20 0:42 ` [Tarantool-patches] [PATCH 11/16] buffer: remove static_alloc() from Lua Vladislav Shpilevoy via Tarantool-patches
@ 2021-03-20 0:42 ` Vladislav Shpilevoy via Tarantool-patches
2021-03-20 0:42 ` [Tarantool-patches] [PATCH 13/16] sio: rework sio_strfaddr() Vladislav Shpilevoy via Tarantool-patches
` (17 subsequent siblings)
21 siblings, 0 replies; 32+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-03-20 0:42 UTC (permalink / raw)
To: tarantool-patches, gorcunov, sergepetrenko
In a few places to push a formatted string was used 2 calls:
tt_sprintf() + lua_pushstring(). It wasn't necessary because Lua
API has lua_pushfstring() with a big enough subset of printf
format features.
But more importantly - it was a bug. lua_pushstring() is a GC
point. Before copying the passed string it tries to invoke Lua GC,
which might invoke a __gc handler for some cdata, where static
alloc might be used, and it can rewrite the string passed to
lua_pushstring() in the beginning of the stack.
Part of #5632
---
src/box/lua/space.cc | 6 ++----
src/lua/fiber.c | 2 +-
src/lua/utf8.c | 5 ++---
3 files changed, 5 insertions(+), 8 deletions(-)
diff --git a/src/box/lua/space.cc b/src/box/lua/space.cc
index 544a18f47..8d4d8cc23 100644
--- a/src/box/lua/space.cc
+++ b/src/box/lua/space.cc
@@ -568,8 +568,7 @@ lbox_space_frommap(struct lua_State *L)
space = space_by_id(id);
if (space == NULL) {
lua_pushnil(L);
- lua_pushstring(L, tt_sprintf("Space with id '%d' "\
- "doesn't exist", id));
+ lua_pushfstring(L, "Space with id '%d' doesn't exist", id);
return 2;
}
assert(space->format != NULL);
@@ -586,8 +585,7 @@ lbox_space_frommap(struct lua_State *L)
if (tuple_fieldno_by_name(dict, key, key_len, key_hash,
&fieldno)) {
lua_pushnil(L);
- lua_pushstring(L, tt_sprintf("Unknown field '%s'",
- key));
+ lua_pushfstring(L, "Unknown field '%s'", key);
return 2;
}
lua_rawseti(L, -3, fieldno+1);
diff --git a/src/lua/fiber.c b/src/lua/fiber.c
index edbd06ebc..02ec3d158 100644
--- a/src/lua/fiber.c
+++ b/src/lua/fiber.c
@@ -337,7 +337,7 @@ lbox_fiber_top_entry(struct fiber *f, void *cb_ctx)
{
struct lua_State *L = (struct lua_State *) cb_ctx;
- lua_pushstring(L, tt_sprintf("%u/%s", f->fid, f->name));
+ lua_pushfstring(L, "%f/%s", (lua_Number)f->fid, f->name);
lua_newtable(L);
diff --git a/src/lua/utf8.c b/src/lua/utf8.c
index bf9bb98f4..e060fec8f 100644
--- a/src/lua/utf8.c
+++ b/src/lua/utf8.c
@@ -81,9 +81,8 @@ utf8_str_to_case(struct lua_State *L, const char *src, int src_bsize,
} else {
cord_ibuf_put(ibuf);
lua_pushnil(L);
- lua_pushstring(L, tt_sprintf("error during ICU case "\
- "transform: %s",
- u_errorName(err)));
+ lua_pushfstring(L, "error during ICU case "
+ "transform: %s", u_errorName(err));
return 2;
}
/*
--
2.24.3 (Apple Git-128)
^ permalink raw reply [flat|nested] 32+ messages in thread* [Tarantool-patches] [PATCH 13/16] sio: rework sio_strfaddr()
2021-03-20 0:42 [Tarantool-patches] [PATCH 00/16] Cord buffer, static alloc, and Lua GC bug Vladislav Shpilevoy via Tarantool-patches
` (3 preceding siblings ...)
2021-03-20 0:42 ` [Tarantool-patches] [PATCH 12/16] lua: use lua_pushfstring() instead of tt_sprintf() Vladislav Shpilevoy via Tarantool-patches
@ 2021-03-20 0:42 ` Vladislav Shpilevoy via Tarantool-patches
2021-03-20 0:42 ` [Tarantool-patches] [PATCH 14/16] sio: increase SERVICE_NAME_MAXLEN size Vladislav Shpilevoy via Tarantool-patches
` (16 subsequent siblings)
21 siblings, 0 replies; 32+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-03-20 0:42 UTC (permalink / raw)
To: tarantool-patches, gorcunov, sergepetrenko
The function was overcomplicated, and made it harder to update it
in the next patches with functional changes.
The main source of the complication was usage of both inet_ntoa()
and getnameinfo(). The latter is more universal, it can cover the
case of the former.
The patch makes it use only getnameinfo() for IP addresses
regardless of v4 or v6.
Needed for #5632
---
src/lib/core/sio.c | 42 ++++++++++++++++--------------------------
1 file changed, 16 insertions(+), 26 deletions(-)
diff --git a/src/lib/core/sio.c b/src/lib/core/sio.c
index d008526d5..deffee86c 100644
--- a/src/lib/core/sio.c
+++ b/src/lib/core/sio.c
@@ -329,33 +329,23 @@ sio_getsockname(int fd, struct sockaddr *addr, socklen_t *addrlen)
const char *
sio_strfaddr(const struct sockaddr *addr, socklen_t addrlen)
{
- switch (addr->sa_family) {
- case AF_UNIX: {
- struct sockaddr_un *u = (struct sockaddr_un *) addr;
- if (addrlen >= sizeof(*u))
- return tt_sprintf("unix/:%s", u->sun_path);
- else
- return tt_sprintf("unix/:(socket)");
- break;
- }
- case AF_INET: {
- struct sockaddr_in *in = (struct sockaddr_in *) addr;
- return tt_snprintf(SERVICE_NAME_MAXLEN, "%s:%d",
- inet_ntoa(in->sin_addr),
- ntohs(in->sin_port));
- }
- default: {
- char host[NI_MAXHOST], serv[NI_MAXSERV];
- if (getnameinfo(addr, addrlen, host, sizeof(host),
- serv, sizeof(serv),
- NI_NUMERICHOST | NI_NUMERICSERV) != 0)
- return tt_sprintf("(host):(port)");
-
- return tt_snprintf(NI_MAXHOST + NI_MAXSERV, "[%s]:%s",
- host, serv);
- }
+ if (addr->sa_family == AF_UNIX) {
+ struct sockaddr_un *u = (struct sockaddr_un *)addr;
+ if (addrlen >= sizeof(*u))
+ return tt_sprintf("unix/:%s", u->sun_path);
+ else
+ return tt_sprintf("unix/:(socket)");
+ } else {
+ char host[NI_MAXHOST], serv[NI_MAXSERV];
+ int flags = NI_NUMERICHOST | NI_NUMERICSERV;
+ if (getnameinfo(addr, addrlen, host, sizeof(host), serv,
+ sizeof(serv), flags) != 0)
+ return tt_sprintf("(host):(port)");
+ else if (addr->sa_family == AF_INET)
+ return tt_sprintf("%s:%s", host, serv);
+ else
+ return tt_sprintf("[%s]:%s", host, serv);
}
- unreachable();
}
int
--
2.24.3 (Apple Git-128)
^ permalink raw reply [flat|nested] 32+ messages in thread* [Tarantool-patches] [PATCH 14/16] sio: increase SERVICE_NAME_MAXLEN size
2021-03-20 0:42 [Tarantool-patches] [PATCH 00/16] Cord buffer, static alloc, and Lua GC bug Vladislav Shpilevoy via Tarantool-patches
` (4 preceding siblings ...)
2021-03-20 0:42 ` [Tarantool-patches] [PATCH 13/16] sio: rework sio_strfaddr() Vladislav Shpilevoy via Tarantool-patches
@ 2021-03-20 0:42 ` Vladislav Shpilevoy via Tarantool-patches
2021-03-21 21:58 ` Cyrill Gorcunov via Tarantool-patches
2021-03-20 0:42 ` [Tarantool-patches] [PATCH 15/16] sio: introduce and use sio_snprintf() Vladislav Shpilevoy via Tarantool-patches
` (15 subsequent siblings)
21 siblings, 1 reply; 32+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-03-20 0:42 UTC (permalink / raw)
To: tarantool-patches, gorcunov, sergepetrenko
It was 32, and couldn't fit long IPv6 and Unix socket addresses.
The patch makes it 200 so now it fits any supported addresses
family used in the code.
Having SERVICE_NAME_MAXLEN valid is necessary to be able to save
a complete address string on the stack in the places where the
static buffer returned by sio_strfaddr() can't be used safely. For
instance, in the code working with Lua due to Lua GC which might
be invoked any moment and in a __gc handler could overwrite the
static buffer.
Needed for #5632
---
src/lib/core/sio.c | 2 +-
src/lib/core/sio.h | 11 ++++++++++-
2 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/src/lib/core/sio.c b/src/lib/core/sio.c
index deffee86c..25c34ea59 100644
--- a/src/lib/core/sio.c
+++ b/src/lib/core/sio.c
@@ -78,7 +78,7 @@ sio_socketname(int fd)
{
/* Preserve errno */
int save_errno = errno;
- int name_size = 2 * SERVICE_NAME_MAXLEN;
+ int name_size = SERVICE_NAME_MAXLEN;
char *name = static_alloc(name_size);
int rc = sio_socketname_to_buffer(fd, name, name_size);
/*
diff --git a/src/lib/core/sio.h b/src/lib/core/sio.h
index db2e3281f..4f5a7f6f3 100644
--- a/src/lib/core/sio.h
+++ b/src/lib/core/sio.h
@@ -52,7 +52,16 @@
extern "C" {
#endif /* defined(__cplusplus) */
-enum { SERVICE_NAME_MAXLEN = 32 };
+enum {
+ /**
+ * - Unix socket path is 108 bytes max;
+ * - IP(v4, v6) max string len is 45;
+ *
+ * Max result is rounded up just in case the numbers are a bit different
+ * on various platforms.
+ */
+ SERVICE_NAME_MAXLEN = 200,
+};
/**
* Check if an errno, returned from a sio function, means a
--
2.24.3 (Apple Git-128)
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [Tarantool-patches] [PATCH 14/16] sio: increase SERVICE_NAME_MAXLEN size
2021-03-20 0:42 ` [Tarantool-patches] [PATCH 14/16] sio: increase SERVICE_NAME_MAXLEN size Vladislav Shpilevoy via Tarantool-patches
@ 2021-03-21 21:58 ` Cyrill Gorcunov via Tarantool-patches
2021-03-22 22:32 ` Vladislav Shpilevoy via Tarantool-patches
0 siblings, 1 reply; 32+ messages in thread
From: Cyrill Gorcunov via Tarantool-patches @ 2021-03-21 21:58 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
On Sat, Mar 20, 2021 at 01:42:36AM +0100, Vladislav Shpilevoy wrote:
>
> -enum { SERVICE_NAME_MAXLEN = 32 };
> +enum {
> + /**
> + * - Unix socket path is 108 bytes max;
> + * - IP(v4, v6) max string len is 45;
Actually IPv6 may include a scope as well. Currently at least
libc indeed does limit it to 45 symbols plus eos mark so we're
safe. Still 200 seems to be much over the top, maybe 128 instead?
Also pow2 gonna be easier to manage by any memory manager.
I don't insist though just a thought and this could be tuned up
on top.
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [Tarantool-patches] [PATCH 14/16] sio: increase SERVICE_NAME_MAXLEN size
2021-03-21 21:58 ` Cyrill Gorcunov via Tarantool-patches
@ 2021-03-22 22:32 ` Vladislav Shpilevoy via Tarantool-patches
2021-03-23 6:56 ` Cyrill Gorcunov via Tarantool-patches
0 siblings, 1 reply; 32+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-03-22 22:32 UTC (permalink / raw)
To: Cyrill Gorcunov; +Cc: tarantool-patches
Hi! Thanks for the review!
On 21.03.2021 22:58, Cyrill Gorcunov wrote:
> On Sat, Mar 20, 2021 at 01:42:36AM +0100, Vladislav Shpilevoy wrote:
>>
>> -enum { SERVICE_NAME_MAXLEN = 32 };
>> +enum {
>> + /**
>> + * - Unix socket path is 108 bytes max;
>> + * - IP(v4, v6) max string len is 45;
>
> Actually IPv6 may include a scope as well. Currently at least
> libc indeed does limit it to 45 symbols plus eos mark so we're
> safe. Still 200 seems to be much over the top, maybe 128 instead?
I decided to be paranoid and made it 200 deliberately. Anyway it is
not used in any swarm allocations. 128 seems right on the edge. You
can have 108 bytes of Unix socket path, + strlen("unix/:"). It is
already 114.
Also it is used for sio_socketname(), where you have addr str (114)
+ strlen(", aka ")(6) + strlen(", peer of ")(10), which is already
130 total.
> Also pow2 gonna be easier to manage by any memory manager.
It would matter if the buffer would be ever allocated on its own.
But it is always either on the stack, or inside of another structure,
where total size is not a power of 2 anyway, most likely.
> I don't insist though just a thought and this could be tuned up
> on top.
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [Tarantool-patches] [PATCH 14/16] sio: increase SERVICE_NAME_MAXLEN size
2021-03-22 22:32 ` Vladislav Shpilevoy via Tarantool-patches
@ 2021-03-23 6:56 ` Cyrill Gorcunov via Tarantool-patches
0 siblings, 0 replies; 32+ messages in thread
From: Cyrill Gorcunov via Tarantool-patches @ 2021-03-23 6:56 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
On Mon, Mar 22, 2021 at 11:32:14PM +0100, Vladislav Shpilevoy wrote:
> >
> > Actually IPv6 may include a scope as well. Currently at least
> > libc indeed does limit it to 45 symbols plus eos mark so we're
> > safe. Still 200 seems to be much over the top, maybe 128 instead?
>
> I decided to be paranoid and made it 200 deliberately. Anyway it is
> not used in any swarm allocations. 128 seems right on the edge. You
> can have 108 bytes of Unix socket path, + strlen("unix/:"). It is
> already 114.
>
> Also it is used for sio_socketname(), where you have addr str (114)
> + strlen(", aka ")(6) + strlen(", peer of ")(10), which is already
> 130 total.
Aha! Thanks for explanation.
> > Also pow2 gonna be easier to manage by any memory manager.
>
> It would matter if the buffer would be ever allocated on its own.
> But it is always either on the stack, or inside of another structure,
> where total size is not a power of 2 anyway, most likely.
FWIW the data on stack always allocated by size of stack line, ie pow2.
But dosn't matter, 200 should be fine, thanks!
^ permalink raw reply [flat|nested] 32+ messages in thread
* [Tarantool-patches] [PATCH 15/16] sio: introduce and use sio_snprintf()
2021-03-20 0:42 [Tarantool-patches] [PATCH 00/16] Cord buffer, static alloc, and Lua GC bug Vladislav Shpilevoy via Tarantool-patches
` (5 preceding siblings ...)
2021-03-20 0:42 ` [Tarantool-patches] [PATCH 14/16] sio: increase SERVICE_NAME_MAXLEN size Vladislav Shpilevoy via Tarantool-patches
@ 2021-03-20 0:42 ` Vladislav Shpilevoy via Tarantool-patches
2021-03-20 0:42 ` [Tarantool-patches] [PATCH 16/16] buffer: remove Lua registers Vladislav Shpilevoy via Tarantool-patches
` (14 subsequent siblings)
21 siblings, 0 replies; 32+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-03-20 0:42 UTC (permalink / raw)
To: tarantool-patches, gorcunov, sergepetrenko
sio_strfaddr() can't be used in the places where static buffer
is not acceptable - in any code which wants to push the value to
Lua, or the address string must be long living.
The patch introduces sio_snprintf(), which does the same, but
saves the result into a provided buffer with a limited size.
In the Lua C code the patch saves the address string on the stack
which makes it safe against Lua GC interruptions.
Part of #5632
---
src/box/iproto.cc | 8 +++++---
src/box/iproto.h | 2 +-
src/box/lua/info.c | 5 +++--
src/box/lua/session.c | 7 +++++--
src/lib/core/sio.c | 40 ++++++++++++++++++++++++++++------------
src/lib/core/sio.h | 4 ++++
6 files changed, 46 insertions(+), 20 deletions(-)
diff --git a/src/box/iproto.cc b/src/box/iproto.cc
index f7330af21..238842e17 100644
--- a/src/box/iproto.cc
+++ b/src/box/iproto.cc
@@ -138,12 +138,14 @@ static struct sockaddr_storage iproto_bound_address_storage;
static socklen_t iproto_bound_address_len;
const char *
-iproto_bound_address(void)
+iproto_bound_address(char *buf)
{
if (iproto_bound_address_len == 0)
return NULL;
- return sio_strfaddr((struct sockaddr *) &iproto_bound_address_storage,
- iproto_bound_address_len);
+ sio_addr_snprintf(buf, SERVICE_NAME_MAXLEN,
+ (struct sockaddr *) &iproto_bound_address_storage,
+ iproto_bound_address_len);
+ return buf;
}
/**
diff --git a/src/box/iproto.h b/src/box/iproto.h
index 392e4f08e..f6f7101a1 100644
--- a/src/box/iproto.h
+++ b/src/box/iproto.h
@@ -85,7 +85,7 @@ iproto_reset_stat(void);
* iproto. To be shown in box.info.
*/
const char *
-iproto_bound_address(void);
+iproto_bound_address(char *buf);
#if defined(__cplusplus)
} /* extern "C" */
diff --git a/src/box/lua/info.c b/src/box/lua/info.c
index c4c9fa0a0..1e0c0148a 100644
--- a/src/box/lua/info.c
+++ b/src/box/lua/info.c
@@ -52,7 +52,7 @@
#include "box/raft.h"
#include "lua/utils.h"
#include "fiber.h"
-#include "tt_static.h"
+#include "sio.h"
static void
lbox_pushvclock(struct lua_State *L, const struct vclock *vclock)
@@ -574,7 +574,8 @@ static int
lbox_info_listen(struct lua_State *L)
{
/* NULL is ok, no need to check. */
- lua_pushstring(L, iproto_bound_address());
+ char addrbuf[SERVICE_NAME_MAXLEN];
+ lua_pushstring(L, iproto_bound_address(addrbuf));
return 1;
}
diff --git a/src/box/lua/session.c b/src/box/lua/session.c
index 0a20aaad1..ae8c7094b 100644
--- a/src/box/lua/session.c
+++ b/src/box/lua/session.c
@@ -273,10 +273,13 @@ lbox_session_peer(struct lua_State *L)
struct sockaddr_storage addr;
socklen_t addrlen = sizeof(addr);
- if (sio_getpeername(fd, (struct sockaddr *)&addr, &addrlen) < 0)
+ struct sockaddr *addr_base = (struct sockaddr *)&addr;
+ if (sio_getpeername(fd, addr_base, &addrlen) < 0)
luaL_error(L, "session.peer(): getpeername() failed");
- lua_pushstring(L, sio_strfaddr((struct sockaddr *)&addr, addrlen));
+ char addrbuf[SERVICE_NAME_MAXLEN];
+ sio_addr_snprintf(addrbuf, sizeof(addrbuf), addr_base, addrlen);
+ lua_pushstring(L, addrbuf);
return 1;
}
diff --git a/src/lib/core/sio.c b/src/lib/core/sio.c
index 25c34ea59..6d1732332 100644
--- a/src/lib/core/sio.c
+++ b/src/lib/core/sio.c
@@ -59,16 +59,17 @@ sio_socketname_to_buffer(int fd, char *buf, int size)
return 0;
struct sockaddr_storage addr;
socklen_t addrlen = sizeof(addr);
- int rc = getsockname(fd, (struct sockaddr *) &addr, &addrlen);
+ struct sockaddr *base_addr = (struct sockaddr *)&addr;
+ int rc = getsockname(fd, base_addr, &addrlen);
if (rc == 0) {
- SNPRINT(n, snprintf, buf, size, ", aka %s",
- sio_strfaddr((struct sockaddr *)&addr, addrlen));
+ SNPRINT(n, snprintf, buf, size, ", aka ");
+ SNPRINT(n, sio_addr_snprintf, buf, size, base_addr, addrlen);
}
addrlen = sizeof(addr);
rc = getpeername(fd, (struct sockaddr *) &addr, &addrlen);
if (rc == 0) {
- SNPRINT(n, snprintf, buf, size, ", peer of %s",
- sio_strfaddr((struct sockaddr *)&addr, addrlen));
+ SNPRINT(n, snprintf, buf, size, ", peer of ");
+ SNPRINT(n, sio_addr_snprintf, buf, size, base_addr, addrlen);
}
return 0;
}
@@ -326,26 +327,41 @@ sio_getsockname(int fd, struct sockaddr *addr, socklen_t *addrlen)
return 0;
}
-const char *
-sio_strfaddr(const struct sockaddr *addr, socklen_t addrlen)
+int
+sio_addr_snprintf(char *buf, size_t size, const struct sockaddr *addr,
+ socklen_t addrlen)
{
+ int res;
if (addr->sa_family == AF_UNIX) {
struct sockaddr_un *u = (struct sockaddr_un *)addr;
if (addrlen >= sizeof(*u))
- return tt_sprintf("unix/:%s", u->sun_path);
+ res = snprintf(buf, size, "unix/:%s", u->sun_path);
else
- return tt_sprintf("unix/:(socket)");
+ res = snprintf(buf, size, "unix/:(socket)");
} else {
char host[NI_MAXHOST], serv[NI_MAXSERV];
int flags = NI_NUMERICHOST | NI_NUMERICSERV;
if (getnameinfo(addr, addrlen, host, sizeof(host), serv,
sizeof(serv), flags) != 0)
- return tt_sprintf("(host):(port)");
+ res = snprintf(buf, size, "(host):(port)");
else if (addr->sa_family == AF_INET)
- return tt_sprintf("%s:%s", host, serv);
+ res = snprintf(buf, size, "%s:%s", host, serv);
else
- return tt_sprintf("[%s]:%s", host, serv);
+ res = snprintf(buf, size, "[%s]:%s", host, serv);
}
+ assert(res + 1 < SERVICE_NAME_MAXLEN);
+ assert(res >= 0);
+ return res;
+}
+
+const char *
+sio_strfaddr(const struct sockaddr *addr, socklen_t addrlen)
+{
+ int size = SERVICE_NAME_MAXLEN;
+ char *buf = (char *) static_reserve(size);
+ /* +1 for terminating 0. */
+ static_alloc(sio_addr_snprintf(buf, size, addr, addrlen) + 1);
+ return buf;
}
int
diff --git a/src/lib/core/sio.h b/src/lib/core/sio.h
index 4f5a7f6f3..016f1f079 100644
--- a/src/lib/core/sio.h
+++ b/src/lib/core/sio.h
@@ -73,6 +73,10 @@ sio_wouldblock(int err)
return err == EAGAIN || err == EWOULDBLOCK || err == EINTR;
}
+/** Format the address into the given buffer. Behaves like snprintf(). */
+int
+sio_addr_snprintf(char *buf, size_t size, const struct sockaddr *addr,
+ socklen_t addrlen);
/**
* Format the address provided in struct sockaddr *addr.
--
2.24.3 (Apple Git-128)
^ permalink raw reply [flat|nested] 32+ messages in thread* [Tarantool-patches] [PATCH 16/16] buffer: remove Lua registers
2021-03-20 0:42 [Tarantool-patches] [PATCH 00/16] Cord buffer, static alloc, and Lua GC bug Vladislav Shpilevoy via Tarantool-patches
` (6 preceding siblings ...)
2021-03-20 0:42 ` [Tarantool-patches] [PATCH 15/16] sio: introduce and use sio_snprintf() Vladislav Shpilevoy via Tarantool-patches
@ 2021-03-20 0:42 ` Vladislav Shpilevoy via Tarantool-patches
2021-03-20 0:42 ` [Tarantool-patches] [PATCH 02/16] test: don't use IBUF_SHARED in the tests Vladislav Shpilevoy via Tarantool-patches
` (13 subsequent siblings)
21 siblings, 0 replies; 32+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-03-20 0:42 UTC (permalink / raw)
To: tarantool-patches, gorcunov, sergepetrenko
Lua buffer module used to have a couple of preallocated objects of
type 'union c_register'. It was a bunch of C scalar and array
types intended for use instead of ffi.new() where it was needed to
allocate a temporary object like 'int[1]' just to be able to pass
'int *' into a C function via FFI.
It was a bit faster than ffi.new() even for small sizes. For
instance (when JIT works), getting a register to use it as
'int[1]' cost around 0.2-0.3 ns while ffi.new('int[1]') costs
around 0.4 ns. Also the code looked cleaner.
But Lua registers were global and therefore had the same issue as
IBUF_SHARED and static_alloc() in Lua - no ownership, and sudden
reuse when GC starts right the register is still in use in some
Lua code. __gc handlers could wipe the register values making the
original code behave unpredictably.
IBUF_SHARED was fixed by proper ownership implementation, but it
is not necessary with Lua registers. It could be done with the
buffer.ffi_stash_new() feature, but its performance is about 0.8
ns which is worse than plain ffi.new() for simple scalar types.
This patch eliminates Lua registers, and uses ffi.new() instead
everywhere.
Closes #5632
---
changelogs/unreleased/fix-ibuf-static.md | 7 +++
src/box/lua/net_box.lua | 4 +-
src/box/lua/schema.lua | 2 +-
src/lua/buffer.lua | 43 -------------------
src/lua/crypto.lua | 11 ++---
src/lua/msgpackffi.lua | 50 ++++++++++++----------
src/lua/socket.lua | 20 +++------
src/lua/string.lua | 8 +++-
src/lua/swim.lua | 2 +-
test/app-tap/gh-5632-gc-buf-reuse.test.lua | 46 +++++++++++++++++++-
test/app-tap/module_api.test.lua | 3 +-
test/app/buffer.result | 36 ----------------
test/app/buffer.test.lua | 14 ------
13 files changed, 102 insertions(+), 144 deletions(-)
create mode 100644 changelogs/unreleased/fix-ibuf-static.md
delete mode 100644 test/app/buffer.result
delete mode 100644 test/app/buffer.test.lua
diff --git a/changelogs/unreleased/fix-ibuf-static.md b/changelogs/unreleased/fix-ibuf-static.md
new file mode 100644
index 000000000..34450b85d
--- /dev/null
+++ b/changelogs/unreleased/fix-ibuf-static.md
@@ -0,0 +1,7 @@
+## bugfix/core
+
+* Extensive usage of `uri` and `uuid` modules with debug log level could lead to
+ a crash or corrupted result of the functions from these modules. Also their
+ usage from the callbacks passed to `ffi.gc()` could lead to the same but much
+ easier. The same could happen with some functions from the modules `fio`,
+ `box.tuple`, `iconv` (gh-5632).
diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
index de25026a5..3878abf21 100644
--- a/src/box/lua/net_box.lua
+++ b/src/box/lua/net_box.lua
@@ -15,7 +15,6 @@ local fiber_clock = fiber.clock
local fiber_self = fiber.self
local decode = msgpack.decode_unchecked
local decode_map_header = msgpack.decode_map_header
-local buffer_reg = buffer.reg1
local table_new = require('table.new')
local check_iterator_type = box.internal.check_iterator_type
@@ -147,8 +146,7 @@ local method_decoder = {
}
local function decode_error(raw_data)
- local ptr = buffer_reg.acucp
- ptr[0] = raw_data
+ local ptr = ffi.new('const char *[1]', raw_data)
local err = ffi.C.error_unpack_unsafe(ptr)
if err ~= nil then
err._refs = err._refs + 1
diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
index b345b3050..96503a50e 100644
--- a/src/box/lua/schema.lua
+++ b/src/box/lua/schema.lua
@@ -2234,7 +2234,7 @@ sequence_mt.next = function(self)
end
sequence_mt.current = function(self)
- local ai64 = buffer.reg1.ai64
+ local ai64 = ffi.new('int64_t[1]')
local rc = builtin.box_sequence_current(self.id, ai64)
if rc < 0 then
box.error(box.error.last())
diff --git a/src/lua/buffer.lua b/src/lua/buffer.lua
index 4d0bef91a..182c0b015 100644
--- a/src/lua/buffer.lua
+++ b/src/lua/buffer.lua
@@ -41,35 +41,6 @@ ibuf_reinit(struct ibuf *ibuf);
void *
ibuf_reserve_slow(struct ibuf *ibuf, size_t size);
-
-/**
- * Register is a buffer to use with FFI functions, which usually
- * operate with pointers to scalar values like int, char, size_t,
- * void *. To avoid doing 'ffi.new(<type>[1])' on each such FFI
- * function invocation, a module can use one of attributes of the
- * register union.
- *
- * Naming policy of the attributes is easy to remember:
- * 'a' for array type + type name first letters + 'p' for pointer.
- *
- * For example:
- * - int[1] - <a>rray of <i>nt - ai;
- * - const unsigned char *[1] -
- * <a>rray of <c>onst <u>nsigned <c>har <p> pointer - acucp.
- */
-union c_register {
- size_t as[1];
- void *ap[1];
- int ai[1];
- char ac[1];
- const unsigned char *acucp[1];
- unsigned long aul[1];
- uint16_t u16;
- uint32_t u32;
- uint64_t u64;
- int64_t i64;
- int64_t ai64[1];
-};
]]
local builtin = ffi.C
@@ -260,16 +231,6 @@ local function ffi_stash_new(c_type)
}
end
---
--- Sometimes it is wanted to use several temporary registers at
--- once. For example, when a C function takes 2 C pointers. Then
--- one register is not enough - its attributes share memory. Note,
--- registers are not allocated with separate ffi.new calls
--- deliberately. With a single allocation they fit into 1 cache
--- line, and reduce the heap fragmentation.
---
-local reg_array = ffi.new('union c_register[?]', 2)
-
--
-- Cord buffer is useful for the places, where
--
@@ -313,9 +274,5 @@ return {
internal = internal,
ibuf = ibuf_new;
READAHEAD = READAHEAD;
- -- Keep reference.
- reg_array = reg_array,
- reg1 = reg_array[0],
- reg2 = reg_array[1],
ffi_stash_new = ffi_stash_new,
}
diff --git a/src/lua/crypto.lua b/src/lua/crypto.lua
index 5a219cf30..6a62368b5 100644
--- a/src/lua/crypto.lua
+++ b/src/lua/crypto.lua
@@ -2,7 +2,6 @@
local ffi = require('ffi')
local buffer = require('buffer')
-local reg = buffer.reg1
local cord_ibuf_take = buffer.internal.cord_ibuf_take
local cord_ibuf_put = buffer.internal.cord_ibuf_put
@@ -134,10 +133,11 @@ local function digest_final(self)
return error('Digest not initialized')
end
self.initialized = false
- if ffi.C.EVP_DigestFinal_ex(self.ctx, self.buf.wpos, reg.ai) ~= 1 then
+ local ai = ffi.new('int[1]')
+ if ffi.C.EVP_DigestFinal_ex(self.ctx, self.buf.wpos, ai) ~= 1 then
return error('Can\'t finalize digest: ' .. openssl_err_str())
end
- return ffi.string(self.buf.wpos, reg.ai[0])
+ return ffi.string(self.buf.wpos, ai[0])
end
local function digest_free(self)
@@ -208,11 +208,12 @@ local function hmac_final(self)
self.initialized = false
local ibuf = cord_ibuf_take()
local buf = ibuf:alloc(64)
- if ffi.C.HMAC_Final(self.ctx, buf, reg.ai) ~= 1 then
+ local ai = ffi.new('int[1]')
+ if ffi.C.HMAC_Final(self.ctx, buf, ai) ~= 1 then
cord_ibuf_put(ibuf)
return error('Can\'t finalize HMAC: ' .. openssl_err_str())
end
- buf = ffi.string(buf, reg.ai[0])
+ buf = ffi.string(buf, ai[0])
cord_ibuf_put(ibuf)
return buf
end
diff --git a/src/lua/msgpackffi.lua b/src/lua/msgpackffi.lua
index 6c245d7aa..1d54f11b8 100644
--- a/src/lua/msgpackffi.lua
+++ b/src/lua/msgpackffi.lua
@@ -39,7 +39,6 @@ uuid_unpack(const char **data, uint32_t len, struct tt_uuid *uuid);
]])
local strict_alignment = (jit.arch == 'arm')
-local reg = buffer.reg1
local function bswap_u16(num)
return bit.rshift(bit.bswap(tonumber(num)), 16)
@@ -76,10 +75,10 @@ end
local encode_u16
if strict_alignment then
encode_u16 = function(buf, code, num)
- reg.u16 = bswap_u16(num)
+ local u16 = ffi.new('uint16_t[1]', bswap_u16(num))
local p = buf:alloc(3)
p[0] = code
- ffi.copy(p + 1, reg, 2)
+ ffi.copy(p + 1, u16, 2)
end
else
encode_u16 = function(buf, code, num)
@@ -92,10 +91,11 @@ end
local encode_u32
if strict_alignment then
encode_u32 = function(buf, code, num)
- reg.u32 = ffi.cast('uint32_t', bit.bswap(tonumber(num)))
+ local u32 = ffi.new('uint32_t[1]',
+ ffi.cast('uint32_t', bit.bswap(tonumber(num))))
local p = buf:alloc(5)
p[0] = code
- ffi.copy(p + 1, reg, 4)
+ ffi.copy(p + 1, u32, 4)
end
else
encode_u32 = function(buf, code, num)
@@ -109,10 +109,10 @@ end
local encode_u64
if strict_alignment then
encode_u64 = function(buf, code, num)
- reg.u64 = bit.bswap(ffi.cast('uint64_t', num))
+ local u64 = ffi.new('uint64_t[1]', bit.bswap(ffi.cast('uint64_t', num)))
local p = buf:alloc(9)
p[0] = code
- ffi.copy(p + 1, reg, 8)
+ ffi.copy(p + 1, u64, 8)
end
else
encode_u64 = function(buf, code, num)
@@ -339,9 +339,10 @@ end
local decode_u16
if strict_alignment then
decode_u16 = function(data)
- ffi.copy(reg, data[0], 2)
+ local u16 = ffi.new('uint16_t[1]')
+ ffi.copy(u16, data[0], 2)
data[0] = data[0] + 2
- return tonumber(bswap_u16(reg.u16))
+ return tonumber(bswap_u16(u16[0]))
end
else
decode_u16 = function(data)
@@ -354,10 +355,11 @@ end
local decode_u32
if strict_alignment then
decode_u32 = function(data)
- ffi.copy(reg, data[0], 4)
+ local u32 = ffi.new('uint32_t[1]')
+ ffi.copy(u32, data[0], 4)
data[0] = data[0] + 4
return tonumber(
- ffi.cast('uint32_t', bit.bswap(tonumber(reg.u32))))
+ ffi.cast('uint32_t', bit.bswap(tonumber(u32[0]))))
end
else
decode_u32 = function(data)
@@ -371,9 +373,10 @@ end
local decode_u64
if strict_alignment then
decode_u64 = function(data)
- ffi.copy(reg, data[0], 8);
+ local u64 = ffi.new('uint64_t[1]')
+ ffi.copy(u64, data[0], 8);
data[0] = data[0] + 8
- local num = bit.bswap(reg.u64)
+ local num = bit.bswap(u64[0])
if num <= DBL_INT_MAX then
return tonumber(num) -- return as 'number'
end
@@ -400,8 +403,9 @@ end
local decode_i16
if strict_alignment then
decode_i16 = function(data)
- ffi.copy(reg, data[0], 2)
- local num = bswap_u16(reg.u16)
+ local u16 = ffi.new('uint16_t[1]')
+ ffi.copy(u16, data[0], 2)
+ local num = bswap_u16(u16[0])
data[0] = data[0] + 2
-- note: this double cast is actually necessary
return tonumber(ffi.cast('int16_t', ffi.cast('uint16_t', num)))
@@ -418,8 +422,9 @@ end
local decode_i32
if strict_alignment then
decode_i32 = function(data)
- ffi.copy(reg, data[0], 4)
- local num = bit.bswap(tonumber(reg.u32))
+ local u32 = ffi.new('uint32_t[1]')
+ ffi.copy(u32, data[0], 4)
+ local num = bit.bswap(tonumber(u32[0]))
data[0] = data[0] + 4
return num
end
@@ -434,9 +439,10 @@ end
local decode_i64
if strict_alignment then
decode_i64 = function(data)
- ffi.copy(reg, data[0], 8)
+ local i64 = ffi.new('int64_t[1]')
+ ffi.copy(i64, data[0], 8)
data[0] = data[0] + 8
- local num = bit.bswap(reg.i64)
+ local num = bit.bswap(i64[0])
if num >= -DBL_INT_MAX and num <= DBL_INT_MAX then
return tonumber(num) -- return as 'number'
end
@@ -588,13 +594,11 @@ decode_r = function(data)
end
---
--- A temporary const char ** buffer.
-- All decode_XXX functions accept const char **data as its first argument,
-- like libmsgpuck does. After decoding data[0] position is changed to the next
-- element. It is significally faster on LuaJIT to use double pointer than
-- return result, newpos.
--
-local bufp = reg.acucp;
local function check_offset(offset, len)
if offset == nil then
@@ -614,13 +618,13 @@ local function decode_unchecked(str, offset)
if type(str) == "string" then
offset = check_offset(offset, #str)
local buf = ffi.cast(char_ptr_t, str)
- bufp[0] = buf + offset - 1
+ local bufp = ffi.new('const unsigned char *[1]', buf + offset - 1)
local r = decode_r(bufp)
return r, bufp[0] - buf + 1
elseif ffi.istype(char_ptr_t, str) then
-- Note: ffi.istype() ignores the const qualifier, so both
-- (char *) and (const char *) buffers are valid.
- bufp[0] = str
+ local bufp = ffi.new('const unsigned char *[1]', str)
local r = decode_r(bufp)
return r, ffi.cast(ffi.typeof(str), bufp[0])
else
diff --git a/src/lua/socket.lua b/src/lua/socket.lua
index 3b2b13e19..cda62d171 100644
--- a/src/lua/socket.lua
+++ b/src/lua/socket.lua
@@ -10,8 +10,6 @@ local fiber = require('fiber')
local fio = require('fio')
local log = require('log')
local buffer = require('buffer')
-local reg1 = buffer.reg1
-local reg2 = buffer.reg2
local cord_ibuf_take = buffer.internal.cord_ibuf_take
local cord_ibuf_put = buffer.internal.cord_ibuf_put
local cord_ibuf_drop = buffer.internal.cord_ibuf_drop
@@ -478,9 +476,9 @@ local function socket_setsockopt(self, level, name, value)
end
if info.type == 1 then
- reg1.ai[0] = value
+ local ai = ffi.new('int[1]', value)
local res = ffi.C.setsockopt(fd,
- level, info.iname, reg1.ai, ffi.sizeof('int'))
+ level, info.iname, ai, ffi.sizeof('int'))
if res < 0 then
self._errno = boxerrno()
@@ -522,10 +520,8 @@ local function socket_getsockopt(self, level, name)
self._errno = nil
if info.type == 1 then
- local value = reg1.ai
- value[0] = 0
- local len = reg2.as
- len[0] = ffi.sizeof('int')
+ local value = ffi.new('int[1]')
+ local len = ffi.new('size_t[1]', ffi.sizeof('int'))
local res = ffi.C.getsockopt(fd, level, info.iname, value, len)
if res < 0 then
@@ -542,8 +538,7 @@ local function socket_getsockopt(self, level, name)
if info.type == 2 then
local ibuf = cord_ibuf_take()
local value = ibuf:alloc(256)
- local len = reg1.as
- len[0] = 256
+ local len = ffi.new('size_t[1]', 256)
local res = ffi.C.getsockopt(fd, level, info.iname, value, len)
if res < 0 then
self._errno = boxerrno()
@@ -569,8 +564,7 @@ local function socket_linger(self, active, timeout)
self._errno = nil
if active == nil then
local value = ffi.new('linger_t[1]')
- local len = reg1.as
- len[0] = ffi.sizeof('linger_t')
+ local len = ffi.new('size_t[1]', ffi.sizeof('linger_t'))
local res = ffi.C.getsockopt(fd, level, info.iname, value, len)
if res < 0 then
self._errno = boxerrno()
@@ -825,7 +819,7 @@ local function get_recv_size(self, size)
-- them using message peek.
local iflags = get_iflags(internal.SEND_FLAGS, {'MSG_PEEK'})
assert(iflags ~= nil)
- size = tonumber(ffi.C.recv(fd, reg1.ac, 1, iflags))
+ size = tonumber(ffi.C.recv(fd, ffi.new('char[1]'), 1, iflags))
-- Prevent race condition: proceed with the case when
-- a datagram of length > 0 has been arrived after the
-- getsockopt call above.
diff --git a/src/lua/string.lua b/src/lua/string.lua
index 105110b52..5b0036da4 100644
--- a/src/lua/string.lua
+++ b/src/lua/string.lua
@@ -16,8 +16,6 @@ ffi.cdef[[
]]
local c_char_ptr = ffi.typeof('const char *')
-local strip_newstart = buffer.reg1.aul
-local strip_newlen = buffer.reg2.aul
local memcmp = ffi.C.memcmp
local memmem = ffi.C.memmem
@@ -371,6 +369,8 @@ local function string_strip(inp, chars)
end
local casted_inp = c_char_ptr(inp)
+ local strip_newstart = ffi.new('unsigned long[1]')
+ local strip_newlen = ffi.new('unsigned long[1]')
ffi.C.string_strip_helper(inp, #inp, chars, #chars, true, true,
strip_newstart, strip_newlen)
return ffi.string(casted_inp + strip_newstart[0], strip_newlen[0])
@@ -392,6 +392,8 @@ local function string_lstrip(inp, chars)
end
local casted_inp = c_char_ptr(inp)
+ local strip_newstart = ffi.new('unsigned long[1]')
+ local strip_newlen = ffi.new('unsigned long[1]')
ffi.C.string_strip_helper(inp, #inp, chars, #chars, true, false,
strip_newstart, strip_newlen)
return ffi.string(casted_inp + strip_newstart[0], strip_newlen[0])
@@ -413,6 +415,8 @@ local function string_rstrip(inp, chars)
end
local casted_inp = c_char_ptr(inp)
+ local strip_newstart = ffi.new('unsigned long[1]')
+ local strip_newlen = ffi.new('unsigned long[1]')
ffi.C.string_strip_helper(inp, #inp, chars, #chars, false, true,
strip_newstart, strip_newlen)
return ffi.string(casted_inp + strip_newstart[0], strip_newlen[0])
diff --git a/src/lua/swim.lua b/src/lua/swim.lua
index 42b0d15ef..f61aa6b58 100644
--- a/src/lua/swim.lua
+++ b/src/lua/swim.lua
@@ -348,7 +348,7 @@ local function swim_member_is_dropped(m)
end
local function swim_member_payload_raw(ptr)
- local int = buffer.reg1.ai
+ local int = ffi.new('int[1]')
local cdata = capi.swim_member_payload(ptr, int)
return cdata, int[0]
end
diff --git a/test/app-tap/gh-5632-gc-buf-reuse.test.lua b/test/app-tap/gh-5632-gc-buf-reuse.test.lua
index bfa86615f..3b8138f11 100755
--- a/test/app-tap/gh-5632-gc-buf-reuse.test.lua
+++ b/test/app-tap/gh-5632-gc-buf-reuse.test.lua
@@ -11,6 +11,7 @@ local tap = require('tap')
local ffi = require('ffi')
local uuid = require('uuid')
local uri = require('uri')
+local msgpackffi = require('msgpackffi')
local function test_uuid(test)
test:plan(1)
@@ -99,9 +100,52 @@ local function test_uri(test)
test:ok(is_success, 'uri in gc')
end
+local function test_msgpackffi(test)
+ test:plan(1)
+
+ local mp_encode = msgpackffi.encode
+ local mp_decode = msgpackffi.decode
+ local gc_count = 100
+ local iter_count = 1000
+ local is_success = true
+ local data = {0, 1, 1000, 100000000, 'str', true, 1.1}
+
+ local function do_encode()
+ if not is_success then
+ return
+ end
+ local t = mp_encode(data)
+ t = mp_decode(t)
+ if #t ~= #data then
+ is_success = false
+ return
+ end
+ for i = 1, #t do
+ if t[i] ~= data[i] then
+ is_success = false
+ return
+ end
+ end
+ end
+
+ local function create_gc()
+ for i = 1, gc_count do
+ ffi.gc(ffi.new('char[1]'), do_encode)
+ end
+ end
+
+ for i = 1, iter_count do
+ create_gc()
+ do_encode()
+ end
+
+ test:ok(is_success, 'msgpackffi in gc')
+end
+
local test = tap.test('gh-5632-gc-buf-reuse')
-test:plan(2)
+test:plan(3)
test:test('uuid in __gc', test_uuid)
test:test('uri in __gc', test_uri)
+test:test('msgpackffi in __gc', test_msgpackffi)
os.exit(test:check() and 0 or 1)
diff --git a/test/app-tap/module_api.test.lua b/test/app-tap/module_api.test.lua
index f0a864e02..c945ac4bc 100755
--- a/test/app-tap/module_api.test.lua
+++ b/test/app-tap/module_api.test.lua
@@ -41,7 +41,7 @@ local function test_pushcdata(test, module)
end
local function test_buffers(test, module)
- test:plan(9)
+ test:plan(8)
local buffer = require('buffer')
local bufalloc = ffi.new('char[?]', 128)
@@ -53,7 +53,6 @@ local function test_buffers(test, module)
test:ok(not module.toibuf({}), 'toibuf of {}')
test:ok(not module.toibuf(1LL), 'toibuf of 1LL')
test:ok(not module.toibuf(box.NULL), 'toibuf of box.NULL')
- test:ok(not module.toibuf(buffer.reg1), 'toibuf of reg1')
test:ok(not module.toibuf(bufalloc), 'toibuf of allocated buffer')
test:ok(module.toibuf(ibuf_ptr), "toibuf of ibuf*")
test:ok(module.toibuf(ibuf), 'toibuf of ibuf')
diff --git a/test/app/buffer.result b/test/app/buffer.result
deleted file mode 100644
index 0934ae58f..000000000
--- a/test/app/buffer.result
+++ /dev/null
@@ -1,36 +0,0 @@
-test_run = require('test_run').new()
----
-...
-buffer = require('buffer')
----
-...
-ffi = require('ffi')
----
-...
--- Registers.
-reg1 = buffer.reg1
----
-...
-reg1.u16 = 100
----
-...
-u16 = ffi.new('uint16_t[1]')
----
-...
-ffi.copy(u16, reg1, 2)
----
-...
-u16[0]
----
-- 100
-...
-u16[0] = 200
----
-...
-ffi.copy(reg1, u16, 2)
----
-...
-reg1.u16
----
-- 200
-...
diff --git a/test/app/buffer.test.lua b/test/app/buffer.test.lua
deleted file mode 100644
index 1873e189e..000000000
--- a/test/app/buffer.test.lua
+++ /dev/null
@@ -1,14 +0,0 @@
-test_run = require('test_run').new()
-buffer = require('buffer')
-ffi = require('ffi')
-
--- Registers.
-reg1 = buffer.reg1
-reg1.u16 = 100
-u16 = ffi.new('uint16_t[1]')
-ffi.copy(u16, reg1, 2)
-u16[0]
-
-u16[0] = 200
-ffi.copy(reg1, u16, 2)
-reg1.u16
--
2.24.3 (Apple Git-128)
^ permalink raw reply [flat|nested] 32+ messages in thread* [Tarantool-patches] [PATCH 02/16] test: don't use IBUF_SHARED in the tests
2021-03-20 0:42 [Tarantool-patches] [PATCH 00/16] Cord buffer, static alloc, and Lua GC bug Vladislav Shpilevoy via Tarantool-patches
` (7 preceding siblings ...)
2021-03-20 0:42 ` [Tarantool-patches] [PATCH 16/16] buffer: remove Lua registers Vladislav Shpilevoy via Tarantool-patches
@ 2021-03-20 0:42 ` Vladislav Shpilevoy via Tarantool-patches
2021-03-22 7:35 ` Cyrill Gorcunov via Tarantool-patches
2021-03-20 0:42 ` [Tarantool-patches] [PATCH 03/16] tuple: pass global ibuf explicitly where possible Vladislav Shpilevoy via Tarantool-patches
` (12 subsequent siblings)
21 siblings, 1 reply; 32+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-03-20 0:42 UTC (permalink / raw)
To: tarantool-patches, gorcunov, sergepetrenko
In msgpack test it is used only to check that 'struct ibuf *' can
be passed to encode() functions. But soon IBUF_SHARED will be
deleted, and its alternative won't be yield-tolerant. This means
it can't be used in this test. There are yields between the buffer
usages.
In varbinary test it is used in a too complicated way to be able
to put it back normally. And otherwise its usage does not make
much sense - without put() it is going to be created from the
scratch on non-first usage until a yield.
In the module_api test it is used to check if some function works
with 'struct ibuf *'. Can be done without IBUF_SHARED.
Part of #5632
---
test/app-tap/module_api.test.lua | 4 +++-
test/app/msgpack.result | 7 +++++--
test/app/msgpack.test.lua | 5 +++--
test/box/varbinary_type.result | 3 +--
test/box/varbinary_type.test.lua | 3 +--
5 files changed, 13 insertions(+), 9 deletions(-)
diff --git a/test/app-tap/module_api.test.lua b/test/app-tap/module_api.test.lua
index c43d8eacd..28e33a02d 100755
--- a/test/app-tap/module_api.test.lua
+++ b/test/app-tap/module_api.test.lua
@@ -1,5 +1,6 @@
#!/usr/bin/env tarantool
+local ffi = require('ffi')
local fio = require('fio')
box.cfg{log = "tarantool.log"}
@@ -45,6 +46,7 @@ local function test_buffers(test, module)
local bufalloc = buffer.static_alloc("char", 128)
local ibuf = buffer.ibuf()
local pbuf = ibuf:alloc(128)
+ local ibuf_ptr = ffi.cast('struct ibuf *', ibuf)
test:ok(not module.toibuf(nil), 'toibuf of nil')
test:ok(not module.toibuf({}), 'toibuf of {}')
@@ -52,7 +54,7 @@ local function test_buffers(test, module)
test:ok(not module.toibuf(box.NULL), 'toibuf of box.NULL')
test:ok(not module.toibuf(buffer.reg1), 'toibuf of reg1')
test:ok(not module.toibuf(bufalloc), 'toibuf of allocated buffer')
- test:ok(module.toibuf(buffer.IBUF_SHARED), "toibuf of ibuf*")
+ test:ok(module.toibuf(ibuf_ptr), "toibuf of ibuf*")
test:ok(module.toibuf(ibuf), 'toibuf of ibuf')
test:ok(not module.toibuf(pbuf), 'toibuf of pointer to ibuf data')
end
diff --git a/test/app/msgpack.result b/test/app/msgpack.result
index ddf06fc9d..faee66029 100644
--- a/test/app/msgpack.result
+++ b/test/app/msgpack.result
@@ -206,10 +206,10 @@ msgpack.decode(buf.rpos, buf:size() - 1)
- error: 'msgpack.decode: invalid MsgPack'
...
-- Provide a buffer. Try both 'struct ibuf' and 'struct ibuf *'.
-buf = buffer.IBUF_SHARED
+buf_storage = buffer.ibuf()
---
...
-buf:reset()
+buf = ffi.cast('struct ibuf *', buf_storage)
---
...
size = msgpack.encode({a = 1, b = 2}, buf)
@@ -219,6 +219,9 @@ size = msgpack.encode({a = 1, b = 2}, buf)
---
- {'a': 1, 'b': 2}
...
+buf_storage = nil
+---
+...
buf = buffer.ibuf()
---
...
diff --git a/test/app/msgpack.test.lua b/test/app/msgpack.test.lua
index 17e05df5c..f5299f550 100644
--- a/test/app/msgpack.test.lua
+++ b/test/app/msgpack.test.lua
@@ -65,10 +65,11 @@ msgpack.encode({1, 2, 3}, buf)
msgpack.decode(buf.rpos, buf:size() - 1)
-- Provide a buffer. Try both 'struct ibuf' and 'struct ibuf *'.
-buf = buffer.IBUF_SHARED
-buf:reset()
+buf_storage = buffer.ibuf()
+buf = ffi.cast('struct ibuf *', buf_storage)
size = msgpack.encode({a = 1, b = 2}, buf)
(msgpack.decode(buf.rpos, size))
+buf_storage = nil
buf = buffer.ibuf()
size = msgpack.encode({c = 3, d = 4}, buf)
(msgpack.decode(buf.rpos, size))
diff --git a/test/box/varbinary_type.result b/test/box/varbinary_type.result
index 9620ee39e..5aa2996df 100644
--- a/test/box/varbinary_type.result
+++ b/test/box/varbinary_type.result
@@ -40,8 +40,7 @@ test_run:cmd("setopt delimiter ';'")
- true
...
function encode_bin(bytes)
- local tmpbuf = buffer.IBUF_SHARED
- tmpbuf:reset()
+ local tmpbuf = buffer.ibuf()
local p = tmpbuf:alloc(3 + #bytes)
p[0] = 0x91
p[1] = 0xC4
diff --git a/test/box/varbinary_type.test.lua b/test/box/varbinary_type.test.lua
index 3c6b66246..7b9a1e721 100644
--- a/test/box/varbinary_type.test.lua
+++ b/test/box/varbinary_type.test.lua
@@ -16,8 +16,7 @@ ffi = require('ffi')
test_run:cmd("setopt delimiter ';'")
function encode_bin(bytes)
- local tmpbuf = buffer.IBUF_SHARED
- tmpbuf:reset()
+ local tmpbuf = buffer.ibuf()
local p = tmpbuf:alloc(3 + #bytes)
p[0] = 0x91
p[1] = 0xC4
--
2.24.3 (Apple Git-128)
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [Tarantool-patches] [PATCH 02/16] test: don't use IBUF_SHARED in the tests
2021-03-20 0:42 ` [Tarantool-patches] [PATCH 02/16] test: don't use IBUF_SHARED in the tests Vladislav Shpilevoy via Tarantool-patches
@ 2021-03-22 7:35 ` Cyrill Gorcunov via Tarantool-patches
0 siblings, 0 replies; 32+ messages in thread
From: Cyrill Gorcunov via Tarantool-patches @ 2021-03-22 7:35 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
On Sat, Mar 20, 2021 at 01:42:39AM +0100, Vladislav Shpilevoy wrote:
> In msgpack test it is used only to check that 'struct ibuf *' can
> be passed to encode() functions. But soon IBUF_SHARED will be
> deleted, and its alternative won't be yield-tolerant. This means
> it can't be used in this test. There are yields between the buffer
> usages.
>
> In varbinary test it is used in a too complicated way to be able
> to put it back normally. And otherwise its usage does not make
> much sense - without put() it is going to be created from the
> scratch on non-first usage until a yield.
>
> In the module_api test it is used to check if some function works
> with 'struct ibuf *'. Can be done without IBUF_SHARED.
Ack
^ permalink raw reply [flat|nested] 32+ messages in thread
* [Tarantool-patches] [PATCH 03/16] tuple: pass global ibuf explicitly where possible
2021-03-20 0:42 [Tarantool-patches] [PATCH 00/16] Cord buffer, static alloc, and Lua GC bug Vladislav Shpilevoy via Tarantool-patches
` (8 preceding siblings ...)
2021-03-20 0:42 ` [Tarantool-patches] [PATCH 02/16] test: don't use IBUF_SHARED in the tests Vladislav Shpilevoy via Tarantool-patches
@ 2021-03-20 0:42 ` Vladislav Shpilevoy via Tarantool-patches
2021-03-20 0:42 ` [Tarantool-patches] [PATCH 04/16] iconv: take errno before reseting the context Vladislav Shpilevoy via Tarantool-patches
` (11 subsequent siblings)
21 siblings, 0 replies; 32+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-03-20 0:42 UTC (permalink / raw)
To: tarantool-patches, gorcunov, sergepetrenko
Code in lua/tuple.c used global tarantool_lua_ibuf in many places
relying on it never being changed and not reused by other code
until a yield.
But it is not so. In fact, as it was discovered in #5632, in any
Lua function may be started GC. Any GC handler might touch some
API also using tarantool_lua_ibuf inside.
This makes the first usage in lua/tuple.c invalid - the buffer
could be reset or reallocated or its wpos/rpos could change during
GC.
In order to fix this, first of all there should be clear points
where the buffer is taken, and where it becomes not needed
anymore.
The patch makes code in lua/tuple.c take tarantool_lua_ibuf when
it is needed first time. Not during usage. The same is done for
the fiber region for the API symmetry.
Part of #5632
---
src/box/lua/tuple.c | 60 ++++++++++++++++++++++++++++-----------------
1 file changed, 37 insertions(+), 23 deletions(-)
diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c
index 3e6f043b4..ffd01d899 100644
--- a/src/box/lua/tuple.c
+++ b/src/box/lua/tuple.c
@@ -135,10 +135,8 @@ luaT_istuple(struct lua_State *L, int narg)
* Helper for <lbox_tuple_new>().
*/
static int
-luaT_tuple_encode_values(struct lua_State *L)
+luaT_tuple_encode_values(struct lua_State *L, struct ibuf *buf)
{
- struct ibuf *buf = tarantool_lua_ibuf;
- ibuf_reset(buf);
struct mpstream stream;
mpstream_init(&stream, buf, ibuf_reserve_cb, ibuf_alloc_cb, luamp_error,
L);
@@ -151,22 +149,31 @@ luaT_tuple_encode_values(struct lua_State *L)
return 0;
}
-typedef void luaT_mpstream_init_f(struct mpstream *stream, struct lua_State *L);
+typedef void luaT_mpstream_init_f(struct mpstream *stream, struct lua_State *L,
+ void *buffer);
static void
-luaT_mpstream_init_lua_ibuf(struct mpstream *stream, struct lua_State *L)
+luaT_mpstream_init_lua_ibuf(struct mpstream *stream, struct lua_State *L,
+ void *buffer)
{
- mpstream_init(stream, tarantool_lua_ibuf, ibuf_reserve_cb,
+ mpstream_init(stream, (struct ibuf *)buffer, ibuf_reserve_cb,
ibuf_alloc_cb, luamp_error, L);
}
static void
-luaT_mpstream_init_box_region(struct mpstream *stream, struct lua_State *L)
+luaT_mpstream_init_box_region(struct mpstream *stream, struct lua_State *L,
+ void *buffer)
{
- mpstream_init(stream, &fiber()->gc, region_reserve_cb, region_alloc_cb,
- luamp_error, L);
+ mpstream_init(stream, (struct region *)buffer, region_reserve_cb,
+ region_alloc_cb, luamp_error, L);
}
+/** Helper to pass parameters into luaT_tuple_encode_table via the Lua stack. */
+struct luaT_tuple_encode_ctx {
+ luaT_mpstream_init_f *mpstream_init_f;
+ void *buffer;
+};
+
/**
* Encode a Lua table or a tuple as MsgPack.
*
@@ -178,8 +185,8 @@ static int
luaT_tuple_encode_table(struct lua_State *L)
{
struct mpstream stream;
- luaT_mpstream_init_f *luaT_mpstream_init_f = lua_topointer(L, 1);
- luaT_mpstream_init_f(&stream, L);
+ const struct luaT_tuple_encode_ctx *ctx = lua_topointer(L, 1);
+ ctx->mpstream_init_f(&stream, L, ctx->buffer);
luamp_encode_tuple(L, &tuple_serializer, &stream, 2);
mpstream_flush(&stream);
return 0;
@@ -190,7 +197,8 @@ luaT_tuple_encode_table(struct lua_State *L)
*/
static int
luaT_tuple_encode_on_mpstream(struct lua_State *L, int idx,
- luaT_mpstream_init_f *luaT_mpstream_init_f)
+ luaT_mpstream_init_f *luaT_mpstream_init_f,
+ void *buffer)
{
assert(idx != 0);
if (!lua_istable(L, idx) && !luaT_istuple(L, idx)) {
@@ -213,7 +221,11 @@ luaT_tuple_encode_on_mpstream(struct lua_State *L, int idx,
lua_rawgeti(L, LUA_REGISTRYINDEX, luaT_tuple_encode_table_ref);
assert(lua_isfunction(L, -1));
- lua_pushlightuserdata(L, luaT_mpstream_init_f);
+ struct luaT_tuple_encode_ctx ctx = {
+ .mpstream_init_f = luaT_mpstream_init_f,
+ .buffer = buffer,
+ };
+ lua_pushlightuserdata(L, &ctx);
lua_pushvalue(L, idx);
int rc = luaT_call(L, 2, 0);
@@ -226,12 +238,11 @@ luaT_tuple_encode_on_mpstream(struct lua_State *L, int idx,
*/
static char *
luaT_tuple_encode_on_lua_ibuf(struct lua_State *L, int idx,
- size_t *tuple_len_ptr)
+ size_t *tuple_len_ptr, struct ibuf *buf)
{
- struct ibuf *buf = tarantool_lua_ibuf;
- ibuf_reset(buf);
if (luaT_tuple_encode_on_mpstream(L, idx,
- luaT_mpstream_init_lua_ibuf) != 0)
+ luaT_mpstream_init_lua_ibuf,
+ buf) != 0)
return NULL;
if (tuple_len_ptr != NULL)
*tuple_len_ptr = ibuf_used(buf);
@@ -247,7 +258,8 @@ luaT_tuple_encode(struct lua_State *L, int idx, size_t *tuple_len_ptr)
struct region *region = &fiber()->gc;
size_t region_svp = region_used(region);
if (luaT_tuple_encode_on_mpstream(L, idx,
- luaT_mpstream_init_box_region) != 0) {
+ luaT_mpstream_init_box_region,
+ region) != 0) {
region_truncate(region, region_svp);
return NULL;
}
@@ -268,15 +280,16 @@ luaT_tuple_encode(struct lua_State *L, int idx, size_t *tuple_len_ptr)
box_tuple_t *
luaT_tuple_new(struct lua_State *L, int idx, box_tuple_format_t *format)
{
+ struct ibuf *ibuf = tarantool_lua_ibuf;
+ ibuf_reset(ibuf);
size_t tuple_len;
- char *tuple_data = luaT_tuple_encode_on_lua_ibuf(L, idx, &tuple_len);
+ char *tuple_data = luaT_tuple_encode_on_lua_ibuf(L, idx, &tuple_len,
+ ibuf);
if (tuple_data == NULL)
return NULL;
box_tuple_t *tuple = box_tuple_new(format, tuple_data,
tuple_data + tuple_len);
- if (tuple == NULL)
- return NULL;
- ibuf_reinit(tarantool_lua_ibuf);
+ ibuf_reinit(ibuf);
return tuple;
}
@@ -296,7 +309,8 @@ lbox_tuple_new(lua_State *L)
box_tuple_format_t *fmt = box_tuple_format_default();
if (argc != 1 || (!lua_istable(L, 1) && !luaT_istuple(L, 1))) {
struct ibuf *buf = tarantool_lua_ibuf;
- luaT_tuple_encode_values(L); /* may raise */
+ ibuf_reset(buf);
+ luaT_tuple_encode_values(L, buf); /* may raise */
struct tuple *tuple = box_tuple_new(fmt, buf->buf,
buf->buf + ibuf_used(buf));
ibuf_reinit(buf);
--
2.24.3 (Apple Git-128)
^ permalink raw reply [flat|nested] 32+ messages in thread* [Tarantool-patches] [PATCH 04/16] iconv: take errno before reseting the context
2021-03-20 0:42 [Tarantool-patches] [PATCH 00/16] Cord buffer, static alloc, and Lua GC bug Vladislav Shpilevoy via Tarantool-patches
` (9 preceding siblings ...)
2021-03-20 0:42 ` [Tarantool-patches] [PATCH 03/16] tuple: pass global ibuf explicitly where possible Vladislav Shpilevoy via Tarantool-patches
@ 2021-03-20 0:42 ` Vladislav Shpilevoy via Tarantool-patches
2021-03-20 0:42 ` [Tarantool-patches] [PATCH 05/16] cord_buf: introduce cord_buf API Vladislav Shpilevoy via Tarantool-patches
` (10 subsequent siblings)
21 siblings, 0 replies; 32+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-03-20 0:42 UTC (permalink / raw)
To: tarantool-patches, gorcunov, sergepetrenko
In Lua iconv_convert() in case ffi.C.tnt_iconv() with normal
arguments failed, tried to clear iconv context by calling the
function again with all arguments NULL. Then it looked at errno.
But the second call could do anything with errno. For instance, it
could also fail, and change errno.
The patch saves errno into a variable before calling tnt_iconv()
second time.
It still does not give a perfect protection as it was discovered
in scope of #5632, but still better.
The patch is mostly motivated by the next patches about #5632
which will add another call to the error path, and it should
better be after errno save.
Needed for #5632
---
src/lua/iconv.lua | 19 +++++++++++--------
1 file changed, 11 insertions(+), 8 deletions(-)
diff --git a/src/lua/iconv.lua b/src/lua/iconv.lua
index ade66d760..e68509dec 100644
--- a/src/lua/iconv.lua
+++ b/src/lua/iconv.lua
@@ -43,15 +43,18 @@ local function iconv_convert(iconv, data)
buf_left[0] = buf:unused()
local res = ffi.C.tnt_iconv(iconv, data_ptr, data_left,
buf_ptr, buf_left)
- if res == ffi.cast('size_t', -1) and errno() ~= E2BIG then
- ffi.C.tnt_iconv(iconv, nil, nil, nil, nil)
- if errno() == EINVAL then
- error('Invalid multibyte sequence')
+ if res == ffi.cast('size_t', -1) then
+ local err = errno()
+ if err ~= E2BIG then
+ ffi.C.tnt_iconv(iconv, nil, nil, nil, nil)
+ if err == EINVAL then
+ error('Invalid multibyte sequence')
+ end
+ if err == EILSEQ then
+ error('Incomplete multibyte sequence')
+ end
+ error('Unknown conversion error: ' .. errno.strerror(err))
end
- if errno() == EILSEQ then
- error('Incomplete multibyte sequence')
- end
- error('Unknown conversion error: ' .. errno.strerror())
end
buf:alloc(buf:unused() - buf_left[0])
end
--
2.24.3 (Apple Git-128)
^ permalink raw reply [flat|nested] 32+ messages in thread* [Tarantool-patches] [PATCH 05/16] cord_buf: introduce cord_buf API
2021-03-20 0:42 [Tarantool-patches] [PATCH 00/16] Cord buffer, static alloc, and Lua GC bug Vladislav Shpilevoy via Tarantool-patches
` (10 preceding siblings ...)
2021-03-20 0:42 ` [Tarantool-patches] [PATCH 04/16] iconv: take errno before reseting the context Vladislav Shpilevoy via Tarantool-patches
@ 2021-03-20 0:42 ` Vladislav Shpilevoy via Tarantool-patches
2021-03-20 0:42 ` [Tarantool-patches] [PATCH 06/16] cord_buf: introduce ownership management Vladislav Shpilevoy via Tarantool-patches
` (9 subsequent siblings)
21 siblings, 0 replies; 32+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-03-20 0:42 UTC (permalink / raw)
To: tarantool-patches, gorcunov, sergepetrenko
There was a global ibuf object called tarantool_lua_ibuf. It was
used in all the places working with Lua which didn't have yields,
and where fiber's region could be potentially slower due to not
being able to guarantee the allocated memory is contiguous.
Yields during the ibuf usage were prohibited because another fiber
would take the same ibuf and override its previous content which
was still used by another fiber.
But it wasn't taken into account that there is Lua GC. It can be
invoked from any Lua function in Lua C code, and almost on any
line in the Lua scripts. During GC some deleted objects might have
GC handlers installed as __gc metamethods. From the handler they
could call Tarantool functions, including the ones using the
global ibuf.
Therefore ibuf could be overridden not only at yields, but almost
in any moment. Because with the Lua GC at hand, the multitasking
is not strictly "cooperative" anymore.
It is necessary to implement ownership for the global buffer. The
patch prepares the API for this: the buffer is moved to its own
file, and has methods take(), put(), and drop().
Take() is supposed to make the current fiber own the buffer. Put()
makes it available again. Drop() does the same but also clears the
buffer (frees its memory). The ownership itself is a subject for
the next patches. Here only the API is prepared.
The patch "hits" performance a little. Previously the get of
buffer.IBUF_SHARED cost around 1 ns. Now cord_ibuf_take() +
cord_ibuf_put() cost around 5 ns together. The next patches will
make it worse, up to 15 ns until #5871 is done.
Part of #5632
---
src/box/lua/schema.lua | 47 +++++++++++------
src/box/lua/tuple.c | 28 +++++-----
src/box/lua/tuple.lua | 14 +++--
src/exports.h | 4 +-
src/lib/core/CMakeLists.txt | 1 +
src/lib/core/cord_buf.c | 47 +++++++++++++++++
src/lib/core/cord_buf.h | 45 ++++++++++++++++
src/lua/buffer.lua | 51 ++++++++++++++++++-
src/lua/iconv.lua | 8 +--
src/lua/init.c | 3 --
src/lua/msgpack.c | 6 +--
src/lua/msgpackffi.lua | 7 +--
src/lua/socket.lua | 14 +++--
src/lua/swim.lua | 17 ++++---
src/lua/utf8.c | 16 +++---
src/lua/utils.h | 1 -
.../test/static-build/exports.test.lua | 4 +-
test/unit/luaT_tuple_new.c | 4 --
18 files changed, 242 insertions(+), 75 deletions(-)
create mode 100644 src/lib/core/cord_buf.c
create mode 100644 src/lib/core/cord_buf.h
diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
index fea8640a6..cc5a3451a 100644
--- a/src/box/lua/schema.lua
+++ b/src/box/lua/schema.lua
@@ -18,6 +18,8 @@ local tuple_encode = box.internal.tuple.encode
local tuple_bless = box.internal.tuple.bless
local is_tuple = box.tuple.is
assert(tuple_encode ~= nil and tuple_bless ~= nil and is_tuple ~= nil)
+local cord_ibuf_take = buffer.internal.cord_ibuf_take
+local cord_ibuf_put = buffer.internal.cord_ibuf_put
local INT64_MIN = tonumber64('-9223372036854775808')
local INT64_MAX = tonumber64('9223372036854775807')
@@ -1785,9 +1787,12 @@ base_index_mt.__len = base_index_mt.len
-- min and max
base_index_mt.min_ffi = function(index, key)
check_index_arg(index, 'min')
- local pkey, pkey_end = tuple_encode(key)
- if builtin.box_index_min(index.space_id, index.id,
- pkey, pkey_end, ptuple) ~= 0 then
+ local ibuf = cord_ibuf_take()
+ local pkey, pkey_end = tuple_encode(ibuf, key)
+ local nok = builtin.box_index_min(index.space_id, index.id, pkey, pkey_end,
+ ptuple) ~= 0
+ cord_ibuf_put(ibuf)
+ if nok then
box.error() -- error
elseif ptuple[0] ~= nil then
return tuple_bless(ptuple[0])
@@ -1802,9 +1807,12 @@ base_index_mt.min_luac = function(index, key)
end
base_index_mt.max_ffi = function(index, key)
check_index_arg(index, 'max')
- local pkey, pkey_end = tuple_encode(key)
- if builtin.box_index_max(index.space_id, index.id,
- pkey, pkey_end, ptuple) ~= 0 then
+ local ibuf = cord_ibuf_take()
+ local pkey, pkey_end = tuple_encode(ibuf, key)
+ local nok = builtin.box_index_max(index.space_id, index.id, pkey, pkey_end,
+ ptuple) ~= 0
+ cord_ibuf_put(ibuf)
+ if nok then
box.error() -- error
elseif ptuple[0] ~= nil then
return tuple_bless(ptuple[0])
@@ -1837,10 +1845,12 @@ end
-- iteration
base_index_mt.pairs_ffi = function(index, key, opts)
check_index_arg(index, 'pairs')
- local pkey, pkey_end = tuple_encode(key)
+ local ibuf = cord_ibuf_take()
+ local pkey, pkey_end = tuple_encode(ibuf, key)
local itype = check_iterator_type(opts, pkey + 1 >= pkey_end);
local keybuf = ffi.string(pkey, pkey_end - pkey)
+ cord_ibuf_put(ibuf)
local pkeybuf = ffi.cast('const char *', keybuf)
local cdata = builtin.box_index_iterator(index.space_id, index.id,
itype, pkeybuf, pkeybuf + #keybuf);
@@ -1864,10 +1874,12 @@ end
-- index subtree size
base_index_mt.count_ffi = function(index, key, opts)
check_index_arg(index, 'count')
- local pkey, pkey_end = tuple_encode(key)
+ local ibuf = cord_ibuf_take()
+ local pkey, pkey_end = tuple_encode(ibuf, key)
local itype = check_iterator_type(opts, pkey + 1 >= pkey_end);
local count = builtin.box_index_count(index.space_id, index.id,
itype, pkey, pkey_end);
+ cord_ibuf_put(ibuf)
if count == -1 then
box.error()
end
@@ -1882,9 +1894,12 @@ end
base_index_mt.get_ffi = function(index, key)
check_index_arg(index, 'get')
- local key, key_end = tuple_encode(key)
- if builtin.box_index_get(index.space_id, index.id,
- key, key_end, ptuple) ~= 0 then
+ local ibuf = cord_ibuf_take()
+ local key, key_end = tuple_encode(ibuf, key)
+ local nok = builtin.box_index_get(index.space_id, index.id, key, key_end,
+ ptuple) ~= 0
+ cord_ibuf_put(ibuf)
+ if nok then
return box.error() -- error
elseif ptuple[0] ~= nil then
return tuple_bless(ptuple[0])
@@ -1915,13 +1930,15 @@ end
base_index_mt.select_ffi = function(index, key, opts)
check_index_arg(index, 'select')
- local key, key_end = tuple_encode(key)
+ local ibuf = cord_ibuf_take()
+ local key, key_end = tuple_encode(ibuf, key)
local iterator, offset, limit = check_select_opts(opts, key + 1 >= key_end)
local port = ffi.cast('struct port *', port_c)
-
- if builtin.box_select(index.space_id, index.id,
- iterator, offset, limit, key, key_end, port) ~= 0 then
+ local nok = builtin.box_select(index.space_id, index.id, iterator, offset,
+ limit, key, key_end, port) ~= 0
+ cord_ibuf_put(ibuf)
+ if nok then
return box.error()
end
diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c
index ffd01d899..f7198a025 100644
--- a/src/box/lua/tuple.c
+++ b/src/box/lua/tuple.c
@@ -36,6 +36,7 @@
#include "diag.h" /* diag_set() */
#include <small/ibuf.h>
#include <small/region.h>
+#include "cord_buf.h"
#include <fiber.h>
#include "box/tuple.h"
@@ -280,16 +281,18 @@ luaT_tuple_encode(struct lua_State *L, int idx, size_t *tuple_len_ptr)
box_tuple_t *
luaT_tuple_new(struct lua_State *L, int idx, box_tuple_format_t *format)
{
- struct ibuf *ibuf = tarantool_lua_ibuf;
- ibuf_reset(ibuf);
+ struct ibuf *ibuf = cord_ibuf_take();
size_t tuple_len;
+ box_tuple_t *tuple;
char *tuple_data = luaT_tuple_encode_on_lua_ibuf(L, idx, &tuple_len,
ibuf);
- if (tuple_data == NULL)
- return NULL;
- box_tuple_t *tuple = box_tuple_new(format, tuple_data,
- tuple_data + tuple_len);
- ibuf_reinit(ibuf);
+ if (tuple_data == NULL) {
+ tuple = NULL;
+ } else {
+ tuple = box_tuple_new(format, tuple_data,
+ tuple_data + tuple_len);
+ }
+ cord_ibuf_drop(ibuf);
return tuple;
}
@@ -308,12 +311,11 @@ lbox_tuple_new(lua_State *L)
*/
box_tuple_format_t *fmt = box_tuple_format_default();
if (argc != 1 || (!lua_istable(L, 1) && !luaT_istuple(L, 1))) {
- struct ibuf *buf = tarantool_lua_ibuf;
- ibuf_reset(buf);
+ struct ibuf *buf = cord_ibuf_take();
luaT_tuple_encode_values(L, buf); /* may raise */
struct tuple *tuple = box_tuple_new(fmt, buf->buf,
buf->buf + ibuf_used(buf));
- ibuf_reinit(buf);
+ cord_ibuf_drop(buf);
if (tuple == NULL)
return luaT_error(L);
luaT_pushtuple(L, tuple);
@@ -573,8 +575,7 @@ lbox_tuple_transform(struct lua_State *L)
return 1;
}
- struct ibuf *buf = tarantool_lua_ibuf;
- ibuf_reset(buf);
+ struct ibuf *buf = cord_ibuf_take();
struct mpstream stream;
mpstream_init(&stream, buf, ibuf_reserve_cb, ibuf_alloc_cb,
luamp_error, L);
@@ -619,12 +620,11 @@ lbox_tuple_transform(struct lua_State *L)
new_tuple = tuple_new(box_tuple_format_default(),
new_data, new_data + new_size);
region_truncate(region, used);
-
+ cord_ibuf_put(buf);
if (new_tuple == NULL)
luaT_error(L);
luaT_pushtuple(L, new_tuple);
- ibuf_reset(buf);
return 1;
}
diff --git a/src/box/lua/tuple.lua b/src/box/lua/tuple.lua
index 813d66ff0..fa76f4f7f 100644
--- a/src/box/lua/tuple.lua
+++ b/src/box/lua/tuple.lua
@@ -5,6 +5,8 @@ local msgpackffi = require('msgpackffi')
local fun = require('fun')
local buffer = require('buffer')
local internal = require('box.internal')
+local cord_ibuf_take = buffer.internal.cord_ibuf_take
+local cord_ibuf_put = buffer.internal.cord_ibuf_put
ffi.cdef[[
/** \cond public */
@@ -71,9 +73,7 @@ local encode_fix = msgpackffi.internal.encode_fix
local encode_array = msgpackffi.internal.encode_array
local encode_r = msgpackffi.internal.encode_r
-local tuple_encode = function(obj)
- local tmpbuf = buffer.IBUF_SHARED
- tmpbuf:reset()
+local tuple_encode = function(tmpbuf, obj)
if obj == nil then
encode_fix(tmpbuf, 0x90, 0) -- empty array
elseif is_tuple(obj) then
@@ -229,8 +229,10 @@ local function tuple_update(tuple, expr)
if type(expr) ~= 'table' then
error("Usage: tuple:update({ { op, field, arg}+ })")
end
- local pexpr, pexpr_end = tuple_encode(expr)
+ local ibuf = cord_ibuf_take()
+ local pexpr, pexpr_end = tuple_encode(ibuf, expr)
local tuple = builtin.box_tuple_update(tuple, pexpr, pexpr_end)
+ cord_ibuf_put(ibuf)
if tuple == nil then
return box.error()
end
@@ -242,8 +244,10 @@ local function tuple_upsert(tuple, expr)
if type(expr) ~= 'table' then
error("Usage: tuple:upsert({ { op, field, arg}+ })")
end
- local pexpr, pexpr_end = tuple_encode(expr)
+ local ibuf = cord_ibuf_take()
+ local pexpr, pexpr_end = tuple_encode(ibuf, expr)
local tuple = builtin.box_tuple_upsert(tuple, pexpr, pexpr_end)
+ cord_ibuf_put(ibuf)
if tuple == nil then
return box.error()
end
diff --git a/src/exports.h b/src/exports.h
index eb72bc928..ddbe57230 100644
--- a/src/exports.h
+++ b/src/exports.h
@@ -113,6 +113,9 @@ EXPORT(coio_getaddrinfo)
EXPORT(coio_wait)
EXPORT(console_get_output_format)
EXPORT(console_set_output_format)
+EXPORT(cord_ibuf_drop)
+EXPORT(cord_ibuf_put)
+EXPORT(cord_ibuf_take)
EXPORT(cord_slab_cache)
EXPORT(crc32_calc)
EXPORT(crypto_EVP_MD_CTX_free)
@@ -493,7 +496,6 @@ EXPORT(swim_set_codec)
EXPORT(swim_set_payload)
EXPORT(swim_size)
EXPORT(tarantool_exit)
-EXPORT(tarantool_lua_ibuf)
EXPORT(tarantool_lua_slab_cache)
EXPORT(tarantool_uptime)
EXPORT(title_get)
diff --git a/src/lib/core/CMakeLists.txt b/src/lib/core/CMakeLists.txt
index 30cf0dd15..2cd4d0b4f 100644
--- a/src/lib/core/CMakeLists.txt
+++ b/src/lib/core/CMakeLists.txt
@@ -29,6 +29,7 @@ set(core_sources
port.c
decimal.c
mp_decimal.c
+ cord_buf.c
)
if (TARGET_OS_NETBSD)
diff --git a/src/lib/core/cord_buf.c b/src/lib/core/cord_buf.c
new file mode 100644
index 000000000..cac508c3d
--- /dev/null
+++ b/src/lib/core/cord_buf.c
@@ -0,0 +1,47 @@
+/*
+ * SPDX-License-Identifier: BSD-2-Clause
+ *
+ * Copyright 2010-2021, Tarantool AUTHORS, please see AUTHORS file.
+ */
+#include "cord_buf.h"
+#include "fiber.h"
+
+#include "small/ibuf.h"
+
+enum {
+ /* No any reason why that value. Historical constant. */
+ CORD_IBUF_START_CAPACITY = 16384,
+};
+
+static struct ibuf *cord_buf_global = NULL;
+
+struct ibuf *
+cord_ibuf_take(void)
+{
+ assert(cord_is_main());
+ struct ibuf *buf = cord_buf_global;
+ if (buf != NULL) {
+ ibuf_reset(buf);
+ return buf;
+ }
+ buf = malloc(sizeof(*buf));
+ if (buf == NULL)
+ panic("Couldn't allocate thread buffer");
+ ibuf_create(buf, &cord()->slabc, CORD_IBUF_START_CAPACITY);
+ cord_buf_global = buf;
+ return buf;
+}
+
+void
+cord_ibuf_put(struct ibuf *ibuf)
+{
+ (void)ibuf;
+ assert(ibuf == cord_buf_global);
+}
+
+void
+cord_ibuf_drop(struct ibuf *ibuf)
+{
+ ibuf_reinit(ibuf);
+ assert(ibuf == cord_buf_global);
+}
diff --git a/src/lib/core/cord_buf.h b/src/lib/core/cord_buf.h
new file mode 100644
index 000000000..59f429c8f
--- /dev/null
+++ b/src/lib/core/cord_buf.h
@@ -0,0 +1,45 @@
+/*
+ * SPDX-License-Identifier: BSD-2-Clause
+ *
+ * Copyright 2010-2021, Tarantool AUTHORS, please see AUTHORS file.
+ */
+#pragma once
+
+#if defined(__cplusplus)
+extern "C" {
+#endif /* defined(__cplusplus) */
+
+struct ibuf;
+
+/**
+ * Take the global ibuf, or allocate a new one if the stash is empty.
+ */
+struct ibuf *
+cord_ibuf_take(void);
+
+/**
+ * Put the global ibuf back.
+ */
+void
+cord_ibuf_put(struct ibuf *ibuf);
+
+/**
+ * Put the global ibuf back and free its memory. So only the buffer object
+ * itself is saved to the stash. Main reason why it is a dedicated function is
+ * because it is often needed from Lua, and allows not to call :recycle() there,
+ * which would be an additional FFI call before cord_ibuf_put().
+ *
+ * XXX: recycle of the global buffer is a workaround for the ibuf being used in
+ * some places working with Lua API, where it wasn't wanted to "reuse" it
+ * anyhow. Instead, the global buffer is used to protect from the buffer leak in
+ * case it would be created locally, and then a Lua error would be raised. When
+ * the buffer is global, it is not a problem, because it is reused/recycled
+ * later. But it hurts the places, where re-usage could be good. Probably it is
+ * worth to separate take/put() from new/drop() API. Or delete drop() entirely.
+ */
+void
+cord_ibuf_drop(struct ibuf *ibuf);
+
+#if defined(__cplusplus)
+}
+#endif /* defined(__cplusplus) */
diff --git a/src/lua/buffer.lua b/src/lua/buffer.lua
index 00846bb20..d5dbedb0a 100644
--- a/src/lua/buffer.lua
+++ b/src/lua/buffer.lua
@@ -7,7 +7,15 @@ ffi.cdef[[
struct slab_cache;
struct slab_cache *
tarantool_lua_slab_cache();
-extern struct ibuf *tarantool_lua_ibuf;
+
+struct ibuf *
+cord_ibuf_take(void);
+
+void
+cord_ibuf_put(struct ibuf *ibuf);
+
+void
+cord_ibuf_drop(struct ibuf *ibuf);
struct ibuf
{
@@ -244,9 +252,48 @@ end
--
local reg_array = ffi.new('union c_register[?]', 2)
+--
+-- Cord buffer is useful for the places, where
+--
+-- * Want to reuse the already allocated memory which might be stored in the
+-- cord buf. Although sometimes the buffer is recycled, so should not rely on
+-- being able to reuse it always. When reused, the win is the biggest -
+-- becomes about x20 times faster than a new buffer creation (~5ns vs ~100ns);
+--
+-- * Want to avoid allocation of a new ibuf because it produces a new GC object
+-- which is additional load for Lua GC. Although according to benches it is
+-- not super expensive;
+--
+-- * Almost always can put the buffer back manually. Not rely on it being
+-- recycled automatically. It is recycled, but still should not rely on that;
+--
+-- It is important to wrap the C functions, not expose them directly. Because
+-- JIT works a bit better when C functions are called as 'ffi.C.func()' than
+-- 'func()' with func being cached. The only pros is to cache 'ffi.C' itself.
+-- It is quite strange though how having them wrapped into a Lua function is
+-- faster than cached directly as C functions.
+--
+local function cord_ibuf_take()
+ return builtin.cord_ibuf_take()
+end
+
+local function cord_ibuf_put(buf)
+ return builtin.cord_ibuf_put(buf)
+end
+
+local function cord_ibuf_drop(buf)
+ return builtin.cord_ibuf_drop(buf)
+end
+
+local internal = {
+ cord_ibuf_take = cord_ibuf_take,
+ cord_ibuf_put = cord_ibuf_put,
+ cord_ibuf_drop = cord_ibuf_drop,
+}
+
return {
+ internal = internal,
ibuf = ibuf_new;
- IBUF_SHARED = ffi.C.tarantool_lua_ibuf;
READAHEAD = READAHEAD;
static_alloc = static_alloc,
-- Keep reference.
diff --git a/src/lua/iconv.lua b/src/lua/iconv.lua
index e68509dec..732b80514 100644
--- a/src/lua/iconv.lua
+++ b/src/lua/iconv.lua
@@ -1,6 +1,8 @@
local ffi = require('ffi')
local errno = require('errno')
local buffer = require('buffer')
+local cord_ibuf_take = buffer.internal.cord_ibuf_take
+local cord_ibuf_put = buffer.internal.cord_ibuf_put
ffi.cdef[[
typedef struct iconv *iconv_t;
@@ -33,10 +35,9 @@ local function iconv_convert(iconv, data)
-- prepare at lease BUF_SIZE and at most data_len bytes in shared buffer
local output_len = data_len >= BUF_SIZE and data_len or BUF_SIZE
- local buf = buffer.IBUF_SHARED;
+ local buf = cord_ibuf_take();
local buf_ptr = char_ptr_arr_t()
local buf_left = size_t_arr_t()
- buf:reset()
while data_left[0] > 0 do
buf_ptr[0] = buf:reserve(output_len)
@@ -46,6 +47,7 @@ local function iconv_convert(iconv, data)
if res == ffi.cast('size_t', -1) then
local err = errno()
if err ~= E2BIG then
+ cord_ibuf_put(buf)
ffi.C.tnt_iconv(iconv, nil, nil, nil, nil)
if err == EINVAL then
error('Invalid multibyte sequence')
@@ -62,7 +64,7 @@ local function iconv_convert(iconv, data)
-- iconv function sets cd's conversion state to the initial state
ffi.C.tnt_iconv(iconv, nil, nil, nil, nil)
local result = ffi.string(buf.rpos, buf:size())
- buf:reset()
+ cord_ibuf_put(buf)
return result
end
diff --git a/src/lua/init.c b/src/lua/init.c
index 767abdfc5..89d7f8f73 100644
--- a/src/lua/init.c
+++ b/src/lua/init.c
@@ -73,8 +73,6 @@
* The single Lua state of the transaction processor (tx) thread.
*/
struct lua_State *tarantool_L;
-static struct ibuf tarantool_lua_ibuf_body;
-struct ibuf *tarantool_lua_ibuf = &tarantool_lua_ibuf_body;
/**
* The fiber running the startup Lua script
*/
@@ -453,7 +451,6 @@ tarantool_lua_init(const char *tarantool_bin, int argc, char **argv)
if (L == NULL) {
panic("failed to initialize Lua");
}
- ibuf_create(tarantool_lua_ibuf, tarantool_lua_slab_cache(), 16000);
luaL_openlibs(L);
tarantool_lua_setpaths(L);
diff --git a/src/lua/msgpack.c b/src/lua/msgpack.c
index 24b0d2ccd..1e74a6a3c 100644
--- a/src/lua/msgpack.c
+++ b/src/lua/msgpack.c
@@ -46,6 +46,7 @@
#include "lib/uuid/mp_uuid.h" /* mp_decode_uuid() */
#include "lib/core/mp_extension_types.h"
+#include "cord_buf.h"
#include <fiber.h>
void
@@ -362,8 +363,7 @@ lua_msgpack_encode(lua_State *L)
"must be of type 'struct ibuf'");
}
} else {
- buf = tarantool_lua_ibuf;
- ibuf_reset(buf);
+ buf = cord_ibuf_take();
}
size_t used = ibuf_used(buf);
@@ -380,7 +380,7 @@ lua_msgpack_encode(lua_State *L)
lua_pushinteger(L, ibuf_used(buf) - used);
} else {
lua_pushlstring(L, buf->buf, ibuf_used(buf));
- ibuf_reinit(buf);
+ cord_ibuf_drop(buf);
}
return 1;
}
diff --git a/src/lua/msgpackffi.lua b/src/lua/msgpackffi.lua
index 2bc374742..6c245d7aa 100644
--- a/src/lua/msgpackffi.lua
+++ b/src/lua/msgpackffi.lua
@@ -10,6 +10,8 @@ local uint16_ptr_t = ffi.typeof('uint16_t *')
local uint32_ptr_t = ffi.typeof('uint32_t *')
local uint64_ptr_t = ffi.typeof('uint64_t *')
local char_ptr_t = ffi.typeof('char *')
+local cord_ibuf_take = buffer.internal.cord_ibuf_take
+local cord_ibuf_drop = buffer.internal.cord_ibuf_drop
ffi.cdef([[
char *
@@ -294,11 +296,10 @@ local function encode_r(buf, obj, level)
end
local function encode(obj)
- local tmpbuf = buffer.IBUF_SHARED
- tmpbuf:reset()
+ local tmpbuf = cord_ibuf_take()
encode_r(tmpbuf, obj, 0)
local r = ffi.string(tmpbuf.rpos, tmpbuf:size())
- tmpbuf:recycle()
+ cord_ibuf_drop(tmpbuf)
return r
end
diff --git a/src/lua/socket.lua b/src/lua/socket.lua
index 78d91f010..7c24b5655 100644
--- a/src/lua/socket.lua
+++ b/src/lua/socket.lua
@@ -13,6 +13,8 @@ local buffer = require('buffer')
local reg1 = buffer.reg1
local reg2 = buffer.reg2
local static_alloc = buffer.static_alloc
+local cord_ibuf_take = buffer.internal.cord_ibuf_take
+local cord_ibuf_drop = buffer.internal.cord_ibuf_drop
local format = string.format
@@ -296,19 +298,15 @@ local function socket_sysread(self, arg1, arg2)
error('socket:sysread(): size can not be negative')
end
- local buf = buffer.IBUF_SHARED
- buf:reset()
+ local buf = cord_ibuf_take()
local p = buf:alloc(size)
local res = sysread(self, p, size)
if res then
- local str = ffi.string(p, res)
- buf:recycle()
- return str
- else
- buf:recycle()
- return res
+ res = ffi.string(p, res)
end
+ cord_ibuf_drop(buf)
+ return res
end
local function socket_nonblock(self, nb)
diff --git a/src/lua/swim.lua b/src/lua/swim.lua
index 1da55337a..42b0d15ef 100644
--- a/src/lua/swim.lua
+++ b/src/lua/swim.lua
@@ -6,6 +6,8 @@ local crypto = require('crypto')
local fiber = require('fiber')
local internal = require('swim')
local schedule_task = fiber._internal.schedule_task
+local cord_ibuf_take = buffer.internal.cord_ibuf_take
+local cord_ibuf_put = buffer.internal.cord_ibuf_put
ffi.cdef[[
struct swim;
@@ -655,14 +657,17 @@ end
local function swim_set_payload(s, payload)
local func_name = 'swim:set_payload'
local ptr = swim_check_instance(s, func_name)
- local payload_size = 0
- if payload ~= nil then
- local buf = buffer.IBUF_SHARED
- buf:reset()
- payload_size = msgpack.encode(payload, buf)
+ local rc
+ if payload == nil then
+ rc = capi.swim_set_payload(ptr, nil, 0)
+ else
+ local buf = cord_ibuf_take()
+ local payload_size = msgpack.encode(payload, buf)
payload = buf.rpos
+ rc = capi.swim_set_payload(ptr, payload, payload_size)
+ cord_ibuf_put(buf)
end
- if capi.swim_set_payload(ptr, payload, payload_size) ~= 0 then
+ if rc ~= 0 then
return nil, box.error.last()
end
return true
diff --git a/src/lua/utf8.c b/src/lua/utf8.c
index 9c678cad4..bf9bb98f4 100644
--- a/src/lua/utf8.c
+++ b/src/lua/utf8.c
@@ -33,12 +33,11 @@
#include "coll/coll.h"
#include "lua/utils.h"
#include "lua/utf8.h"
+#include "cord_buf.h"
#include "diag.h"
#include "small/ibuf.h"
#include "tt_static.h"
-extern struct ibuf *tarantool_lua_ibuf;
-
/** Collations for cmp/casecmp functions. */
static struct coll *unicode_coll = NULL;
static struct coll *unicode_ci_coll = NULL;
@@ -52,12 +51,13 @@ utf8_str_to_case(struct lua_State *L, const char *src, int src_bsize,
int i = 0;
int dst_bsize = src_bsize;
(void) i;
+ struct ibuf *ibuf = cord_ibuf_take();
do {
UErrorCode err = U_ZERO_ERROR;
- ibuf_reset(tarantool_lua_ibuf);
- char *dst = ibuf_alloc(tarantool_lua_ibuf, dst_bsize);
+ char *dst = ibuf_alloc(ibuf, dst_bsize);
if (dst == NULL) {
diag_set(OutOfMemory, dst_bsize, "ibuf_alloc", "dst");
+ cord_ibuf_put(ibuf);
return luaT_error(L);
}
int real_bsize;
@@ -73,11 +73,13 @@ utf8_str_to_case(struct lua_State *L, const char *src, int src_bsize,
if (err == U_ZERO_ERROR ||
err == U_STRING_NOT_TERMINATED_WARNING) {
lua_pushlstring(L, dst, real_bsize);
+ cord_ibuf_put(ibuf);
return 1;
} else if (err == U_BUFFER_OVERFLOW_ERROR) {
assert(real_bsize > dst_bsize);
dst_bsize = real_bsize;
} else {
+ cord_ibuf_put(ibuf);
lua_pushnil(L);
lua_pushstring(L, tt_sprintf("error during ICU case "\
"transform: %s",
@@ -249,9 +251,10 @@ utf8_char(struct lua_State *L)
return 1;
}
/* Slow way - use dynamic buffer. */
- ibuf_reset(tarantool_lua_ibuf);
- char *str = ibuf_alloc(tarantool_lua_ibuf, top * U8_MAX_LENGTH);
+ struct ibuf *ibuf = cord_ibuf_take();
+ char *str = ibuf_alloc(ibuf, top * U8_MAX_LENGTH);
if (str == NULL) {
+ cord_ibuf_put(ibuf);
diag_set(OutOfMemory, top * U8_MAX_LENGTH, "ibuf_alloc",
"str");
return luaT_error(L);
@@ -261,6 +264,7 @@ utf8_char(struct lua_State *L)
U8_APPEND_UNSAFE(str, len, c);
}
lua_pushlstring(L, str, len);
+ cord_ibuf_put(ibuf);
return 1;
}
diff --git a/src/lua/utils.h b/src/lua/utils.h
index 37531676d..4a164868b 100644
--- a/src/lua/utils.h
+++ b/src/lua/utils.h
@@ -72,7 +72,6 @@ struct tt_uuid;
* snprintf(m_errmsg, sizeof(m_errmsg), "%s", msg ? msg : "");
*/
extern struct lua_State *tarantool_L;
-extern struct ibuf *tarantool_lua_ibuf;
struct tt_uuid *
luaL_pushuuid(struct lua_State *L);
diff --git a/static-build/test/static-build/exports.test.lua b/static-build/test/static-build/exports.test.lua
index 9b9eaa471..d5dbe7de9 100755
--- a/static-build/test/static-build/exports.test.lua
+++ b/static-build/test/static-build/exports.test.lua
@@ -26,6 +26,9 @@ local check_symbols = {
'guava',
'base64_decode',
'base64_encode',
+ 'cord_ibuf_drop',
+ 'cord_ibuf_put',
+ 'cord_ibuf_take',
'SHA1internal',
'random_bytes',
'fiber_time',
@@ -42,7 +45,6 @@ local check_symbols = {
'exception_get_int',
'exception_get_string',
- 'tarantool_lua_ibuf',
'uuid_nil',
'tt_uuid_create',
'tt_uuid_str',
diff --git a/test/unit/luaT_tuple_new.c b/test/unit/luaT_tuple_new.c
index 965b2e6e0..1d7c9072a 100644
--- a/test/unit/luaT_tuple_new.c
+++ b/test/unit/luaT_tuple_new.c
@@ -25,8 +25,6 @@
* box/tuple.test.lua.
*/
-extern struct ibuf *tarantool_lua_ibuf;
-
uint32_t
min_u32(uint32_t a, uint32_t b)
{
@@ -184,8 +182,6 @@ main()
memory_init();
fiber_init(fiber_c_invoke);
- ibuf_create(tarantool_lua_ibuf, &cord()->slabc, 16000);
-
struct lua_State *L = luaL_newstate();
luaL_openlibs(L);
--
2.24.3 (Apple Git-128)
^ permalink raw reply [flat|nested] 32+ messages in thread* [Tarantool-patches] [PATCH 06/16] cord_buf: introduce ownership management
2021-03-20 0:42 [Tarantool-patches] [PATCH 00/16] Cord buffer, static alloc, and Lua GC bug Vladislav Shpilevoy via Tarantool-patches
` (11 preceding siblings ...)
2021-03-20 0:42 ` [Tarantool-patches] [PATCH 05/16] cord_buf: introduce cord_buf API Vladislav Shpilevoy via Tarantool-patches
@ 2021-03-20 0:42 ` Vladislav Shpilevoy via Tarantool-patches
2021-03-22 16:48 ` Serge Petrenko via Tarantool-patches
2021-03-20 0:42 ` [Tarantool-patches] [PATCH 07/16] buffer: implement ffi stash Vladislav Shpilevoy via Tarantool-patches
` (8 subsequent siblings)
21 siblings, 1 reply; 32+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-03-20 0:42 UTC (permalink / raw)
To: tarantool-patches, gorcunov, sergepetrenko
The global ibuf used for hot Lua and Lua C code didn't have
ownership management. As a result, it could be reused in some
unexpected ways during Lua GC via __gc handlers, even if it was
currently in use in some code below the stack.
The patch makes cord_ibuf_take() steal the global buffer from its
global stash, and assign to the current fiber. cord_ibuf_put()
puts it back to the stash, and detaches from the fiber. If yield
happens before cord_ibuf_put(), the buffer is detached
automatically.
Fiber attach/detach is done via on_yield/on_stop triggers. The
buffer is not supposed to survive a yield, so this allows to
free/put the buffer back to the stash even if the owner didn't do
that. For instance, if a Lua exception was raised before
cord_ibuf_put() was called.
This makes cord buffer being safe to use in any yield-free code,
even if Lua GC might be started. And in non-Lua code as well.
Part of #5632
---
src/lib/core/cord_buf.c | 150 +++++++++++++++++++++++++++++++----
src/lib/core/cord_buf.h | 6 +-
test/app-tap/buffer.test.lua | 59 ++++++++++++++
3 files changed, 199 insertions(+), 16 deletions(-)
create mode 100755 test/app-tap/buffer.test.lua
diff --git a/src/lib/core/cord_buf.c b/src/lib/core/cord_buf.c
index cac508c3d..9450d75bc 100644
--- a/src/lib/core/cord_buf.c
+++ b/src/lib/core/cord_buf.c
@@ -5,6 +5,7 @@
*/
#include "cord_buf.h"
#include "fiber.h"
+#include "trigger.h"
#include "small/ibuf.h"
@@ -13,35 +14,154 @@ enum {
CORD_IBUF_START_CAPACITY = 16384,
};
-static struct ibuf *cord_buf_global = NULL;
+/** Global buffer with automatic collection on fiber yield. */
+struct cord_buf {
+ /** Base buffer. */
+ struct ibuf base;
+ /**
+ * Triggers on fiber stop/yield when the buffer is either destroyed or
+ * cached to the global stash for later reuse.
+ */
+ struct trigger on_stop;
+ struct trigger on_yield;
+#if !NDEBUG
+ /**
+ * Fiber owning the buffer right now. Used for debug and sanity checks
+ * only.
+ */
+ struct fiber *owner;
+#endif
+};
-struct ibuf *
-cord_ibuf_take(void)
+/**
+ * The global buffer last saved to the cache. Having it here is supposed to
+ * help to reuse the buffer's already allocated data sometimes.
+ */
+static struct cord_buf *cord_buf_global = NULL;
+
+static inline void
+cord_buf_put(struct cord_buf *buf);
+
+static void
+cord_buf_delete(struct cord_buf *buf);
+
+static inline void
+cord_buf_set_owner(struct cord_buf *buf)
{
- assert(cord_is_main());
- struct ibuf *buf = cord_buf_global;
- if (buf != NULL) {
- ibuf_reset(buf);
- return buf;
- }
- buf = malloc(sizeof(*buf));
+ assert(buf->owner == NULL);
+ struct fiber *f = fiber();
+ trigger_add(&f->on_stop, &buf->on_stop);
+ trigger_add(&f->on_yield, &buf->on_yield);
+#if !NDEBUG
+ buf->owner = f;
+#endif
+ ibuf_reset(&buf->base);
+}
+
+static inline void
+cord_buf_clear_owner(struct cord_buf *buf)
+{
+ assert(buf->owner == fiber());
+ trigger_clear(&buf->on_stop);
+ trigger_clear(&buf->on_yield);
+#if !NDEBUG
+ buf->owner = NULL;
+#endif
+}
+
+static int
+cord_buf_on_stop(struct trigger *trigger, void *event)
+{
+ (void)event;
+ struct cord_buf *buf = trigger->data;
+ assert(trigger == &buf->on_stop);
+ cord_buf_put(buf);
+ return 0;
+}
+
+static int
+cord_buf_on_yield(struct trigger *trigger, void *event)
+{
+ (void)event;
+ struct cord_buf *buf = trigger->data;
+ assert(trigger == &buf->on_yield);
+ cord_buf_put(buf);
+ return 0;
+}
+
+static struct cord_buf *
+cord_buf_new(void)
+{
+ struct cord_buf *buf = malloc(sizeof(*buf));
if (buf == NULL)
panic("Couldn't allocate thread buffer");
- ibuf_create(buf, &cord()->slabc, CORD_IBUF_START_CAPACITY);
- cord_buf_global = buf;
+ ibuf_create(&buf->base, &cord()->slabc, CORD_IBUF_START_CAPACITY);
+ trigger_create(&buf->on_stop, cord_buf_on_stop, buf, NULL);
+ trigger_create(&buf->on_yield, cord_buf_on_yield, buf, NULL);
+#if !NDEBUG
+ buf->owner = NULL;
+#endif
+ return buf;
+}
+
+static inline void
+cord_buf_put(struct cord_buf *buf)
+{
+ assert(cord_is_main());
+ cord_buf_clear_owner(buf);
+ /*
+ * Delete if the stash is busy. It could happen if there was >= 2
+ * buffers at some point and one of them is already saved back to the
+ * stash.
+ *
+ * XXX: in future it might be useful to consider saving the buffers into
+ * a list. Maybe keep always at most 2 buffers, because usually there
+ * are at most 2 contexts: normal Lua and Lua during GC. Recursive
+ * GC is supposed to be rare, no need to optimize it.
+ */
+ if (cord_buf_global == NULL)
+ cord_buf_global = buf;
+ else
+ cord_buf_delete(buf);
+}
+
+static inline struct cord_buf *
+cord_buf_take(void)
+{
+ assert(cord_is_main());
+ struct cord_buf *buf = cord_buf_global;
+ if (buf != NULL)
+ cord_buf_global = NULL;
+ else
+ buf = cord_buf_new();
+ cord_buf_set_owner(buf);
return buf;
}
+static void
+cord_buf_delete(struct cord_buf *buf)
+{
+ assert(buf->owner == NULL);
+ ibuf_destroy(&buf->base);
+ TRASH(buf);
+ free(buf);
+}
+
+struct ibuf *
+cord_ibuf_take(void)
+{
+ return &cord_buf_take()->base;
+}
+
void
cord_ibuf_put(struct ibuf *ibuf)
{
- (void)ibuf;
- assert(ibuf == cord_buf_global);
+ cord_buf_put((struct cord_buf *)ibuf);
}
void
cord_ibuf_drop(struct ibuf *ibuf)
{
ibuf_reinit(ibuf);
- assert(ibuf == cord_buf_global);
+ cord_ibuf_put(ibuf);
}
diff --git a/src/lib/core/cord_buf.h b/src/lib/core/cord_buf.h
index 59f429c8f..5e65d138b 100644
--- a/src/lib/core/cord_buf.h
+++ b/src/lib/core/cord_buf.h
@@ -18,7 +18,9 @@ struct ibuf *
cord_ibuf_take(void);
/**
- * Put the global ibuf back.
+ * Put the global ibuf back. It is not necessary - the buffer is put back on the
+ * next yield. But then it can't be reused/freed until the yield. Put it back
+ * manually when possible.
*/
void
cord_ibuf_put(struct ibuf *ibuf);
@@ -29,6 +31,8 @@ cord_ibuf_put(struct ibuf *ibuf);
* because it is often needed from Lua, and allows not to call :recycle() there,
* which would be an additional FFI call before cord_ibuf_put().
*
+ * Drop is not necessary though, see the put() comment.
+ *
* XXX: recycle of the global buffer is a workaround for the ibuf being used in
* some places working with Lua API, where it wasn't wanted to "reuse" it
* anyhow. Instead, the global buffer is used to protect from the buffer leak in
diff --git a/test/app-tap/buffer.test.lua b/test/app-tap/buffer.test.lua
new file mode 100755
index 000000000..f57b3cf45
--- /dev/null
+++ b/test/app-tap/buffer.test.lua
@@ -0,0 +1,59 @@
+#!/usr/bin/env tarantool
+
+local tap = require('tap')
+local fiber = require('fiber')
+local buffer = require('buffer')
+local cord_ibuf_take = buffer.internal.cord_ibuf_take
+local cord_ibuf_put = buffer.internal.cord_ibuf_put
+local cord_ibuf_drop = buffer.internal.cord_ibuf_drop
+
+local function test_cord_ibuf(test)
+ test:plan(10)
+
+ local ibuf1 = cord_ibuf_take()
+ test:is(ibuf1:size(), 0, 'is empty')
+ ibuf1:alloc(1)
+ test:is(ibuf1:size(), 1, 'alloc 1')
+ cord_ibuf_put(ibuf1)
+
+ ibuf1 = cord_ibuf_take()
+ test:is(ibuf1:size(), 0, 'is empty again')
+ ibuf1:alloc(1)
+ cord_ibuf_drop(ibuf1)
+
+ ibuf1 = cord_ibuf_take()
+ test:is(ibuf1:capacity(), 0, 'has no capacity')
+ local pos1 = ibuf1:alloc(1)
+ pos1[0] = 1
+
+ local ibuf2 = cord_ibuf_take()
+ test:isnt(ibuf1, ibuf2, 'can have 2 cord buffers')
+ test:is(ibuf2:size(), 0, 'second is empty')
+ local pos2 = ibuf2:alloc(1)
+ pos2[0] = 2
+ test:is(pos1[0], 1, 'change does not affect the first buffer')
+ cord_ibuf_put(ibuf2)
+ ibuf1 = ibuf2
+
+ fiber.yield()
+ ibuf2 = cord_ibuf_take()
+ test:is(ibuf1, ibuf2, 'yield drops the ownership')
+ cord_ibuf_put(ibuf2)
+
+ ibuf1 = nil
+ local f = fiber.new(function()
+ ibuf1 = cord_ibuf_take()
+ end)
+ f:set_joinable(true)
+ f:join()
+ test:isnt(ibuf1, nil, 'took a cord buf in a new fiber')
+ ibuf2 = cord_ibuf_take()
+ test:is(ibuf1, ibuf2, 'was freed on fiber stop and reused')
+ cord_ibuf_put(ibuf2)
+end
+
+local test = tap.test('buffer')
+test:plan(1)
+test:test("cord buffer", test_cord_ibuf)
+
+os.exit(test:check() and 0 or 1)
--
2.24.3 (Apple Git-128)
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [Tarantool-patches] [PATCH 06/16] cord_buf: introduce ownership management
2021-03-20 0:42 ` [Tarantool-patches] [PATCH 06/16] cord_buf: introduce ownership management Vladislav Shpilevoy via Tarantool-patches
@ 2021-03-22 16:48 ` Serge Petrenko via Tarantool-patches
2021-03-22 22:32 ` Vladislav Shpilevoy via Tarantool-patches
0 siblings, 1 reply; 32+ messages in thread
From: Serge Petrenko via Tarantool-patches @ 2021-03-22 16:48 UTC (permalink / raw)
To: Vladislav Shpilevoy, tarantool-patches, gorcunov
20.03.2021 03:42, Vladislav Shpilevoy пишет:
> The global ibuf used for hot Lua and Lua C code didn't have
> ownership management. As a result, it could be reused in some
> unexpected ways during Lua GC via __gc handlers, even if it was
> currently in use in some code below the stack.
>
> The patch makes cord_ibuf_take() steal the global buffer from its
> global stash, and assign to the current fiber. cord_ibuf_put()
> puts it back to the stash, and detaches from the fiber. If yield
> happens before cord_ibuf_put(), the buffer is detached
> automatically.
>
> Fiber attach/detach is done via on_yield/on_stop triggers. The
> buffer is not supposed to survive a yield, so this allows to
> free/put the buffer back to the stash even if the owner didn't do
> that. For instance, if a Lua exception was raised before
> cord_ibuf_put() was called.
>
> This makes cord buffer being safe to use in any yield-free code,
> even if Lua GC might be started. And in non-Lua code as well.
>
> Part of #5632
> ---
> src/lib/core/cord_buf.c | 150 +++++++++++++++++++++++++++++++----
> src/lib/core/cord_buf.h | 6 +-
> test/app-tap/buffer.test.lua | 59 ++++++++++++++
> 3 files changed, 199 insertions(+), 16 deletions(-)
> create mode 100755 test/app-tap/buffer.test.lua
>
> diff --git a/src/lib/core/cord_buf.c b/src/lib/core/cord_buf.c
> index cac508c3d..9450d75bc 100644
> --- a/src/lib/core/cord_buf.c
> +++ b/src/lib/core/cord_buf.c
> @@ -5,6 +5,7 @@
> */
> #include "cord_buf.h"
> #include "fiber.h"
> +#include "trigger.h"
>
> #include "small/ibuf.h"
>
> @@ -13,35 +14,154 @@ enum {
> CORD_IBUF_START_CAPACITY = 16384,
> };
>
> -static struct ibuf *cord_buf_global = NULL;
> +/** Global buffer with automatic collection on fiber yield. */
> +struct cord_buf {
> + /** Base buffer. */
> + struct ibuf base;
> + /**
> + * Triggers on fiber stop/yield when the buffer is either destroyed or
> + * cached to the global stash for later reuse.
> + */
> + struct trigger on_stop;
> + struct trigger on_yield;
> +#if !NDEBUG
Shouldn't it be `#if !defined(NDEBUG)`? I'm not sure, just asking.
--
Serge Petrenko
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [Tarantool-patches] [PATCH 06/16] cord_buf: introduce ownership management
2021-03-22 16:48 ` Serge Petrenko via Tarantool-patches
@ 2021-03-22 22:32 ` Vladislav Shpilevoy via Tarantool-patches
2021-03-23 7:46 ` Serge Petrenko via Tarantool-patches
0 siblings, 1 reply; 32+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-03-22 22:32 UTC (permalink / raw)
To: Serge Petrenko, tarantool-patches, gorcunov
Hi! Thanks for the review!
>> diff --git a/src/lib/core/cord_buf.c b/src/lib/core/cord_buf.c
>> index cac508c3d..9450d75bc 100644
>> --- a/src/lib/core/cord_buf.c
>> +++ b/src/lib/core/cord_buf.c
>> @@ -5,6 +5,7 @@
>> */
>> #include "cord_buf.h"
>> #include "fiber.h"
>> +#include "trigger.h"
>> #include "small/ibuf.h"
>> @@ -13,35 +14,154 @@ enum {
>> CORD_IBUF_START_CAPACITY = 16384,
>> };
>> -static struct ibuf *cord_buf_global = NULL;
>> +/** Global buffer with automatic collection on fiber yield. */
>> +struct cord_buf {
>> + /** Base buffer. */
>> + struct ibuf base;
>> + /**
>> + * Triggers on fiber stop/yield when the buffer is either destroyed or
>> + * cached to the global stash for later reuse.
>> + */
>> + struct trigger on_stop;
>> + struct trigger on_yield;
>> +#if !NDEBUG
>
> Shouldn't it be `#if !defined(NDEBUG)`? I'm not sure, just asking.
Yes, totally forgot about it.
====================
diff --git a/src/lib/core/cord_buf.c b/src/lib/core/cord_buf.c
index 9450d75bc..1d0fb3a41 100644
--- a/src/lib/core/cord_buf.c
+++ b/src/lib/core/cord_buf.c
@@ -24,7 +24,7 @@ struct cord_buf {
*/
struct trigger on_stop;
struct trigger on_yield;
-#if !NDEBUG
+#ifndef NDEBUG
/**
* Fiber owning the buffer right now. Used for debug and sanity checks
* only.
@@ -52,7 +52,7 @@ cord_buf_set_owner(struct cord_buf *buf)
struct fiber *f = fiber();
trigger_add(&f->on_stop, &buf->on_stop);
trigger_add(&f->on_yield, &buf->on_yield);
-#if !NDEBUG
+#ifndef NDEBUG
buf->owner = f;
#endif
ibuf_reset(&buf->base);
@@ -64,7 +64,7 @@ cord_buf_clear_owner(struct cord_buf *buf)
assert(buf->owner == fiber());
trigger_clear(&buf->on_stop);
trigger_clear(&buf->on_yield);
-#if !NDEBUG
+#ifndef NDEBUG
buf->owner = NULL;
#endif
}
@@ -98,7 +98,7 @@ cord_buf_new(void)
ibuf_create(&buf->base, &cord()->slabc, CORD_IBUF_START_CAPACITY);
trigger_create(&buf->on_stop, cord_buf_on_stop, buf, NULL);
trigger_create(&buf->on_yield, cord_buf_on_yield, buf, NULL);
-#if !NDEBUG
+#ifndef NDEBUG
buf->owner = NULL;
#endif
return buf;
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [Tarantool-patches] [PATCH 06/16] cord_buf: introduce ownership management
2021-03-22 22:32 ` Vladislav Shpilevoy via Tarantool-patches
@ 2021-03-23 7:46 ` Serge Petrenko via Tarantool-patches
0 siblings, 0 replies; 32+ messages in thread
From: Serge Petrenko via Tarantool-patches @ 2021-03-23 7:46 UTC (permalink / raw)
To: Vladislav Shpilevoy, tarantool-patches, gorcunov
23.03.2021 01:32, Vladislav Shpilevoy пишет:
> Hi! Thanks for the review!
>
>>> diff --git a/src/lib/core/cord_buf.c b/src/lib/core/cord_buf.c
>>> index cac508c3d..9450d75bc 100644
>>> --- a/src/lib/core/cord_buf.c
>>> +++ b/src/lib/core/cord_buf.c
>>> @@ -5,6 +5,7 @@
>>> */
>>> #include "cord_buf.h"
>>> #include "fiber.h"
>>> +#include "trigger.h"
>>> #include "small/ibuf.h"
>>> @@ -13,35 +14,154 @@ enum {
>>> CORD_IBUF_START_CAPACITY = 16384,
>>> };
>>> -static struct ibuf *cord_buf_global = NULL;
>>> +/** Global buffer with automatic collection on fiber yield. */
>>> +struct cord_buf {
>>> + /** Base buffer. */
>>> + struct ibuf base;
>>> + /**
>>> + * Triggers on fiber stop/yield when the buffer is either destroyed or
>>> + * cached to the global stash for later reuse.
>>> + */
>>> + struct trigger on_stop;
>>> + struct trigger on_yield;
>>> +#if !NDEBUG
>> Shouldn't it be `#if !defined(NDEBUG)`? I'm not sure, just asking.
> Yes, totally forgot about it.
Thanks! LGTM.
>
> ====================
> diff --git a/src/lib/core/cord_buf.c b/src/lib/core/cord_buf.c
> index 9450d75bc..1d0fb3a41 100644
> --- a/src/lib/core/cord_buf.c
> +++ b/src/lib/core/cord_buf.c
> @@ -24,7 +24,7 @@ struct cord_buf {
> */
> struct trigger on_stop;
> struct trigger on_yield;
> -#if !NDEBUG
> +#ifndef NDEBUG
> /**
> * Fiber owning the buffer right now. Used for debug and sanity checks
> * only.
> @@ -52,7 +52,7 @@ cord_buf_set_owner(struct cord_buf *buf)
> struct fiber *f = fiber();
> trigger_add(&f->on_stop, &buf->on_stop);
> trigger_add(&f->on_yield, &buf->on_yield);
> -#if !NDEBUG
> +#ifndef NDEBUG
> buf->owner = f;
> #endif
> ibuf_reset(&buf->base);
> @@ -64,7 +64,7 @@ cord_buf_clear_owner(struct cord_buf *buf)
> assert(buf->owner == fiber());
> trigger_clear(&buf->on_stop);
> trigger_clear(&buf->on_yield);
> -#if !NDEBUG
> +#ifndef NDEBUG
> buf->owner = NULL;
> #endif
> }
> @@ -98,7 +98,7 @@ cord_buf_new(void)
> ibuf_create(&buf->base, &cord()->slabc, CORD_IBUF_START_CAPACITY);
> trigger_create(&buf->on_stop, cord_buf_on_stop, buf, NULL);
> trigger_create(&buf->on_yield, cord_buf_on_yield, buf, NULL);
> -#if !NDEBUG
> +#ifndef NDEBUG
> buf->owner = NULL;
> #endif
> return buf;
--
Serge Petrenko
^ permalink raw reply [flat|nested] 32+ messages in thread
* [Tarantool-patches] [PATCH 07/16] buffer: implement ffi stash
2021-03-20 0:42 [Tarantool-patches] [PATCH 00/16] Cord buffer, static alloc, and Lua GC bug Vladislav Shpilevoy via Tarantool-patches
` (12 preceding siblings ...)
2021-03-20 0:42 ` [Tarantool-patches] [PATCH 06/16] cord_buf: introduce ownership management Vladislav Shpilevoy via Tarantool-patches
@ 2021-03-20 0:42 ` Vladislav Shpilevoy via Tarantool-patches
2021-03-23 0:29 ` Vladislav Shpilevoy via Tarantool-patches
2021-03-20 0:42 ` [Tarantool-patches] [PATCH 08/16] uuid: replace static_alloc with " Vladislav Shpilevoy via Tarantool-patches
` (7 subsequent siblings)
21 siblings, 1 reply; 32+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-03-20 0:42 UTC (permalink / raw)
To: tarantool-patches, gorcunov, sergepetrenko
Buffer module now exposes ffi_stash_new() function which returns 2
functions take() and put().
FFI stash implements proper ownership of global heavy-to-create
objects which can only be created via FFI. Such as structs,
pointers, arrays.
It should help to fix buffer's registers (buffer.reg1,
buffer.reg2, buffer.reg_array), and other global FFI objects such
as 'struct port_c' in schema.lua.
The issue is that when these objects are global, they might be
re-used right during usage in case Lua starts GC and invokes
__gc handlers. Just like it happened with IBUF_SHARED and
static_alloc().
Part of #5632
---
src/lib/core/cord_buf.h | 5 ++++
src/lua/buffer.lua | 52 ++++++++++++++++++++++++++++++++++++++++-
2 files changed, 56 insertions(+), 1 deletion(-)
diff --git a/src/lib/core/cord_buf.h b/src/lib/core/cord_buf.h
index 5e65d138b..5ad5290c1 100644
--- a/src/lib/core/cord_buf.h
+++ b/src/lib/core/cord_buf.h
@@ -21,6 +21,11 @@ cord_ibuf_take(void);
* Put the global ibuf back. It is not necessary - the buffer is put back on the
* next yield. But then it can't be reused/freed until the yield. Put it back
* manually when possible.
+ *
+ * XXX: buffer auto-put could be made more robust via some amendments. One
+ * option - push a cdata with __gc handler on the stack which puts the buffer
+ * back, and disable it manually when all worked without errors. The cons is
+ * that it is expensive.
*/
void
cord_ibuf_put(struct ibuf *ibuf);
diff --git a/src/lua/buffer.lua b/src/lua/buffer.lua
index d5dbedb0a..9bbd1d98d 100644
--- a/src/lua/buffer.lua
+++ b/src/lua/buffer.lua
@@ -214,6 +214,55 @@ local function ibuf_new(arg)
errorf('Usage: ibuf([size])')
end
+--
+-- Stash keeps an FFI object for re-usage and helps to ensure the proper
+-- ownership. Is supposed to be used in yield-free code when almost always it is
+-- possible to put the taken object back.
+-- Then cost of the stash is almost the same as ffi.new() for small objects like
+-- 'int[1]' even when jitted. Examples:
+--
+-- * ffi.new('int[1]') is about ~0.4ns, while the stash take() + put() is about
+-- ~0.8ns;
+--
+-- * Much better on objects > 128 bytes in size. ffi.new('struct uri[1]') is
+-- ~300ns, while the stash is still ~0.8ns;
+--
+-- * For structs not allocated as an array is also much better than ffi.new().
+-- For instance, ffi.new('struct tt_uuid') is ~300ns, the stash is ~0.8ns.
+-- Even though 'struct tt_uuid' is 16 bytes;
+--
+local function ffi_stash_new(c_type)
+ local item = nil
+
+ local function take()
+ local res
+ -- This line is guaranteed to be GC-safe. GC is not invoked. Because
+ -- there are no allocation. So it can be considered 'atomic'.
+ res, item = item, nil
+ -- The next lines don't need to be atomic and can survive GC. The only
+ -- important part was to take the global item and set it to nil.
+ if res then
+ return res
+ end
+ return ffi.new(c_type)
+ end
+
+ local function put(i)
+ -- It is ok to rewrite the existing global item if it was set. Does
+ -- not matter. They are all the same.
+ item = i
+ end
+
+ -- Due to some random reason if the stash returns a table with methods it
+ -- works faster than returning them as multiple values. Regardless of how
+ -- the methods are used later. Even if the caller will cache take and put
+ -- methods anyway.
+ return {
+ take = take,
+ put = put,
+ }
+end
+
--
-- NOTE: ffi.new() with inlined size <= 128 works even faster
-- than this allocator. If your size is a constant <= 128 -
@@ -299,5 +348,6 @@ return {
-- Keep reference.
reg_array = reg_array,
reg1 = reg_array[0],
- reg2 = reg_array[1]
+ reg2 = reg_array[1],
+ ffi_stash_new = ffi_stash_new,
}
--
2.24.3 (Apple Git-128)
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [Tarantool-patches] [PATCH 07/16] buffer: implement ffi stash
2021-03-20 0:42 ` [Tarantool-patches] [PATCH 07/16] buffer: implement ffi stash Vladislav Shpilevoy via Tarantool-patches
@ 2021-03-23 0:29 ` Vladislav Shpilevoy via Tarantool-patches
0 siblings, 0 replies; 32+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-03-23 0:29 UTC (permalink / raw)
To: tarantool-patches, gorcunov, sergepetrenko
> diff --git a/src/lib/core/cord_buf.h b/src/lib/core/cord_buf.h
> index 5e65d138b..5ad5290c1 100644
> --- a/src/lib/core/cord_buf.h
> +++ b/src/lib/core/cord_buf.h
> @@ -21,6 +21,11 @@ cord_ibuf_take(void);
> * Put the global ibuf back. It is not necessary - the buffer is put back on the
> * next yield. But then it can't be reused/freed until the yield. Put it back
> * manually when possible.
> + *
> + * XXX: buffer auto-put could be made more robust via some amendments. One
> + * option - push a cdata with __gc handler on the stack which puts the buffer
> + * back, and disable it manually when all worked without errors. The cons is
> + * that it is expensive.
This comment has nothing to do with the commit. I added it
accidentally. Dropped on the branch.
^ permalink raw reply [flat|nested] 32+ messages in thread
* [Tarantool-patches] [PATCH 08/16] uuid: replace static_alloc with ffi stash
2021-03-20 0:42 [Tarantool-patches] [PATCH 00/16] Cord buffer, static alloc, and Lua GC bug Vladislav Shpilevoy via Tarantool-patches
` (13 preceding siblings ...)
2021-03-20 0:42 ` [Tarantool-patches] [PATCH 07/16] buffer: implement ffi stash Vladislav Shpilevoy via Tarantool-patches
@ 2021-03-20 0:42 ` Vladislav Shpilevoy via Tarantool-patches
2021-03-20 0:42 ` [Tarantool-patches] [PATCH 09/16] uuid: drop tt_uuid_str() from Lua Vladislav Shpilevoy via Tarantool-patches
` (6 subsequent siblings)
21 siblings, 0 replies; 32+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-03-20 0:42 UTC (permalink / raw)
To: tarantool-patches, gorcunov, sergepetrenko
static_alloc() appears not to be safe to use in Lua, because it
does not provide any ownership protection for the returned values.
The problem appears when something is allocated, then Lua GC
starts, and some __gc handlers might also use static_alloc(). In
Lua and in C - both lead to the buffer being corrupted in its
original usage place.
The patch is a part of activity of getting rid of static_alloc()
in Lua. It removes it from uuid Lua module and makes it use the
new FFI stash feature, which helps to cache frequently used and
heavy to allocate FFI values.
ffi.new() is not used, because
- It produces a new GC object;
- ffi.new('struct tt_uuid') costs around 300ns while FFI stash
costs around 0.8ns (although it is magically fixed when
ffi.new('struct tt_uuid[1]') is used);
- Without JIT ffi.new() costs about the same as the stash, ~280ns
for small objects like tt_uuid.
The patch makes uuid perf a bit better in the places where
static_alloc() was used, because its cost was around 7ns for one
allocation.
---
src/lua/uuid.lua | 102 ++++++++++++++++++++++++++++-------------------
1 file changed, 62 insertions(+), 40 deletions(-)
diff --git a/src/lua/uuid.lua b/src/lua/uuid.lua
index 9cebf3ace..74f8c924c 100644
--- a/src/lua/uuid.lua
+++ b/src/lua/uuid.lua
@@ -1,7 +1,7 @@
-- uuid.lua (internal file)
local ffi = require("ffi")
-local static_alloc = require('buffer').static_alloc
+local buffer = require('buffer')
local builtin = ffi.C
ffi.cdef[[
@@ -27,6 +27,9 @@ extern const struct tt_uuid uuid_nil;
local uuid_t = ffi.typeof('struct tt_uuid')
local UUID_STR_LEN = 36
local UUID_LEN = ffi.sizeof(uuid_t)
+local uuid_stash = buffer.ffi_stash_new(uuid_t)
+local uuid_stash_take = uuid_stash.take
+local uuid_stash_put = uuid_stash.put
local is_uuid = function(value)
return ffi.istype(uuid_t, value)
@@ -66,10 +69,12 @@ local uuid_tobin = function(uu, byteorder)
return error('Usage: uuid:bin([byteorder])')
end
if need_bswap(byteorder) then
- local uuidbuf = static_alloc('struct tt_uuid')
+ local uuidbuf = uuid_stash_take()
ffi.copy(uuidbuf, uu, UUID_LEN)
builtin.tt_uuid_bswap(uuidbuf)
- return ffi.string(ffi.cast('char *', uuidbuf), UUID_LEN)
+ uu = ffi.string(ffi.cast('char *', uuidbuf), UUID_LEN)
+ uuid_stash_put(uuidbuf)
+ return uu
end
return ffi.string(ffi.cast('char *', uu), UUID_LEN)
end
@@ -93,42 +98,29 @@ local uuid_isnil = function(uu)
return builtin.tt_uuid_is_nil(uu)
end
-local fromstr = function(str)
- local uu = static_alloc('struct tt_uuid')
- local rc = builtin.tt_uuid_from_string(str, uu)
- if rc ~= 0 then
- return nil
- end
- return uu
-end
-
-local to_uuid = function(value)
- if is_uuid(value) then
- return value
- end
- if type(value) == 'string' then
- return fromstr(value)
- end
- return nil
-end
-
-local check_uuid = function(value, index, err_lvl)
- value = to_uuid(value)
- if value ~= nil then
- return value
- end
-
+local function error_convert_arg(arg_index)
local err_fmt = 'incorrect value to convert to uuid as %d argument'
- error(err_fmt:format(index), err_lvl)
+ error(err_fmt:format(arg_index), 3)
end
local uuid_eq = function(lhs, rhs)
- rhs = to_uuid(rhs)
- if rhs == nil then
+ if not is_uuid(lhs) then
+ lhs, rhs = rhs, lhs
+ elseif is_uuid(rhs) then
+ return builtin.tt_uuid_is_equal(lhs, rhs)
+ end
+ if type(rhs) ~= 'string' then
return false
end
- lhs = check_uuid(lhs, 1, 3)
- return builtin.tt_uuid_is_equal(lhs, rhs)
+ local buf = uuid_stash_take()
+ local rc = builtin.tt_uuid_from_string(rhs, buf) == 0
+ if rc then
+ rc = builtin.tt_uuid_is_equal(lhs, buf)
+ else
+ rc = false
+ end
+ uuid_stash_put(buf)
+ return rc
end
local uuid_new = function()
@@ -138,20 +130,50 @@ local uuid_new = function()
end
local uuid_new_bin = function(byteorder)
- local uuidbuf = static_alloc('struct tt_uuid')
+ local uuidbuf = uuid_stash_take()
builtin.tt_uuid_create(uuidbuf)
- return uuid_tobin(uuidbuf, byteorder)
+ local res = uuid_tobin(uuidbuf, byteorder)
+ uuid_stash_put(uuidbuf)
+ return res
end
local uuid_new_str = function()
- local uuidbuf = static_alloc('struct tt_uuid')
+ local uuidbuf = uuid_stash_take()
builtin.tt_uuid_create(uuidbuf)
- return uuid_tostring(uuidbuf)
+ local res = uuid_tostring(uuidbuf)
+ uuid_stash_put(uuidbuf)
+ return res
end
local uuid_cmp = function(lhs, rhs)
- lhs = check_uuid(lhs, 1, 4)
- rhs = check_uuid(rhs, 2, 4)
- return builtin.tt_uuid_compare(lhs, rhs)
+ local buf
+ if is_uuid(lhs) then
+ if is_uuid(rhs) then
+ -- Fast path.
+ return builtin.tt_uuid_compare(lhs, rhs)
+ end
+ if type(rhs) ~= 'string' then
+ return error_convert_arg(2)
+ end
+ buf = uuid_stash_take()
+ if builtin.tt_uuid_from_string(rhs, buf) ~= 0 then
+ uuid_stash_put(buf)
+ return error_convert_arg(2)
+ end
+ rhs = buf
+ else
+ if type(lhs) ~= 'string' then
+ return error_convert_arg(1)
+ end
+ buf = uuid_stash_take()
+ if builtin.tt_uuid_from_string(lhs, buf) ~= 0 then
+ uuid_stash_put(buf)
+ return error_convert_arg(1)
+ end
+ lhs = buf
+ end
+ local rc = builtin.tt_uuid_compare(lhs, rhs)
+ uuid_stash_put(buf)
+ return rc
end
local uuid_lt = function(lhs, rhs)
return uuid_cmp(lhs, rhs) < 0
--
2.24.3 (Apple Git-128)
^ permalink raw reply [flat|nested] 32+ messages in thread* [Tarantool-patches] [PATCH 09/16] uuid: drop tt_uuid_str() from Lua
2021-03-20 0:42 [Tarantool-patches] [PATCH 00/16] Cord buffer, static alloc, and Lua GC bug Vladislav Shpilevoy via Tarantool-patches
` (14 preceding siblings ...)
2021-03-20 0:42 ` [Tarantool-patches] [PATCH 08/16] uuid: replace static_alloc with " Vladislav Shpilevoy via Tarantool-patches
@ 2021-03-20 0:42 ` Vladislav Shpilevoy via Tarantool-patches
2021-03-21 16:38 ` [Tarantool-patches] [PATCH 00/16] Cord buffer, static alloc, and Lua GC bug Vladislav Shpilevoy via Tarantool-patches
` (5 subsequent siblings)
21 siblings, 0 replies; 32+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-03-20 0:42 UTC (permalink / raw)
To: tarantool-patches, gorcunov, sergepetrenko
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
---
src/exports.h | 2 +-
src/lua/uuid.lua | 13 ++++--
test/app-tap/gh-5632-gc-buf-reuse.test.lua | 49 ++++++++++++++++++++++
3 files changed, 60 insertions(+), 4 deletions(-)
create mode 100755 test/app-tap/gh-5632-gc-buf-reuse.test.lua
diff --git a/src/exports.h b/src/exports.h
index ddbe57230..a90b9406a 100644
--- a/src/exports.h
+++ b/src/exports.h
@@ -517,7 +517,7 @@ EXPORT(tt_uuid_create)
EXPORT(tt_uuid_from_string)
EXPORT(tt_uuid_is_equal)
EXPORT(tt_uuid_is_nil)
-EXPORT(tt_uuid_str)
+EXPORT(tt_uuid_to_string)
EXPORT(uri_format)
EXPORT(uri_parse)
EXPORT(uuid_nil)
diff --git a/src/lua/uuid.lua b/src/lua/uuid.lua
index 74f8c924c..3047b665c 100644
--- a/src/lua/uuid.lua
+++ b/src/lua/uuid.lua
@@ -17,8 +17,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);
int
tt_uuid_compare(const struct tt_uuid *a, const struct tt_uuid *b);
extern const struct tt_uuid uuid_nil;
@@ -31,6 +29,11 @@ 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 is_uuid = function(value)
return ffi.istype(uuid_t, value)
end
@@ -39,7 +42,11 @@ local uuid_tostring = function(uu)
if not is_uuid(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..8fe662d3f
--- /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 i = 1, gc_count do
+ ffi.gc(ffi.new('char[1]'), function() uuid_to_str() end)
+ end
+ end
+
+ for i = 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)
--
2.24.3 (Apple Git-128)
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [Tarantool-patches] [PATCH 00/16] Cord buffer, static alloc, and Lua GC bug
2021-03-20 0:42 [Tarantool-patches] [PATCH 00/16] Cord buffer, static alloc, and Lua GC bug Vladislav Shpilevoy via Tarantool-patches
` (15 preceding siblings ...)
2021-03-20 0:42 ` [Tarantool-patches] [PATCH 09/16] uuid: drop tt_uuid_str() from Lua Vladislav Shpilevoy via Tarantool-patches
@ 2021-03-21 16:38 ` Vladislav Shpilevoy via Tarantool-patches
2021-03-22 7:52 ` Cyrill Gorcunov via Tarantool-patches
` (4 subsequent siblings)
21 siblings, 0 replies; 32+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-03-21 16:38 UTC (permalink / raw)
To: tarantool-patches, gorcunov, sergepetrenko
Also there is always an option to ban usage of almost everything in
__gc handlers except C functions and Lua fiber API (to wakeup a fiber
to do the deletion later in a normal context).
But someone should make that decision then. Currently it was said we
must support in __gc everything what does not yield.
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [Tarantool-patches] [PATCH 00/16] Cord buffer, static alloc, and Lua GC bug
2021-03-20 0:42 [Tarantool-patches] [PATCH 00/16] Cord buffer, static alloc, and Lua GC bug Vladislav Shpilevoy via Tarantool-patches
` (16 preceding siblings ...)
2021-03-21 16:38 ` [Tarantool-patches] [PATCH 00/16] Cord buffer, static alloc, and Lua GC bug Vladislav Shpilevoy via Tarantool-patches
@ 2021-03-22 7:52 ` Cyrill Gorcunov via Tarantool-patches
2021-03-22 7:56 ` Konstantin Osipov via Tarantool-patches
` (3 subsequent siblings)
21 siblings, 0 replies; 32+ messages in thread
From: Cyrill Gorcunov via Tarantool-patches @ 2021-03-22 7:52 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
On Sat, Mar 20, 2021 at 01:42:30AM +0100, Vladislav Shpilevoy wrote:
> The patch attempts to fix most of the easy to face issues with the global
> resources not having proper ownership in Lua code and therefore not protected
> again being suddenly reused during Lua GC.
>
> There are many such resources: tarantool_lua_ibuf/IBUF_SHARED, static alloc,
> errno, diag/box.error, box_tuple_last, port_c, and others.
>
> The most easy to screw - ibuf and static alloc. They are fixed in this
> patchset.
>
> The solution is dubious, but there were not found any better alternatives.
>
> A patch for 1.10 will be sent later in a separate CL.
>
> Branch: http://github.com/tarantool/tarantool/tree/gerold103/gh-5632-global-buf-crash-2x
> Issue: https://github.com/tarantool/tarantool/issues/5632
Vlad, the series look ok to me. But i think we need more intensive
testing and catch up if there is some missed. Thus I propose
to merge and get it a spin up while we have time.
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [Tarantool-patches] [PATCH 00/16] Cord buffer, static alloc, and Lua GC bug
2021-03-20 0:42 [Tarantool-patches] [PATCH 00/16] Cord buffer, static alloc, and Lua GC bug Vladislav Shpilevoy via Tarantool-patches
` (17 preceding siblings ...)
2021-03-22 7:52 ` Cyrill Gorcunov via Tarantool-patches
@ 2021-03-22 7:56 ` Konstantin Osipov via Tarantool-patches
2021-03-22 17:17 ` Serge Petrenko via Tarantool-patches
` (2 subsequent siblings)
21 siblings, 0 replies; 32+ messages in thread
From: Konstantin Osipov via Tarantool-patches @ 2021-03-22 7:56 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
* Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org> [21/03/22 10:02]:
> The patch attempts to fix most of the easy to face issues with the global
> resources not having proper ownership in Lua code and therefore not protected
> again being suddenly reused during Lua GC.
The policy has always been to know what you're doing in gc
handlers. Not use shared resources in particular. Not yield. Not
try to grab new resources. This is why, specifically, on_shutdown
triggers weren't implemented - it's impossible to make them both
generic and safe.
While this series perhaps won't harm, I doubt they will help much
if the user is a rookie.
--
Konstantin Osipov, Moscow, Russia
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [Tarantool-patches] [PATCH 00/16] Cord buffer, static alloc, and Lua GC bug
2021-03-20 0:42 [Tarantool-patches] [PATCH 00/16] Cord buffer, static alloc, and Lua GC bug Vladislav Shpilevoy via Tarantool-patches
` (18 preceding siblings ...)
2021-03-22 7:56 ` Konstantin Osipov via Tarantool-patches
@ 2021-03-22 17:17 ` Serge Petrenko via Tarantool-patches
2021-03-23 23:45 ` Vladislav Shpilevoy via Tarantool-patches
2021-03-24 13:28 ` Kirill Yukhin via Tarantool-patches
21 siblings, 0 replies; 32+ messages in thread
From: Serge Petrenko via Tarantool-patches @ 2021-03-22 17:17 UTC (permalink / raw)
To: Vladislav Shpilevoy, tarantool-patches, gorcunov
20.03.2021 03:42, Vladislav Shpilevoy пишет:
> The patch attempts to fix most of the easy to face issues with the global
> resources not having proper ownership in Lua code and therefore not protected
> again being suddenly reused during Lua GC.
>
> There are many such resources: tarantool_lua_ibuf/IBUF_SHARED, static alloc,
> errno, diag/box.error, box_tuple_last, port_c, and others.
>
> The most easy to screw - ibuf and static alloc. They are fixed in this
> patchset.
>
> The solution is dubious, but there were not found any better alternatives.
>
> A patch for 1.10 will be sent later in a separate CL.
>
> Branch: http://github.com/tarantool/tarantool/tree/gerold103/gh-5632-global-buf-crash-2x
> Issue: https://github.com/tarantool/tarantool/issues/5632
Hi! Thanks for the patchset!
Generally LGTM, with one question in reply to patch 06.
>
> Vladislav Shpilevoy (16):
> fio: don't use shared buffer in pread()
> test: don't use IBUF_SHARED in the tests
> tuple: pass global ibuf explicitly where possible
> iconv: take errno before reseting the context
> cord_buf: introduce cord_buf API
> cord_buf: introduce ownership management
> buffer: implement ffi stash
> uuid: replace static_alloc with ffi stash
> uuid: drop tt_uuid_str() from Lua
> uri: replace static_alloc with ffi stash and ibuf
> buffer: remove static_alloc() from Lua
> lua: use lua_pushfstring() instead of tt_sprintf()
> sio: rework sio_strfaddr()
> sio: increase SERVICE_NAME_MAXLEN size
> sio: introduce and use sio_snprintf()
> buffer: remove Lua registers
>
> changelogs/unreleased/fix-ibuf-static.md | 7 +
> src/CMakeLists.txt | 1 -
> src/box/iproto.cc | 8 +-
> src/box/iproto.h | 2 +-
> src/box/lua/info.c | 5 +-
> src/box/lua/net_box.lua | 4 +-
> src/box/lua/schema.lua | 56 ++++--
> src/box/lua/session.c | 7 +-
> src/box/lua/space.cc | 6 +-
> src/box/lua/tuple.c | 80 +++++----
> src/box/lua/tuple.lua | 14 +-
> src/exports.h | 7 +-
> src/lib/core/CMakeLists.txt | 1 +
> src/lib/core/cord_buf.c | 167 ++++++++++++++++++
> src/lib/core/cord_buf.h | 54 ++++++
> src/lib/core/sio.c | 72 ++++----
> src/lib/core/sio.h | 15 +-
> src/lua/buffer.c | 42 -----
> src/lua/buffer.lua | 156 +++++++++-------
> src/lua/crypto.lua | 19 +-
> src/lua/digest.lua | 25 ++-
> src/lua/fiber.c | 2 +-
> src/lua/fio.lua | 12 +-
> src/lua/iconv.lua | 27 +--
> src/lua/init.c | 3 -
> src/lua/msgpack.c | 6 +-
> src/lua/msgpackffi.lua | 57 +++---
> src/lua/socket.lua | 54 +++---
> src/lua/string.lua | 26 ++-
> src/lua/swim.lua | 19 +-
> src/lua/uri.lua | 21 ++-
> src/lua/utf8.c | 21 ++-
> src/lua/utils.h | 1 -
> src/lua/uuid.lua | 115 +++++++-----
> .../test/static-build/exports.test.lua | 4 +-
> test/app-tap/buffer.test.lua | 59 +++++++
> test/app-tap/gh-5632-gc-buf-reuse.test.lua | 151 ++++++++++++++++
> test/app-tap/module_api.test.lua | 10 +-
> test/app-tap/uri.test.lua | 23 +--
> test/app/buffer.result | 53 ------
> test/app/buffer.test.lua | 23 ---
> test/app/fio.result | 160 +++++++++--------
> test/app/fio.test.lua | 67 ++++---
> test/app/msgpack.result | 7 +-
> test/app/msgpack.test.lua | 5 +-
> test/box/varbinary_type.result | 3 +-
> test/box/varbinary_type.test.lua | 3 +-
> test/unit/luaT_tuple_new.c | 4 -
> 48 files changed, 1092 insertions(+), 592 deletions(-)
> create mode 100644 changelogs/unreleased/fix-ibuf-static.md
> create mode 100644 src/lib/core/cord_buf.c
> create mode 100644 src/lib/core/cord_buf.h
> delete mode 100644 src/lua/buffer.c
> create mode 100755 test/app-tap/buffer.test.lua
> create mode 100755 test/app-tap/gh-5632-gc-buf-reuse.test.lua
> delete mode 100644 test/app/buffer.result
> delete mode 100644 test/app/buffer.test.lua
>
--
Serge Petrenko
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [Tarantool-patches] [PATCH 00/16] Cord buffer, static alloc, and Lua GC bug
2021-03-20 0:42 [Tarantool-patches] [PATCH 00/16] Cord buffer, static alloc, and Lua GC bug Vladislav Shpilevoy via Tarantool-patches
` (19 preceding siblings ...)
2021-03-22 17:17 ` Serge Petrenko via Tarantool-patches
@ 2021-03-23 23:45 ` Vladislav Shpilevoy via Tarantool-patches
2021-03-24 13:28 ` Kirill Yukhin via Tarantool-patches
21 siblings, 0 replies; 32+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-03-23 23:45 UTC (permalink / raw)
To: tarantool-patches, gorcunov, sergepetrenko
I force pushed a few trivial updates for luacheck into several commits.
Kirill, if you are going to merge that, please, ensure the CI is green.
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [Tarantool-patches] [PATCH 00/16] Cord buffer, static alloc, and Lua GC bug
2021-03-20 0:42 [Tarantool-patches] [PATCH 00/16] Cord buffer, static alloc, and Lua GC bug Vladislav Shpilevoy via Tarantool-patches
` (20 preceding siblings ...)
2021-03-23 23:45 ` Vladislav Shpilevoy via Tarantool-patches
@ 2021-03-24 13:28 ` Kirill Yukhin via Tarantool-patches
21 siblings, 0 replies; 32+ messages in thread
From: Kirill Yukhin via Tarantool-patches @ 2021-03-24 13:28 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
Hello,
On 20 мар 01:42, Vladislav Shpilevoy via Tarantool-patches wrote:
> The patch attempts to fix most of the easy to face issues with the global
> resources not having proper ownership in Lua code and therefore not protected
> again being suddenly reused during Lua GC.
>
> There are many such resources: tarantool_lua_ibuf/IBUF_SHARED, static alloc,
> errno, diag/box.error, box_tuple_last, port_c, and others.
>
> The most easy to screw - ibuf and static alloc. They are fixed in this
> patchset.
>
> The solution is dubious, but there were not found any better alternatives.
>
> A patch for 1.10 will be sent later in a separate CL.
>
> Branch: http://github.com/tarantool/tarantool/tree/gerold103/gh-5632-global-buf-crash-2x
> Issue: https://github.com/tarantool/tarantool/issues/5632
I've checked your patchset into 2.6, 2.7 and master.
Could you please rebase it onto 1.10?
--
Reggards, Kirill Yukhin
^ permalink raw reply [flat|nested] 32+ messages in thread