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

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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.

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.

> +-- 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.

> +-- `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