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

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

[Maxim Kokryashkin via Tarantool-patches]

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
@@ -255,7 +255,8 @@ static void *mmap_probe(size_t size)
   for (retry = 0; retry < LJ_ALLOC_MMAP_PROBE_MAX; retry++) {
     void *p = mmap((void *)hint_addr, size, MMAP_PROT, MMAP_FLAGS_PROBE, -1, 0);
     uintptr_t addr = (uintptr_t)p;
-    if ((addr >> LJ_ALLOC_MBITS) == 0 && addr >= LJ_ALLOC_MMAP_PROBE_LOWER) {
+    if ((addr >> LJ_ALLOC_MBITS) == 0 && addr >= LJ_ALLOC_MMAP_PROBE_LOWER &&
+	((addr + size) >> LJ_ALLOC_MBITS) == 0) {
       /* We got a suitable address. Bump the hint address. */
       hint_addr = addr + size;
       errno = olderr;
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
+-- 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
+-- `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
-- 
2.40.1

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

[Sergey Kaplun via Tarantool-patches]

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

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

[Maxim Kokryashkin via Tarantool-patches]

[-- 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 --]

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

[Sergey Kaplun via Tarantool-patches]

Hi, Maxim!
Thanks for the fixes!
LGTM, just a few typos.

On 07.06.23, Maxim Kokryashkin wrote:
> 
> 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

Typo: s/occurrs/occurs/

> >+-- failures in checks that rely on pointers being 32-bit.

Typo: s/checks/the checks/

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

<snipped>

> >>
> >>--
> >>Best regards,
> >>Sergey Kaplun
> >--
> >Best regards,
> >Maxim Kokryashkin
> > 

-- 
Best regards,
Sergey Kaplun

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

[Maxim Kokryashkin via Tarantool-patches]

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


Hi, Sergey!
Thanks for the comments!
 
> 
>>Hi, Maxim!
>>Thanks for the fixes!
>>LGTM, just a few typos.
>>
>>On 07.06.23, Maxim Kokryashkin wrote:
>>>
>>> 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
>>
>>Typo: s/occurrs/occurs/
>Fixed, thanks! Branch is force-pushed, here is the diff:
>=============================================
>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
>index 9e31ed5e..a228651b 100644
>--- a/test/tarantool-tests/lj-445-fix-memory-probing-allocator.test.lua
>+++ b/test/tarantool-tests/lj-445-fix-memory-probing-allocator.test.lua
>@@ -25,7 +25,7 @@ collectgarbage('stop')
> -- 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
>+-- When this issue occurs 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.
> --
>=============================================
>>
>>> >+-- failures in checks that rely on pointers being 32-bit.
>>
>>Typo: s/checks/the checks/
>That’s not a typo, ignoring. (Double-checked via Quillbot)
>>
>>> >+-- 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.
>>> >=============================================
>>
>><snipped>
>>
>>> >>
>>> >>--
>>> >>Best regards,
>>> >>Sergey Kaplun
>>> >--
>>> >Best regards,
>>> >Maxim Kokryashkin
>>> > 
>>
>>--
>>Best regards,
>>Sergey Kaplun
>--
>Best regards,
>Maxim Kokryashkin

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

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

[Igor Munkin via Tarantool-patches]

Max,

Thanks for the patch! LGTM, after the fixes for the comments left by
Sergey.

On 31.05.23, Maxim Kokryashkin via Tarantool-patches 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
> 

<snipped>

> -- 
> 2.40.1
> 

-- 
Best regards,
IM

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

[Igor Munkin via Tarantool-patches]

Max,

I've checked the patchset into all long-term branches in
tarantool/luajit and bumped a new version in master, release/2.11 and
release/2.10.

On 31.05.23, Maxim Kokryashkin via Tarantool-patches 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
> 

<snipped>

> -- 
> 2.40.1
> 

-- 
Best regards,
IM