[Tarantool-patches] [PATCH 11/16] buffer: remove static_alloc() from Lua

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

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)