Re: [Tarantool-patches] [PATCH luajit] Fix memory probing allocator to check for valid end address, too.

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

[-- Attachment #1: Type: text/plain, Size: 4568 bytes --]


Hi!
Thanks for the review!
 
> 
>>Hi, Maxim!
>>Thanks for the patch!
>>The patch is LGTM except a few insiginificant nits below.
>>
>>But I'm wondering: can we examine a test case mentioned in the [1]?
>>I.e. create a really long trace, near the upper bound of the 2GB, so
>>its results become meaningless? You may take a look into
>><test/tarantool-tests/gh-4199-gc64-fuse.test.lua> or
>><test/tarantool-tests/gh-6098-fix-side-exit-patching-on-arm64.test.lua>
>>for the inspiration.
>>
>>This is desired to show actual problem, and not changes in some
>>synthetic behaviour.
>As we discussed offline, I’ve added the following comment, branch is force-pushed:
>=============================================
>+-- XXX: This test allocates `cdata` objects, but in real world
>+-- scenarios it can be any object that is allocated with
>+-- LuaJIT's allocator, including, for example, trace, if it
>+-- has been allocated close enough to the memory region
>+-- upper bound and if it is long enough.
>+--
>+-- When this issue occurrs with a trace, it may lead to
>+-- failures in checks that rely on pointers being 32-bit.
>+-- For example, you can see one here: src/lj_asm_x86.h:370.
>+--
>+-- Although it is nice to have a reproducer that shows how
>+-- that issue can affect a non-synthetic execution, it is really
>+-- hard to achieve the described situation with traces because
>+-- allocations are hint-based and there is no robust enough
>+-- way to create a deterministic test for this behavior.
>=============================================
>>
>>On 31.05.23, Maxim Kokryashkin wrote:
>>> From: Mike Pall <mike>
>>>
>>> (cherry-picked from commit 646148e747759f0af3b47f9bd287cedd7e174631)
>>>
>>> Before the patch `mmap_probe` only checked if the allocated chunk
>>> start was within the 2^LJ_ALLOC_MBITS bytes region. However, if the
>>> chunk is big enough, its end can reach outside of that region. This
>>> patch adds the corresponding check, to avoid such situations.
>>>
>>> Maxim Kokryashkin:
>>> * added the description and the test for the problem
>>>
>>> Part of tarantool/tarantool#8516
>>> ---
>>> Branch:  https://github.com/tarantool/luajit/tree/fckxorg/lj-445-fix-memory-probing-allocator
>>> PR:  https://github.com/tarantool/tarantool/pull/8720
>>> LuaJIT issue:  https://github.com/LuaJIT/LuaJIT/issues/445
>>>
>>> src/lj_alloc.c | 3 +-
>>> ...-445-fix-memory-probing-allocator.test.lua | 32 +++++++++++++++++++
>>> 2 files changed, 34 insertions(+), 1 deletion(-)
>>> create mode 100644 test/tarantool-tests/lj-445-fix-memory-probing-allocator.test.lua
>>>
>>> diff --git a/src/lj_alloc.c b/src/lj_alloc.c
>>> index ffcd019b..f7039b5b 100644
>>> --- a/src/lj_alloc.c
>>> +++ b/src/lj_alloc.c
>>
>><snipped>
>>
>>> diff --git a/test/tarantool-tests/lj-445-fix-memory-probing-allocator.test.lua b/test/tarantool-tests/lj-445-fix-memory-probing-allocator.test.lua
>>> new file mode 100644
>>> index 00000000..44763e38
>>> --- /dev/null
>>> +++ b/test/tarantool-tests/lj-445-fix-memory-probing-allocator.test.lua
>>> @@ -0,0 +1,32 @@
>>> +local tap = require('tap')
>>> +local ffi = require('ffi')
>>> +local test = tap.test('lj-445-fix-memory-probing-allocator'):skipcond({
>>> + ['Unlikely to hit beyond the upper bound for GC64'] = ffi.abi('gc64'),
>>> +})
>>> +
>>> +local bit = require('bit')
>>> +local shr = bit.rshift
>>> +local uintptr_t = ffi.typeof('uintptr_t')
>>> +
>>> +-- Due to limitations in the x64 compiler backend, max memory limit is
>>
>>Minor: comment line width is more than 66 symbols.
>Fixed.
>>
>>> +-- two times lower when JIT is not disabled entirely.
>>> +local HAS_JIT = jit.status()
>>> +local LJ_ALLOC_MBITS = HAS_JIT and 31 or 32
>>> +local MAX_GB = HAS_JIT and 2 or 4
>>> +
>>> +test:plan(MAX_GB)
>>> +
>>> +-- Chomp memory in currently allocated GC space.
>>> +collectgarbage('stop')
>>> +
>>> +-- Every allocation must either result in a chunk that fits into the
>>
>>Ditto.
>Fixed.
>>
>>> +-- `MAX_GB`-sized region entirely or return an OOM error.
>>> +for _ = 1, MAX_GB do
>>> + local status, result = pcall(ffi.new, 'char[?]', 1024 * 1024 * 1024)
>>> + if status then
>>> + local upper_bound = ffi.cast(uintptr_t, result) + ffi.sizeof(result)
>>> + test:ok(shr(upper_bound, LJ_ALLOC_MBITS) == 0, 'non-extended address')
>>> + else
>>> + test:ok(result == 'not enough memory', 'OOM encountered')
>>> + end
>>> +end
>>
>>Nit: Mising `os.exit()`.
>>
>>> --
>>> 2.40.1
>>>
>>
>>[1]:  https://github.com/LuaJIT/LuaJIT/issues/445
>>
>>--
>>Best regards,
>>Sergey Kaplun
>--
>Best regards,
>Maxim Kokryashkin
> 

[-- Attachment #2: Type: text/html, Size: 6376 bytes --]