Re: [Tarantool-patches] [PATCH luajit 1/2][v2] Fix embedded bytecode loader.

[Maxim Kokryashkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi, Sergey!
Thanks for the patch!
Please consider my comments below.
On Thu, Aug 31, 2023 at 02:30:38PM +0300, Sergey Bronnikov via Tarantool-patches wrote:
> From: Sergey Bronnikov <sergeyb@tarantool.org>
> 
> (cherry-picked from commit 820339960123dc78a7ce03edf53fcf4fdae0e55d)
> 
> The original problem is specific to x32 and is as follows: when a chunk
> with a bytecode library is loaded into memory, and the address is higher
> than 0x80000100, the `LexState->pe`, that contains an address of the end
> of the bytecode chunk in the memory, will wrap around and become smaller
> than the address in `LexState->p`, that contains an address of the
> beginning of bytecode chunk in the memory. In `bcread_fill()` called by
> `bcread_want()`, `memcpy()` is called with a very large size and causes
> bus error on x86 and segmentation fault on ARM Android.
> 
> The problem cannot be reproduced on platforms supported by Tarantool
> (ARM64, x86_64), so test doesn't reproduce a problem without a patch and
> tests the patch partially.
> 
> Sergey Bronnikov:
> * added the description and the test
> ---
>  src/lib_package.c                             |  4 +-
>  src/lj_bcread.c                               | 10 +-
>  src/lj_lex.c                                  |  6 ++
>  src/lj_lex.h                                  |  1 +
>  .../lj-549-bytecode-loader.test.lua           | 96 +++++++++++++++++++
>  5 files changed, 110 insertions(+), 7 deletions(-)
>  create mode 100644 test/tarantool-tests/lj-549-bytecode-loader.test.lua
> 
> diff --git a/src/lib_package.c b/src/lib_package.c
> index b49f0209..12603038 100644
> --- a/src/lib_package.c
> +++ b/src/lib_package.c
<snipped>

> diff --git a/src/lj_bcread.c b/src/lj_bcread.c
> index cddf6ff1..48ec15e4 100644
> --- a/src/lj_bcread.c
> +++ b/src/lj_bcread.c
<snipped>

> diff --git a/src/lj_lex.c b/src/lj_lex.c
> index cef3c683..6291705f 100644
> --- a/src/lj_lex.c
> +++ b/src/lj_lex.c
<snipped>

> diff --git a/src/lj_lex.h b/src/lj_lex.h
> index ae05a954..a26e504a 100644
> --- a/src/lj_lex.h
> +++ b/src/lj_lex.h
<snipped>

> diff --git a/test/tarantool-tests/lj-549-bytecode-loader.test.lua b/test/tarantool-tests/lj-549-bytecode-loader.test.lua
> new file mode 100644
> index 00000000..889be80a
> --- /dev/null
> +++ b/test/tarantool-tests/lj-549-bytecode-loader.test.lua
> @@ -0,0 +1,96 @@
> +local tap = require('tap')
> +local ffi = require('ffi')
> +local utils = require('utils')
> +local test = tap.test('lj-549-bytecode-loader'):skipcond({
> +    -- ['Test requires GC64 mode enabled'] = not require('ffi').abi('gc64'),
Why this skipcond is commented out?
> +})
> +
> +test:plan(1)
> +
> +-- Test creates a shared library with LuaJIT bytecode,
> +-- loads shared library as a Lua module and checks,
> +-- that no crashes eliminated.
> +--
> +-- $ make HOST_CC='gcc -m32' TARGET_CFLAGS='-m32' \
> +--                           TARGET_LDFLAGS='-m32' \
> +--                           TARGET_SHLDFLAGS='-m32' \
> +--                           -f Makefile.original
> +-- $ echo 'print("test")' > a.lua
> +-- $ LUA_PATH="src/?.lua;;" luajit -b a.lua a.c
> +-- $ gcc -m32 -fPIC -shared a.c -o a.so
> +-- $ luajit -e "require('a')"
> +-- Program received signal SIGBUS, Bus error
> +
> +local function file_exists(fname)
> +   return io.open(fname, 'r') or true and false
> +end
> +
> +local function get_file_name(file)
> +    return file:match("[^/]*$")
> +end
> +
> +local stdout_msg = 'Lango team'
> +local lua_code = ('print(%q)'):format(stdout_msg)
> +local fpath = os.tmpname()
> +local path_lua = ('%s.lua'):format(fpath)
> +local path_c = ('%s.c'):format(fpath)
> +local path_so = ('%s.so'):format(fpath)
> +
> +-- Create a file with a minimal Lua code.
> +local fh = assert(io.open(path_lua, 'w'))
> +fh:write(lua_code)
> +fh:close()
> +
> +local module_name = assert(get_file_name(fpath))
> +
> +local basedir = function(path)
> +    local sep = '/'
> +    return path:match('(.*' .. sep .. ')') or './'
> +end
> +
> +-- Create a C file with LuaJIT bytecode.
> +-- We cannot use utils.makecmd, because command-line generated
> +-- by `makecmd` contains `-e` that is incompatible with option `-b`.
> +local function create_c_file(pathlua, pathc)
> +  local lua_path = os.getenv('LUA_PATH')
> +  local lua_bin = require('utils').exec.luacmd(arg):match('%S+')
> +  local cmd_fmt = 'LUA_PATH="%s" %s -b %s %s'
> +  local cmd = (cmd_fmt):format(lua_path, lua_bin, pathlua, pathc)
> +  local ret = os.execute(cmd)
> +  assert(ret == 0, 'create a C file with bytecode')
> +end
> +
> +create_c_file(path_lua, path_c)
> +assert(file_exists(path_c))
> +
> +-- Compile C source code with LuaJIT bytecode to a shared library.
> +-- `-m64` is not available on ARM64, see
> +-- "3.18.1 AArch64 Options in the manual",
> +-- https://gcc.gnu.org/onlinedocs/gcc/AArch64-Options.html
> +local cflags_64 = jit.arch == 'arm64' and '-march=armv8-a' or '-m64'
> +local cflags = ffi.abi('32bit') and '-m32' or cflags_64
> +local cc_cmd = ('cc %s -fPIC -shared %s -o %s'):format(cflags, path_c, path_so)
> +local ph = io.popen(cc_cmd)
> +ph:close()
I suggest using the os.execute and checking the exit code.
Popen is excessive here.

> +assert(file_exists(path_so))
> +
> +-- Load shared library as a Lua module.
> +local lua_cpath = ('"/tmp/?.so;"'):format(basedir(fpath))
> +assert(file_exists(path_so))
> +local cmd = utils.exec.makecmd(arg, {
> +    script = ('-e "require([[%s]])"'):format(module_name),
> +    env = {
> +        LUA_CPATH = lua_cpath,
> +        -- It is required to cleanup LUA_PATH, otherwise
> +        -- LuaJIT loads Lua module, see tarantool-tests/utils/init.lua.
> +        LUA_PATH = '',
> +    },
> +})
> +local res = cmd()
> +test:ok(res == stdout_msg, 'bytecode loader works')
> +
> +os.remove(path_lua)
> +os.remove(path_c)
> +os.remove(path_so)
> +
> +os.exit(test:check() and 0 or 1)
`test:done` should be used instead.
> -- 
> 2.34.1
>