From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id E40DE53915C; Fri, 1 Sep 2023 12:42:14 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org E40DE53915C DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1693561335; bh=rP9MugJNeCa/fyTHHo7pC7pevXzW7UFoD7pe1VizTh4=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=GA+WrGaOv5pc92rX7+TSDBP+mAtMDXcG2ZadA1n38M0F0LZEdRNcPBpEbgNuijw1z B5qBg/2I0MLwY9N/Mtw+KYBfvyJW0hQDYw5fcqQB+BwXef5owmP6eLlLD7GDXQBtGR 6ymrNPfqnwP772xQCQQu+MxruMlHfdbyYIeRmeKg= Received: from smtp50.i.mail.ru (smtp50.i.mail.ru [95.163.41.92]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id C668A536486 for ; Fri, 1 Sep 2023 12:42:13 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org C668A536486 Received: by smtp50.i.mail.ru with esmtpa (envelope-from ) id 1qc0fG-00BYIW-3D; Fri, 01 Sep 2023 12:42:07 +0300 Date: Fri, 1 Sep 2023 12:42:06 +0300 To: Sergey Bronnikov Message-ID: References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Mailru-Src: smtp X-7564579A: EEAE043A70213CC8 X-77F55803: 4F1203BC0FB41BD96201AD55A1C8F7DB61D29A291ECBF3870B600DAE42344AB9182A05F5380850404C228DA9ACA6FE27DE384654301BFF624A2A6C65E7E5A7CE9428577137137538E36B399111AE7AF6 X-C1DE0DAB: 0D63561A33F958A5B658BCDD888899425AE6AE9819FD8C60D9D28241BCED210EF87CCE6106E1FC07E67D4AC08A07B9B0AD74539164518AE5CB5012B2E24CD356 X-C8649E89: 1C3962B70DF3F0AD5177F0B940C8B66ECE892A7B2722663E91682638B966EB3F662256BEEFA9527FC1A79046CF5364DB21EFE23BA014DBC702A9A7C0A0C06B8A2384379B532B8F610ACAE0F2C96F30E037FD76D11AF80A0DEC0B5C6CD4B747AFEB99FC5B3E49C465EA455F16B58544A21C197AAF4D2E4732965026E5D17F6739C77C69D99B9914278E50E1F0597A6FD5CD72808BE417F3B9E0E7457915DAA85F X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2bioj1b73jmY4IdZJibDoTlyNiA== X-Mailru-Sender: 11C2EC085EDE56FA38FD4C59F7EFE4079C6C490C09EAF1E80F9E1FBD6CB44181532CED4E0E5C03BCD51284F0FE6F529ABC7555A253F5B200DF104D74F62EE79D27EC13EC74F6107F4198E0F3ECE9B5443453F38A29522196 X-Mras: OK Subject: Re: [Tarantool-patches] [PATCH luajit 1/2][v2] Fix embedded bytecode loader. X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Maxim Kokryashkin via Tarantool-patches Reply-To: Maxim Kokryashkin Cc: max.kokryashkin@gmail.com, tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 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 > > (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 > 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 > 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 > 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 > 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 >