[Tarantool-patches] [PATCH] Fix saved bytecode encapsulated in ELF objects.
Sergey Bronnikov
sergeyb at tarantool.org
Mon May 29 17:34:17 MSK 2023
Hi, Sergey!
Thanks for review, please see my answers (and patches with fixes) below.
Changes force-pushed, and I'll send a second version to the mailing list.
Sergey
On 5/23/23 16:09, Sergey Kaplun wrote:
> Hi, Sergey!
> Thanks for the patch!
> Please consider my comments below.
>
> On 22.05.23, Sergey Bronnikov wrote:
>> From: Sergey Bronnikov <sergeyb at tarantool.org>
>>
>> Thanks to Dimitry Andric.
>>
>> (cherry picked from commit 7dbf0b05f1228c1c719866db5e5f3d58f87f74c8)
>>
>> Function `bcsave.lua:bcsave_elfobj()` produced an object file in ELF
>> format with a wrong size size of `.strtab`. Wrong size causes lld to
>> show an error message similar to: `ld: error: obj/bytecode.o: string
>> table non-null terminated`.
>>
>> Sergey Bronnikov:
>> * added the description and the test for the problem
>>
>> Signed-off-by: Sergey Bronnikov <sergeyb at tarantool.org>
>> ---
>> Branch: https://github.com/tarantool/luajit/tree/ligurio/gh-366-strtab-section-correct-size
>> PR: https://github.com/tarantool/tarantool/pull/8678
>> ---
>> src/jit/bcsave.lua | 2 +-
>> .../lj-366-strtab-correct-size.test.lua | 148 ++++++++++++++++++
>> 2 files changed, 149 insertions(+), 1 deletion(-)
>> create mode 100644 test/tarantool-tests/lj-366-strtab-correct-size.test.lua
>>
>> diff --git a/src/jit/bcsave.lua b/src/jit/bcsave.lua
>> index c17c88e0..2553d97e 100644
>> --- a/src/jit/bcsave.lua
>> +++ b/src/jit/bcsave.lua
>> @@ -275,7 +275,7 @@ typedef struct {
>> o.sect[2].size = fofs(ofs)
>> o.sect[3].type = f32(3) -- .strtab
>> o.sect[3].ofs = fofs(sofs + ofs)
>> - o.sect[3].size = fofs(#symname+1)
>> + o.sect[3].size = fofs(#symname+2)
>> ffi.copy(o.space+ofs+1, symname)
>> ofs = ofs + #symname + 2
>> o.sect[4].type = f32(1) -- .rodata
>> diff --git a/test/tarantool-tests/lj-366-strtab-correct-size.test.lua b/test/tarantool-tests/lj-366-strtab-correct-size.test.lua
>> new file mode 100644
>> index 00000000..d4b51537
>> --- /dev/null
>> +++ b/test/tarantool-tests/lj-366-strtab-correct-size.test.lua
>> @@ -0,0 +1,148 @@
>> +local tap = require('tap')
>> +local test = tap.test('lj-366-strtab-correct-size'):skipcond({
>> + -- Test is ELF-specific and because LuaJIT exports object files in ELF format
> Typo: s/Test/The test/
> Typo: s/, and/and/
Fixed!
>
> Nit: Comment line is wider than 66 symbols.
> Here and below.
Fixed!
--- a/test/tarantool-tests/lj-366-strtab-correct-size.test.lua
+++ b/test/tarantool-tests/lj-366-strtab-correct-size.test.lua
@@ -1,8 +1,9 @@
local tap = require('tap')
local test = tap.test('lj-366-strtab-correct-size'):skipcond({
- -- Test is ELF-specific and because LuaJIT exports object files in
ELF format
- -- for all operating systems except macOS and Windows we skip test on
- -- these OSes. See src/jit/bcsave.lua:bcsave_obj.
+ -- The test is ELF-specific, and because LuaJIT exports object
+ -- files in ELF format for all operating systems except macOS
+ -- and Windows we skip test on these OSes.
+ -- See src/jit/bcsave.lua:bcsave_obj.
['Disabled on Windows'] = jit.os == 'Windows',
['Disabled on macOS'] = jit.os == 'OSX',
})
>> + -- for all operating systems except macOS and Windows we skip test on
>> + -- these OSes. See src/jit/bcsave.lua:bcsave_obj.
>> + ['Disabled on Windows'] = jit.os == 'Windows',
> Side note: I'm not sure, that we shall mention Windows, when we still
> don't support it. Still, this skipcond is necessary, so I'm OK with it.
>
>> + ['Disabled on macOS'] = jit.os == 'OSX',
>> +})
>> +
>> +local ffi = require 'ffi'
>> +
>> +-- Reference numbers for strtab offset and size could be obtained with
>> +-- readelf(1). Note that number system of these number is hexadecimal.
> Typo: s/number/numbers/
Fixed.
@@ -10,7 +11,7 @@ local test =
tap.test('lj-366-strtab-correct-size'):skipcond({
local ffi = require 'ffi'
-- Reference numbers for strtab offset and size could be obtained with
--- readelf(1). Note that number system of these number is hexadecimal.
+-- readelf(1). Note that number system of these numbers are hexadecimal.
--
-- $ luajit -b -n "module_name" -e "print()" xxx.obj
-- $ readelf --section-headers xxx.obj
>> +--
>> +-- $ luajit -b -n "module_name" -e "print()" xxx.obj
>> +-- $ readelf --section-headers xxx.obj
> Minor: May you also provide the linker command with the failure
> mentioned in the issue?
Sure:
$ luajit -b -n "module_name" -e "print()" xxx.obj
$ ld.lld xxx.obj
ld.lld: error: xxx.obj: SHT_STRTAB string table section [index 3]
is non-null terminated
Added to commit message.
> Side note: I suppose that the starting \0 byte [1] was forgotten to
> count. :)
>> +-- There are 6 section headers, starting at offset 0x40:
>> +--
>> +-- Section Headers:
>> +-- [Nr] Name Type Address Offset
>> +-- Size EntSize Flags Link Info Align
>> +-- ...
>> +--
>> +-- [ 3] .strtab STRTAB 0000000000000000 00000223
>> +-- 0000000000000017 0000000000000000 0 0 1
>> +-- ...
>> +
>> +local expected_strtab_size = 23 -- == 0x17
> Minor: Also, comment about this size (length of
> `LJBC_PREFIX..ctx.modname` + 2) is desirable. Maybe we should copy this
> string and calculate the size here as well.
> Feel free to ignore.
Added.
>
>> +local expected_strtab_offset = 547 -- == 0x223
> Why don't declare as 0x17, 0x223?
Because I forgot that Lua allows to use hex numbers.
Fixed.
>
>> +
>> +-- Defined in elf.h.
>> +local sht_symtab = 2
> Minor: Maybe it is better to use UPPER-CASE for constants?
> Feel free to ignore.
Switched to upper-case.
>
>> +
>> +ffi.cdef[[
>> +typedef struct {
>> + uint8_t emagic[4], eclass, eendian, eversion, eosabi, eabiversion, epad[7];
>> + uint16_t type, machine;
>> + uint32_t version;
>> + uint32_t entry, phofs, shofs;
>> + uint32_t flags;
>> + uint16_t ehsize, phentsize, phnum, shentsize, shnum, shstridx;
>> +} ELF32header;
> I suggest to add additional empty line between each declaration for
> better readability.
Added.
>
>> +typedef struct {
>> + uint8_t emagic[4], eclass, eendian, eversion, eosabi, eabiversion, epad[7];
>> + uint16_t type, machine;
>> + uint32_t version;
>> + uint64_t entry, phofs, shofs;
>> + uint32_t flags;
>> + uint16_t ehsize, phentsize, phnum, shentsize, shnum, shstridx;
>> +} ELF64header;
>> +typedef struct {
>> + uint32_t name, type, flags, addr, ofs, size, link, info, align, entsize;
>> +} ELF32sectheader;
>> +typedef struct {
>> + uint32_t name, type;
>> + uint64_t flags, addr, ofs, size;
>> + uint32_t link, info;
>> + uint64_t align, entsize;
>> +} ELF64sectheader;
>> +typedef struct {
>> + uint32_t name, value, size;
>> + uint8_t info, other;
>> + uint16_t sectidx;
>> +} ELF32symbol;
> I have the following declaration in <elf.h>, maybe we should use the the
> same declarations ad litteram? Just to avoid inconsistencies in naming?
> The same for all other structures defined.
>
> | typedef struct
> | {
> | Elf32_Word st_name; /* Symbol name (string tbl index) */
> | Elf32_Addr st_value; /* Symbol value */
> | Elf32_Word st_size; /* Symbol size */
> | unsigned char st_info; /* Symbol type and binding */
> | unsigned char st_other; /* Symbol visibility */
> | Elf32_Section st_shndx; /* Section index */
> | } Elf32_Sym;
>
> Or we may use the same as defined in <src/jit/bcsave.lua> (as you did),
> but the comment about it is desirable.
C definitions were copy-pasted from bcsave.lua, added a comment about it.
>
>> +typedef struct {
>> + uint32_t name;
>> + uint8_t info, other;
>> + uint16_t sectidx;
>> + uint64_t value, size;
>> +} ELF64symbol;
>> +typedef struct {
>> + ELF32header hdr;
>> + ELF32sectheader sect[6];
>> + ELF32symbol sym[2];
>> + uint8_t space[4096];
>> +} ELF32obj;
>> +typedef struct {
>> + ELF64header hdr;
>> + ELF64sectheader sect[6];
>> + ELF64symbol sym[2];
>> + uint8_t space[4096];
>> +} ELF64obj;
>> +]]
>> +
>> +local is64_arch = {
>> + ["x64"] = true,
> Nit: please, use single-quotes for strings, here and below.
Fixed.
>
>> + ["arm64"] = true,
>> + ["arm64be"] = true,
>> + ["ppc"] = false,
>> + ["mips"] = false,
> Why do we need ppc, and mips definitions here?
I would left unsupported arch's here as well, it will simplify a bit work
for those who will port test to these arch's in a future.
>
>> +}
>> +
>> +local is64 = is64_arch[jit.arch] or false
>> +local ELFobj_type = ffi.typeof(is64 and "ELF64obj *" or "ELF32obj *")
>> +local ELFsectheader_type = ffi.typeof(is64 and "ELF64sectheader *" or "ELF32sectheader")
> Should it be `ELF32sectheader *`?
Sure, fixed.
>
>> +
>> +-- Reads a file located in a specified path and returns its content.
>> +local function read_file(path)
>> + local file = assert(io.open(path), 'cannot open an object file')
>> + local content = file:read("*a")
>> + file:close()
>> + return content
>> +end
>> +
>> +-- Parses a buffer in an ELF format and returns an offset and a size of strtab
>> +-- section for a symtab.
>> +local function read_elf_strtab(elf_content)
>> + local elf = ffi.cast(ELFobj_type, elf_content)
>> + local elf_header = elf.hdr
>> + local sec_strtab
>> + for i = 0, elf_header.shnum do
>> + local sec = ffi.cast(ELFsectheader_type, elf.sect[i])
>> + if sec.type == sht_symtab then
> Why don't just to find .strtab instead?
> LuaJIT emits really a small obj file, (it's literally the third section).
strtab is an abstract section, it could be used not for storing symbols
but for other purposes too.
in this test we need a strtab section for symtab.
>
>> + sec_strtab = ffi.cast(ELFsectheader_type, elf.sect[sec.link])
>> + break
>> + end
>> + end
>> + return sec_strtab.size, sec_strtab.ofs
>> +end
>> +
>> +local function lua_bin_path(arg)
> Don't get why do we need this function to get luabin?
> Why just `luacmd` is not enough?
luacmd is not suitable for my test without fixing helper.
The problem was in passing additional arguments to a final luajit cmd.
It looked that easier don't use luacmd than fix it for my purposes.
>
>> + local args_str = require('utils').luacmd(arg)
>> + local args = {}
>> + for a in args_str:gmatch("%S+") do
>> + table.insert(args, a);
>> + end
>> + return args[1]
> Why do we need to parse the whole string if we return only first
> occurence of %S+?
Fixed.
>> +end
>> +
>> +test:plan(5)
>> +
>> +local elf_filename = os.tmpname() .. '.obj'
>> +local lua_path = os.getenv('LUA_PATH')
>> +local lua_bin = lua_bin_path(arg)
>> +local cmd = ('LUA_PATH="%s" %s -b -n "module_name" -e "print()" %s'):format(lua_path, lua_bin, elf_filename)
> Nit: Code line is wider than 80 symbols.
Fixed.
>
>> +local ret = os.execute(cmd)
>> +test:ok(ret == 0, 'create an object file')
> I suggest to use `assert()` for these checks for test correcness.
> `test:*()` functions -- to check some bugs
> `assert()` -- to verify test correctness or validity
> So, we have only 2 test to plan: for size and offset.
With asserts I don't like that it breaks test report.
According to TAP spec there is "bail out" that should handle cases
when something goes wrong and we couldn't continue testing.
In our implementation there is not "bail out" support.
Replaced test:ok() to asserts where checks are related to test
environment and
output now looks as below:
TAP version 13
1..2
ok - check .strtab size
ok - check .strtab offset
>
>> +
>> +local elf_content = read_file(elf_filename)
>> +test:ok(#elf_content ~= 0, 'read an object file')
>> +local strtab_size, strtab_offset = read_elf_strtab(elf_content)
>> +test:ok(strtab_size == expected_strtab_size, 'check .strtab size')
>> +test:ok(strtab_offset == expected_strtab_offset, 'check .strtab offset')
>> +
>> +ret = os.execute(("rm -f %s"):format(elf_filename))
> I suppose, that we may use `os.remove()` here.
Sure, replaced to os.remove().
>
>> +test:ok(ret == 0, 'remove an object file')
>> +
>> +os.exit(test:check() and 0 or 1)
>> --
>> 2.34.1
>>
> [1]: https://refspecs.linuxbase.org/elf/gabi4+/ch4.strtab.html
>
More information about the Tarantool-patches
mailing list