[Tarantool-patches] [PATCH 16/16] buffer: remove Lua registers

[Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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)