[Tarantool-patches] [PATCH luajit] Ensure correct stack top for OOM error message.

[Tarantool-patches] [PATCH luajit] Ensure correct stack top for OOM error message.

[Sergey Kaplun via Tarantool-patches]

From: Mike Pall <mike>

Reported by Sergey Kaplun.

(cherry picked from commit ca8d3257bb44e42100c7910c47dcdcf01f494187)

`lj_err_mem()` doesn't set up `L->top` for Lua frames, but uses it for
pushing error message on the stack. So, when we call some routine that
does some allocations, it can raise the OOM error (like `lj_tab_dup()`
in `BC_TDUP`) and this error may corrupt stack for unwind in situations
when `L->top` < `L->base`.

This patch restores `L->top` for Lua frames when raise the error via
`lj_err_mem()`.

Sergey Kaplun:
* added the description and the test for the problem

Resolves tarantool/tarantool#3840
Part of tarantool/tarantool#7230
---

Issues:
* https://github.com/LuaJIT/LuaJIT/issues/906
* https://github.com/tarantool/tarantool/issues/7230
* https://github.com/tarantool/tarantool/issues/3840
PR: https://github.com/tarantool/tarantool/pull/7915
Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-906-fix-err-mem

Red LuaJIT CI for MacOS Release builds is a known issue with self-hosted
runners, as Igor has said before.

 src/lj_err.c                                  |  1 +
 .../lj-906-fix-err-mem.test.lua               | 90 +++++++++++++++++++
 2 files changed, 91 insertions(+)
 create mode 100644 test/tarantool-tests/lj-906-fix-err-mem.test.lua

diff --git a/src/lj_err.c b/src/lj_err.c
index c310daf6..70354489 100644
--- a/src/lj_err.c
+++ b/src/lj_err.c
@@ -546,6 +546,7 @@ LJ_NOINLINE void lj_err_mem(lua_State *L)
 {
   if (L->status == LUA_ERRERR+1)  /* Don't touch the stack during lua_open. */
     lj_vm_unwind_c(L->cframe, LUA_ERRMEM);
+  if (curr_funcisL(L)) L->top = curr_topL(L);
   setstrV(L, L->top++, lj_err_str(L, LJ_ERR_ERRMEM));
   lj_err_throw(L, LUA_ERRMEM);
 }
diff --git a/test/tarantool-tests/lj-906-fix-err-mem.test.lua b/test/tarantool-tests/lj-906-fix-err-mem.test.lua
new file mode 100644
index 00000000..a139e1c9
--- /dev/null
+++ b/test/tarantool-tests/lj-906-fix-err-mem.test.lua
@@ -0,0 +1,90 @@
+local tap = require('tap')
+local ffi = require('ffi')
+local table_new = require('table.new')
+
+-- Avoid test to be killed.
+require('utils').skipcond(ffi.abi('gc64'), 'test is not GC64 only')
+
+local test = tap.test('lj-906-fix-err-mem')
+test:plan(1)
+
+local KB = 1024
+local MB = 1024 * KB
+
+-- The maximum available table size, taking into account created
+-- constants for one function.
+local TNEW_SIZE = 511
+
+local gc_anchor = {}
+
+-- This function works until raises the error.
+local function eat_chunks(size)
+  -- Need raise the OOM error inside TDUP, not TNEW, so reserve
+  -- memory for it.
+  -- luacheck: no unused
+  local tnew_anchor = table_new(TNEW_SIZE, 0)
+  while true do
+    table.insert(gc_anchor, ffi.new('char [?]', size))
+  end
+end
+
+-- Function to format inner tab leading to TDUP emitting.
+local function format_inner_tab()
+  local inner_tab = ''
+  local inner_depth = 128
+  -- Repeate table template for TDUP.
+  for _ = 1, inner_depth do
+    inner_tab = inner_tab .. '{a ='
+  end
+  inner_tab = inner_tab .. '{}'
+  for _ = 1, inner_depth do
+    inner_tab = inner_tab .. '},'
+  end
+  return inner_tab
+end
+
+local function format_TDUP_chunk()
+  local big_tab = 'local _ = {\n'
+  local inner_tab = format_inner_tab()
+  for _ = 1, TNEW_SIZE do
+    big_tab = big_tab .. inner_tab .. '\n'
+  end
+  big_tab = big_tab .. '}'
+  return big_tab
+end
+
+local TDUP, err = loadstring(format_TDUP_chunk())
+assert(TDUP, err)
+
+local function frame_before_TDUP()
+  -- Stack slots are needed for coredump in case of misbehaviour.
+  -- luacheck: no unused
+  local frame_slot1, frame_slot2
+  TDUP()
+  return frame_slot1, frame_slot2
+end
+
+collectgarbage()
+collectgarbage('stop')
+
+-- Avoid OOM on traces.
+jit.off()
+
+-- Stack slots are needed for coredump in case of misbehaviour.
+-- luacheck: no unused
+local r, e = pcall(eat_chunks, 8 * MB)
+collectgarbage()
+pcall(eat_chunks, 8 * KB)
+collectgarbage()
+pcall(eat_chunks, 8)
+collectgarbage()
+
+pcall(frame_before_TDUP)
+
+-- Release memory for `tap` functions.
+gc_anchor = nil
+collectgarbage()
+
+test:ok(true, 'correctly throw memory error')
+
+os.exit(test:check() and 0 or 1)
-- 
2.34.1

Re: [Tarantool-patches] [PATCH luajit] Ensure correct stack top for OOM error message.

[Sergey Kaplun via Tarantool-patches]

Hi, again!

On 09.11.22, Sergey Kaplun wrote:
> From: Mike Pall <mike>

<snipped>

> +
> +-- Avoid OOM on traces.
> +jit.off()
> +
> +-- Stack slots are needed for coredump in case of misbehaviour.
> +-- luacheck: no unused
> +local r, e = pcall(eat_chunks, 8 * MB)
> +collectgarbage()
> +pcall(eat_chunks, 8 * KB)
> +collectgarbage()
> +pcall(eat_chunks, 8)
> +collectgarbage()

Forgot that `collectgarbage()` (`full_gc()`) enables GC. Fixed. Branch
is force-pushed.

===================================================================
diff --git a/test/tarantool-tests/lj-906-fix-err-mem.test.lua b/test/tarantool-tests/lj-906-fix-err-mem.test.lua
index a139e1c9..f512e802 100644
--- a/test/tarantool-tests/lj-906-fix-err-mem.test.lua
+++ b/test/tarantool-tests/lj-906-fix-err-mem.test.lua
@@ -74,10 +74,13 @@ jit.off()
 -- luacheck: no unused
 local r, e = pcall(eat_chunks, 8 * MB)
 collectgarbage()
+collectgarbage('stop')
 pcall(eat_chunks, 8 * KB)
 collectgarbage()
+collectgarbage('stop')
 pcall(eat_chunks, 8)
 collectgarbage()
+collectgarbage('stop')
 
 pcall(frame_before_TDUP)
 
===================================================================

> +
> +pcall(frame_before_TDUP)
> +
> +-- Release memory for `tap` functions.
> +gc_anchor = nil
> +collectgarbage()
> +
> +test:ok(true, 'correctly throw memory error')
> +
> +os.exit(test:check() and 0 or 1)
> -- 
> 2.34.1
> 

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit] Ensure correct stack top for OOM error message.

[Igor Munkin via Tarantool-patches]

Sergey,

Thanks for the patch!

On 09.11.22, Sergey Kaplun wrote:
> From: Mike Pall <mike>
> 
> Reported by Sergey Kaplun.
> 
> (cherry picked from commit ca8d3257bb44e42100c7910c47dcdcf01f494187)
> 
> `lj_err_mem()` doesn't set up `L->top` for Lua frames, but uses it for

I believe it "doesn't set `L->top` for the current Lua frame, but...",
doesn't it?

> pushing error message on the stack. So, when we call some routine that

Typo: s/some routine/a routine/ or s/some routine/arbitrary routine/.

> does some allocations, it can raise the OOM error (like `lj_tab_dup()`

Strictly saying it's LUA_ERRMEM, not OOM.

> in `BC_TDUP`) and this error may corrupt stack for unwind in situations
> when `L->top` < `L->base`.
> 
> This patch restores `L->top` for Lua frames when raise the error via

Typo: s/raise the error/the error is raised/.

> `lj_err_mem()`.
> 
> Sergey Kaplun:
> * added the description and the test for the problem
> 
> Resolves tarantool/tarantool#3840
> Part of tarantool/tarantool#7230
> ---
> 
> Issues:
> * https://github.com/LuaJIT/LuaJIT/issues/906
> * https://github.com/tarantool/tarantool/issues/7230
> * https://github.com/tarantool/tarantool/issues/3840
> PR: https://github.com/tarantool/tarantool/pull/7915
> Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-906-fix-err-mem
> 
> Red LuaJIT CI for MacOS Release builds is a known issue with self-hosted
> runners, as Igor has said before.

Everything is fixed at the moment, please try to rebase.

> 
>  src/lj_err.c                                  |  1 +
>  .../lj-906-fix-err-mem.test.lua               | 90 +++++++++++++++++++
>  2 files changed, 91 insertions(+)
>  create mode 100644 test/tarantool-tests/lj-906-fix-err-mem.test.lua
> 

<snipped>

> diff --git a/test/tarantool-tests/lj-906-fix-err-mem.test.lua b/test/tarantool-tests/lj-906-fix-err-mem.test.lua
> new file mode 100644
> index 00000000..a139e1c9
> --- /dev/null
> +++ b/test/tarantool-tests/lj-906-fix-err-mem.test.lua
> @@ -0,0 +1,90 @@
> +local tap = require('tap')
> +local ffi = require('ffi')
> +local table_new = require('table.new')
> +
> +-- Avoid test to be killed.
> +require('utils').skipcond(ffi.abi('gc64'), 'test is not GC64 only')
> +
> +local test = tap.test('lj-906-fix-err-mem')
> +test:plan(1)
> +
> +local KB = 1024
> +local MB = 1024 * KB
> +
> +-- The maximum available table size, taking into account created
> +-- constants for one function.
> +local TNEW_SIZE = 511
> +
> +local gc_anchor = {}
> +
> +-- This function works until raises the error.
> +local function eat_chunks(size)
> +  -- Need raise the OOM error inside TDUP, not TNEW, so reserve
> +  -- memory for it.
> +  -- luacheck: no unused
> +  local tnew_anchor = table_new(TNEW_SIZE, 0)
> +  while true do
> +    table.insert(gc_anchor, ffi.new('char [?]', size))
> +  end
> +end
> +
> +-- Function to format inner tab leading to TDUP emitting.
> +local function format_inner_tab()

Minor: Maybe <make_deep_table(depth)> instead of <format_inner_tab()>?
Just asking, feel free to ignore.

> +  local inner_tab = ''
> +  local inner_depth = 128

Why 128? Please, drop a few words.

> +  -- Repeate table template for TDUP.
> +  for _ = 1, inner_depth do
> +    inner_tab = inner_tab .. '{a ='
> +  end
> +  inner_tab = inner_tab .. '{}'
> +  for _ = 1, inner_depth do
> +    inner_tab = inner_tab .. '},'
> +  end
> +  return inner_tab
> +end
> +

Minor: Same (also think about moving these helpers to utils.*).
Maybe <make_flat_table(size)> instead of <format_TDUP_chunk()>? And
mention TDUP specifics in the comment.

Just asking too, feel free to ignore.

> +local function format_TDUP_chunk()
> +  local big_tab = 'local _ = {\n'
> +  local inner_tab = format_inner_tab()
> +  for _ = 1, TNEW_SIZE do
> +    big_tab = big_tab .. inner_tab .. '\n'
> +  end
> +  big_tab = big_tab .. '}'
> +  return big_tab
> +end
> +
> +local TDUP, err = loadstring(format_TDUP_chunk())
> +assert(TDUP, err)
> +
> +local function frame_before_TDUP()
> +  -- Stack slots are needed for coredump in case of misbehaviour.

Why are they needed? Please drop few more words regarding this.

> +  -- luacheck: no unused
> +  local frame_slot1, frame_slot2
> +  TDUP()
> +  return frame_slot1, frame_slot2
> +end
> +

Minor: I believe you can pack these two lines into something one can
call <janitor>:
| local function janitor()
|   collectgarbage('collect')
|   collectgarbage('stop')
| end

Anyway, you can leave this as is.

> +collectgarbage()
> +collectgarbage('stop')
> +
> +-- Avoid OOM on traces.
> +jit.off()
> +
> +-- Stack slots are needed for coredump in case of misbehaviour.
> +-- luacheck: no unused
> +local r, e = pcall(eat_chunks, 8 * MB)
> +collectgarbage()
> +pcall(eat_chunks, 8 * KB)
> +collectgarbage()
> +pcall(eat_chunks, 8)
> +collectgarbage()
> +
> +pcall(frame_before_TDUP)

Do we need to check the status of the <pcall> above?

> +
> +-- Release memory for `tap` functions.
> +gc_anchor = nil
> +collectgarbage()

Is this step required? I doubt.

> +
> +test:ok(true, 'correctly throw memory error')
> +
> +os.exit(test:check() and 0 or 1)
> -- 
> 2.34.1
> 

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH luajit] Ensure correct stack top for OOM error message.

[Sergey Kaplun via Tarantool-patches]

Hi, Igor!

Thanks for the review!
Welcome back to the ML! :)

Updated remote branch considering your comments.

On 11.11.22, Igor Munkin wrote:
> Sergey,
> 
> Thanks for the patch!
> 
> On 09.11.22, Sergey Kaplun wrote:
> > From: Mike Pall <mike>
> > 
> > Reported by Sergey Kaplun.
> > 
> > (cherry picked from commit ca8d3257bb44e42100c7910c47dcdcf01f494187)
> > 
> > `lj_err_mem()` doesn't set up `L->top` for Lua frames, but uses it for
> 
> I believe it "doesn't set `L->top` for the current Lua frame, but...",
> doesn't it?

Fixed, thanks.

> 
> > pushing error message on the stack. So, when we call some routine that
> 
> Typo: s/some routine/a routine/ or s/some routine/arbitrary routine/.

Fixed.

> 
> > does some allocations, it can raise the OOM error (like `lj_tab_dup()`
> 
> Strictly saying it's LUA_ERRMEM, not OOM.

If you say about naming. Replaced.

> 
> > in `BC_TDUP`) and this error may corrupt stack for unwind in situations
> > when `L->top` < `L->base`.
> > 
> > This patch restores `L->top` for Lua frames when raise the error via
> 
> Typo: s/raise the error/the error is raised/.

Fixed.

> 
> > `lj_err_mem()`.
> > 
> > Sergey Kaplun:
> > * added the description and the test for the problem
> > 
> > Resolves tarantool/tarantool#3840
> > Part of tarantool/tarantool#7230
> > ---
> > 
> > Issues:
> > * https://github.com/LuaJIT/LuaJIT/issues/906
> > * https://github.com/tarantool/tarantool/issues/7230
> > * https://github.com/tarantool/tarantool/issues/3840
> > PR: https://github.com/tarantool/tarantool/pull/7915
> > Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-906-fix-err-mem
> > 
> > Red LuaJIT CI for MacOS Release builds is a known issue with self-hosted
> > runners, as Igor has said before.
> 
> Everything is fixed at the moment, please try to rebase.

Rebased. It works now! Thanks!

> 
> > 
> >  src/lj_err.c                                  |  1 +
> >  .../lj-906-fix-err-mem.test.lua               | 90 +++++++++++++++++++
> >  2 files changed, 91 insertions(+)
> >  create mode 100644 test/tarantool-tests/lj-906-fix-err-mem.test.lua
> > 
> 
> <snipped>
> 
> > diff --git a/test/tarantool-tests/lj-906-fix-err-mem.test.lua b/test/tarantool-tests/lj-906-fix-err-mem.test.lua
> > new file mode 100644
> > index 00000000..a139e1c9
> > --- /dev/null
> > +++ b/test/tarantool-tests/lj-906-fix-err-mem.test.lua
> > @@ -0,0 +1,90 @@
> > +local tap = require('tap')
> > +local ffi = require('ffi')
> > +local table_new = require('table.new')
> > +
> > +-- Avoid test to be killed.
> > +require('utils').skipcond(ffi.abi('gc64'), 'test is not GC64 only')
> > +
> > +local test = tap.test('lj-906-fix-err-mem')
> > +test:plan(1)
> > +
> > +local KB = 1024
> > +local MB = 1024 * KB
> > +
> > +-- The maximum available table size, taking into account created
> > +-- constants for one function.
> > +local TNEW_SIZE = 511
> > +
> > +local gc_anchor = {}
> > +
> > +-- This function works until raises the error.
> > +local function eat_chunks(size)
> > +  -- Need raise the OOM error inside TDUP, not TNEW, so reserve
> > +  -- memory for it.
> > +  -- luacheck: no unused
> > +  local tnew_anchor = table_new(TNEW_SIZE, 0)
> > +  while true do
> > +    table.insert(gc_anchor, ffi.new('char [?]', size))
> > +  end
> > +end
> > +
> > +-- Function to format inner tab leading to TDUP emitting.
> > +local function format_inner_tab()
> 
> Minor: Maybe <make_deep_table(depth)> instead of <format_inner_tab()>?
> Just asking, feel free to ignore.

Fixed.

> 
> > +  local inner_tab = ''
> > +  local inner_depth = 128
> 
> Why 128? Please, drop a few words.

Added.

> 
> > +  -- Repeate table template for TDUP.
> > +  for _ = 1, inner_depth do
> > +    inner_tab = inner_tab .. '{a ='
> > +  end
> > +  inner_tab = inner_tab .. '{}'
> > +  for _ = 1, inner_depth do
> > +    inner_tab = inner_tab .. '},'
> > +  end
> > +  return inner_tab
> > +end
> > +
> 
> Minor: Same (also think about moving these helpers to utils.*).
> Maybe <make_flat_table(size)> instead of <format_TDUP_chunk()>?

IMHO, it's too specific to use this function test-wide, so I just
keep it as is. If we need similar functionality somewhere else, we
can move this function to <utils.lua> later.

So ignore here.

>                                                                 And
> mention TDUP specifics in the comment.

Fixed.

> 
> Just asking too, feel free to ignore.
> 
> > +local function format_TDUP_chunk()
> > +  local big_tab = 'local _ = {\n'
> > +  local inner_tab = format_inner_tab()
> > +  for _ = 1, TNEW_SIZE do
> > +    big_tab = big_tab .. inner_tab .. '\n'
> > +  end
> > +  big_tab = big_tab .. '}'
> > +  return big_tab
> > +end
> > +
> > +local TDUP, err = loadstring(format_TDUP_chunk())
> > +assert(TDUP, err)
> > +
> > +local function frame_before_TDUP()
> > +  -- Stack slots are needed for coredump in case of misbehaviour.
> 
> Why are they needed? Please drop few more words regarding this.

Hmm, I obscure coredump without them too (because of rewriting the
other stack slot). But the frame before TDUP is really needed to rewrite
pcall frame. Fixed, and added the comment.

All minor patches about comments above are done via the following
iterative patch:

===================================================================
diff --git a/test/tarantool-tests/lj-906-fix-err-mem.test.lua b/test/tarantool-tests/lj-906-fix-err-mem.test.lua
index f512e802..0ec1b30e 100644
--- a/test/tarantool-tests/lj-906-fix-err-mem.test.lua
+++ b/test/tarantool-tests/lj-906-fix-err-mem.test.lua
@@ -12,7 +12,7 @@ local KB = 1024
 local MB = 1024 * KB
 
 -- The maximum available table size, taking into account created
--- constants for one function.
+-- constants for one function and nested level.
 local TNEW_SIZE = 511
 
 local gc_anchor = {}
@@ -29,9 +29,8 @@ local function eat_chunks(size)
 end
 
 -- Function to format inner tab leading to TDUP emitting.
-local function format_inner_tab()
+local function make_flat_table(inner_depth)
   local inner_tab = ''
-  local inner_depth = 128
   -- Repeate table template for TDUP.
   for _ = 1, inner_depth do
     inner_tab = inner_tab .. '{a ='
@@ -43,9 +42,14 @@ local function format_inner_tab()
   return inner_tab
 end
 
+-- The `lj_err_mem()` doesn't fix `L->top`, when called from
+-- helper function (like `lj_tab_dup()`) before the patch.
+-- This function creates a chunk with many BC_TDUP inside.
 local function format_TDUP_chunk()
   local big_tab = 'local _ = {\n'
-  local inner_tab = format_inner_tab()
+  -- The maximum available table size, taking into account created
+  -- constants for one function and nested level.
+  local inner_tab = make_flat_table(128)
   for _ = 1, TNEW_SIZE do
     big_tab = big_tab .. inner_tab .. '\n'
   end
@@ -56,12 +60,10 @@ end
 local TDUP, err = loadstring(format_TDUP_chunk())
 assert(TDUP, err)
 
+-- Function to create the additional frame after to be rewrited
+-- one in case of `lj_err_mem()` misbehaviour.
 local function frame_before_TDUP()
-  -- Stack slots are needed for coredump in case of misbehaviour.
-  -- luacheck: no unused
-  local frame_slot1, frame_slot2
   TDUP()
-  return frame_slot1, frame_slot2
 end
 
 collectgarbage()
===================================================================


> 
> > +  -- luacheck: no unused
> > +  local frame_slot1, frame_slot2
> > +  TDUP()
> > +  return frame_slot1, frame_slot2
> > +end
> > +
> 
> Minor: I believe you can pack these two lines into something one can
> call <janitor>:
> | local function janitor()
> |   collectgarbage('collect')
> |   collectgarbage('stop')
> | end
> 
> Anyway, you can leave this as is.

Added, thanks!
See the iterative patch below.
===================================================================
diff --git a/test/tarantool-tests/lj-906-fix-err-mem.test.lua b/test/tarantool-tests/lj-906-fix-err-mem.test.lua
index 519e6bf6..7dbc1614 100644
--- a/test/tarantool-tests/lj-906-fix-err-mem.test.lua
+++ b/test/tarantool-tests/lj-906-fix-err-mem.test.lua
@@ -66,8 +66,12 @@ local function frame_before_TDUP()
   TDUP()
 end
 
-collectgarbage()
-collectgarbage('stop')
+local function janitor()
+  collectgarbage('collect')
+  collectgarbage('stop')
+end
+
+janitor()
 
 -- Avoid OOM on traces.
 jit.off()
@@ -75,14 +79,11 @@ jit.off()
 -- Stack slots are needed for coredump in case of misbehaviour.
 -- luacheck: no unused
 local r, e = pcall(eat_chunks, 8 * MB)
-collectgarbage()
-collectgarbage('stop')
+janitor()
 pcall(eat_chunks, 8 * KB)
-collectgarbage()
-collectgarbage('stop')
+janitor()
 pcall(eat_chunks, 8)
-collectgarbage()
-collectgarbage('stop')
+janitor()
 
 pcall(frame_before_TDUP)
===================================================================

 
> 
> > +collectgarbage()
> > +collectgarbage('stop')
> > +
> > +-- Avoid OOM on traces.
> > +jit.off()
> > +
> > +-- Stack slots are needed for coredump in case of misbehaviour.
> > +-- luacheck: no unused
> > +local r, e = pcall(eat_chunks, 8 * MB)
> > +collectgarbage()
> > +pcall(eat_chunks, 8 * KB)
> > +collectgarbage()
> > +pcall(eat_chunks, 8)
> > +collectgarbage()
> > +
> > +pcall(frame_before_TDUP)
> 
> Do we need to check the status of the <pcall> above?

I suppose no. As we disscussed offline the test may be flaky (false
positive, when we can alloc all tables) due to memory allocations.
Also, this part is very fragile, and we don't want to fix test later by
removing this check the status of the `pcall()`.

> 
> > +
> > +-- Release memory for `tap` functions.
> > +gc_anchor = nil
> > +collectgarbage()
> 
> Is this step required? I doubt.

Yes it is. Without it we may get the OOM error inside `test:ok()` or
`test:check()` functions (I faced it on my local machine, when rewrite
this test).

> 
> > +
> > +test:ok(true, 'correctly throw memory error')
> > +
> > +os.exit(test:check() and 0 or 1)
> > -- 
> > 2.34.1
> > 
> 
> -- 
> Best regards,
> IM

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit] Ensure correct stack top for OOM error message.

[Sergey Kaplun via Tarantool-patches]

Hi, again!

Added minor fixes.

On 11.11.22, Sergey Kaplun via Tarantool-patches wrote:

<snipped>

> ===================================================================
> diff --git a/test/tarantool-tests/lj-906-fix-err-mem.test.lua b/test/tarantool-tests/lj-906-fix-err-mem.test.lua
> index f512e802..0ec1b30e 100644
> --- a/test/tarantool-tests/lj-906-fix-err-mem.test.lua
> +++ b/test/tarantool-tests/lj-906-fix-err-mem.test.lua
> @@ -12,7 +12,7 @@ local KB = 1024
>  local MB = 1024 * KB
>  
>  -- The maximum available table size, taking into account created
> --- constants for one function.
> +-- constants for one function and nested level.
>  local TNEW_SIZE = 511
>  
>  local gc_anchor = {}
> @@ -29,9 +29,8 @@ local function eat_chunks(size)
>  end
>  
>  -- Function to format inner tab leading to TDUP emitting.
> -local function format_inner_tab()
> +local function make_flat_table(inner_depth)

Should be deep_table, obviously.

===================================================================
diff --git a/test/tarantool-tests/lj-906-fix-err-mem.test.lua b/test/tarantool-tests/lj-906-fix-err-mem.test.lua
index 7dbc1614..69f74893 100644
--- a/test/tarantool-tests/lj-906-fix-err-mem.test.lua
+++ b/test/tarantool-tests/lj-906-fix-err-mem.test.lua
@@ -29,7 +29,7 @@ local function eat_chunks(size)
 end
 
 -- Function to format inner tab leading to TDUP emitting.
-local function make_flat_table(inner_depth)
+local function make_deep_table(inner_depth)
   local inner_tab = ''
   -- Repeate table template for TDUP.
   for _ = 1, inner_depth do
@@ -49,7 +49,7 @@ local function format_TDUP_chunk()
   local big_tab = 'local _ = {\n'
   -- The maximum available table size, taking into account created
   -- constants for one function and nested level.
-  local inner_tab = make_flat_table(128)
+  local inner_tab = make_deep_table(128)
   for _ = 1, TNEW_SIZE do
     big_tab = big_tab .. inner_tab .. '\n'
   end
===================================================================

>    local inner_tab = ''
> -  local inner_depth = 128
>    -- Repeate table template for TDUP.
>    for _ = 1, inner_depth do
>      inner_tab = inner_tab .. '{a ='
> @@ -43,9 +42,14 @@ local function format_inner_tab()
>    return inner_tab
>  end
>  
> +-- The `lj_err_mem()` doesn't fix `L->top`, when called from
> +-- helper function (like `lj_tab_dup()`) before the patch.
> +-- This function creates a chunk with many BC_TDUP inside.
>  local function format_TDUP_chunk()
>    local big_tab = 'local _ = {\n'
> -  local inner_tab = format_inner_tab()
> +  -- The maximum available table size, taking into account created
> +  -- constants for one function and nested level.
> +  local inner_tab = make_flat_table(128)
>    for _ = 1, TNEW_SIZE do
>      big_tab = big_tab .. inner_tab .. '\n'
>    end
> @@ -56,12 +60,10 @@ end
>  local TDUP, err = loadstring(format_TDUP_chunk())
>  assert(TDUP, err)
>  
> +-- Function to create the additional frame after to be rewrited
> +-- one in case of `lj_err_mem()` misbehaviour.

Also, minor comments fixes and renaming.
===================================================================
diff --git a/test/tarantool-tests/lj-906-fix-err-mem.test.lua b/test/tarantool-tests/lj-906-fix-err-mem.test.lua
index 69f74893..f4ec6620 100644
--- a/test/tarantool-tests/lj-906-fix-err-mem.test.lua
+++ b/test/tarantool-tests/lj-906-fix-err-mem.test.lua
@@ -45,7 +45,7 @@ end
 -- The `lj_err_mem()` doesn't fix `L->top`, when called from
 -- helper function (like `lj_tab_dup()`) before the patch.
 -- This function creates a chunk with many BC_TDUP inside.
-local function format_TDUP_chunk()
+local function make_TDUP_chunk()
   local big_tab = 'local _ = {\n'
   -- The maximum available table size, taking into account created
   -- constants for one function and nested level.
@@ -57,11 +57,11 @@ local function format_TDUP_chunk()
   return big_tab
 end
 
-local TDUP, err = loadstring(format_TDUP_chunk())
+local TDUP, err = loadstring(make_TDUP_chunk())
 assert(TDUP, err)
 
--- Function to create the additional frame after to be rewrited
--- one in case of `lj_err_mem()` misbehaviour.
+-- Function  to create the additional frame to be rewritten later
+-- in case of `lj_err_mem()` misbehaviour.
 local function frame_before_TDUP()
   TDUP()
 end
===================================================================

Branch is force-pushed.
>  local function frame_before_TDUP()
> -  -- Stack slots are needed for coredump in case of misbehaviour.
> -  -- luacheck: no unused
> -  local frame_slot1, frame_slot2
>    TDUP()
> -  return frame_slot1, frame_slot2
>  end
>  
>  collectgarbage()
> ===================================================================
> 
> 
> > 
> > > +  -- luacheck: no unused
> > > +  local frame_slot1, frame_slot2
> > > +  TDUP()
> > > +  return frame_slot1, frame_slot2
> > > +end
> > > +
> > 
> > Minor: I believe you can pack these two lines into something one can
> > call <janitor>:
> > | local function janitor()
> > |   collectgarbage('collect')
> > |   collectgarbage('stop')
> > | end
> > 
> > Anyway, you can leave this as is.
> 
> Added, thanks!
> See the iterative patch below.
> ===================================================================
> diff --git a/test/tarantool-tests/lj-906-fix-err-mem.test.lua b/test/tarantool-tests/lj-906-fix-err-mem.test.lua
> index 519e6bf6..7dbc1614 100644
> --- a/test/tarantool-tests/lj-906-fix-err-mem.test.lua
> +++ b/test/tarantool-tests/lj-906-fix-err-mem.test.lua
> @@ -66,8 +66,12 @@ local function frame_before_TDUP()
>    TDUP()
>  end
>  
> -collectgarbage()
> -collectgarbage('stop')
> +local function janitor()
> +  collectgarbage('collect')
> +  collectgarbage('stop')
> +end
> +
> +janitor()
>  
>  -- Avoid OOM on traces.
>  jit.off()
> @@ -75,14 +79,11 @@ jit.off()
>  -- Stack slots are needed for coredump in case of misbehaviour.
>  -- luacheck: no unused
>  local r, e = pcall(eat_chunks, 8 * MB)
> -collectgarbage()
> -collectgarbage('stop')
> +janitor()
>  pcall(eat_chunks, 8 * KB)
> -collectgarbage()
> -collectgarbage('stop')
> +janitor()
>  pcall(eat_chunks, 8)
> -collectgarbage()
> -collectgarbage('stop')
> +janitor()
>  
>  pcall(frame_before_TDUP)
> ===================================================================
> 
>  
> > 
> > > +collectgarbage()
> > > +collectgarbage('stop')
> > > +
> > > +-- Avoid OOM on traces.
> > > +jit.off()
> > > +
> > > +-- Stack slots are needed for coredump in case of misbehaviour.
> > > +-- luacheck: no unused
> > > +local r, e = pcall(eat_chunks, 8 * MB)
> > > +collectgarbage()
> > > +pcall(eat_chunks, 8 * KB)
> > > +collectgarbage()
> > > +pcall(eat_chunks, 8)
> > > +collectgarbage()
> > > +
> > > +pcall(frame_before_TDUP)
> > 
> > Do we need to check the status of the <pcall> above?
> 
> I suppose no. As we disscussed offline the test may be flaky (false
> positive, when we can alloc all tables) due to memory allocations.
> Also, this part is very fragile, and we don't want to fix test later by
> removing this check the status of the `pcall()`.
> 
> > 
> > > +
> > > +-- Release memory for `tap` functions.
> > > +gc_anchor = nil
> > > +collectgarbage()
> > 
> > Is this step required? I doubt.
> 
> Yes it is. Without it we may get the OOM error inside `test:ok()` or
> `test:check()` functions (I faced it on my local machine, when rewrite
> this test).
> 
> > 
> > > +
> > > +test:ok(true, 'correctly throw memory error')
> > > +
> > > +os.exit(test:check() and 0 or 1)
> > > -- 
> > > 2.34.1
> > > 
> > 
> > -- 
> > Best regards,
> > IM
> 
> -- 
> Best regards,
> Sergey Kaplun

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit] Ensure correct stack top for OOM error message.

[Maxim Kokryashkin via Tarantool-patches]

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


Hi, Sergey!
Thanks for the patch!
LGTM
> 
>>Hi, again!
>>
>>Added minor fixes.
>>
>>On 11.11.22, Sergey Kaplun via Tarantool-patches wrote:
>>
>><snipped>
>>
>>> ===================================================================
>>> diff --git a/test/tarantool-tests/lj-906-fix-err-mem.test.lua b/test/tarantool-tests/lj-906-fix-err-mem.test.lua
>>> index f512e802..0ec1b30e 100644
>>> --- a/test/tarantool-tests/lj-906-fix-err-mem.test.lua
>>> +++ b/test/tarantool-tests/lj-906-fix-err-mem.test.lua
>>> @@ -12,7 +12,7 @@ local KB = 1024
>>> local MB = 1024 * KB
>>>
>>> -- The maximum available table size, taking into account created
>>> --- constants for one function.
>>> +-- constants for one function and nested level.
>>> local TNEW_SIZE = 511
>>>
>>> local gc_anchor = {}
>>> @@ -29,9 +29,8 @@ local function eat_chunks(size)
>>> end
>>>
>>> -- Function to format inner tab leading to TDUP emitting.
>>> -local function format_inner_tab()
>>> +local function make_flat_table(inner_depth)
>>
>>Should be deep_table, obviously.
>>
>>===================================================================
>>diff --git a/test/tarantool-tests/lj-906-fix-err-mem.test.lua b/test/tarantool-tests/lj-906-fix-err-mem.test.lua
>>index 7dbc1614..69f74893 100644
>>--- a/test/tarantool-tests/lj-906-fix-err-mem.test.lua
>>+++ b/test/tarantool-tests/lj-906-fix-err-mem.test.lua
>>@@ -29,7 +29,7 @@ local function eat_chunks(size)
>> end
>> 
>> -- Function to format inner tab leading to TDUP emitting.
>>-local function make_flat_table(inner_depth)
>>+local function make_deep_table(inner_depth)
>>   local inner_tab = ''
>>   -- Repeate table template for TDUP.
>>   for _ = 1, inner_depth do
>>@@ -49,7 +49,7 @@ local function format_TDUP_chunk()
>>   local big_tab = 'local _ = {\n'
>>   -- The maximum available table size, taking into account created
>>   -- constants for one function and nested level.
>>- local inner_tab = make_flat_table(128)
>>+ local inner_tab = make_deep_table(128)
>>   for _ = 1, TNEW_SIZE do
>>     big_tab = big_tab .. inner_tab .. '\n'
>>   end
>>===================================================================
>>
>>> local inner_tab = ''
>>> - local inner_depth = 128
>>> -- Repeate table template for TDUP.
>>> for _ = 1, inner_depth do
>>> inner_tab = inner_tab .. '{a ='
>>> @@ -43,9 +42,14 @@ local function format_inner_tab()
>>> return inner_tab
>>> end
>>>
>>> +-- The `lj_err_mem()` doesn't fix `L->top`, when called from
>>> +-- helper function (like `lj_tab_dup()`) before the patch.
>>> +-- This function creates a chunk with many BC_TDUP inside.
>>> local function format_TDUP_chunk()
>>> local big_tab = 'local _ = {\n'
>>> - local inner_tab = format_inner_tab()
>>> + -- The maximum available table size, taking into account created
>>> + -- constants for one function and nested level.
>>> + local inner_tab = make_flat_table(128)
>>> for _ = 1, TNEW_SIZE do
>>> big_tab = big_tab .. inner_tab .. '\n'
>>> end
>>> @@ -56,12 +60,10 @@ end
>>> local TDUP, err = loadstring(format_TDUP_chunk())
>>> assert(TDUP, err)
>>>
>>> +-- Function to create the additional frame after to be rewrited
>>> +-- one in case of `lj_err_mem()` misbehaviour.
>>
>>Also, minor comments fixes and renaming.
>>===================================================================
>>diff --git a/test/tarantool-tests/lj-906-fix-err-mem.test.lua b/test/tarantool-tests/lj-906-fix-err-mem.test.lua
>>index 69f74893..f4ec6620 100644
>>--- a/test/tarantool-tests/lj-906-fix-err-mem.test.lua
>>+++ b/test/tarantool-tests/lj-906-fix-err-mem.test.lua
>>@@ -45,7 +45,7 @@ end
>> -- The `lj_err_mem()` doesn't fix `L->top`, when called from
>> -- helper function (like `lj_tab_dup()`) before the patch.
>> -- This function creates a chunk with many BC_TDUP inside.
>>-local function format_TDUP_chunk()
>>+local function make_TDUP_chunk()
>>   local big_tab = 'local _ = {\n'
>>   -- The maximum available table size, taking into account created
>>   -- constants for one function and nested level.
>>@@ -57,11 +57,11 @@ local function format_TDUP_chunk()
>>   return big_tab
>> end
>> 
>>-local TDUP, err = loadstring(format_TDUP_chunk())
>>+local TDUP, err = loadstring(make_TDUP_chunk())
>> assert(TDUP, err)
>> 
>>--- Function to create the additional frame after to be rewrited
>>--- one in case of `lj_err_mem()` misbehaviour.
>>+-- Function to create the additional frame to be rewritten later
>>+-- in case of `lj_err_mem()` misbehaviour.
>> local function frame_before_TDUP()
>>   TDUP()
>> end
>>===================================================================
>>
>>Branch is force-pushed.
>>> local function frame_before_TDUP()
>>> - -- Stack slots are needed for coredump in case of misbehaviour.
>>> - -- luacheck: no unused
>>> - local frame_slot1, frame_slot2
>>> TDUP()
>>> - return frame_slot1, frame_slot2
>>> end
>>>
>>> collectgarbage()
>>> ===================================================================
>>>
>>>
>>> >
>>> > > + -- luacheck: no unused
>>> > > + local frame_slot1, frame_slot2
>>> > > + TDUP()
>>> > > + return frame_slot1, frame_slot2
>>> > > +end
>>> > > +
>>> >
>>> > Minor: I believe you can pack these two lines into something one can
>>> > call <janitor>:
>>> > | local function janitor()
>>> > | collectgarbage('collect')
>>> > | collectgarbage('stop')
>>> > | end
>>> >
>>> > Anyway, you can leave this as is.
>>>
>>> Added, thanks!
>>> See the iterative patch below.
>>> ===================================================================
>>> diff --git a/test/tarantool-tests/lj-906-fix-err-mem.test.lua b/test/tarantool-tests/lj-906-fix-err-mem.test.lua
>>> index 519e6bf6..7dbc1614 100644
>>> --- a/test/tarantool-tests/lj-906-fix-err-mem.test.lua
>>> +++ b/test/tarantool-tests/lj-906-fix-err-mem.test.lua
>>> @@ -66,8 +66,12 @@ local function frame_before_TDUP()
>>> TDUP()
>>> end
>>>
>>> -collectgarbage()
>>> -collectgarbage('stop')
>>> +local function janitor()
>>> + collectgarbage('collect')
>>> + collectgarbage('stop')
>>> +end
>>> +
>>> +janitor()
>>>
>>> -- Avoid OOM on traces.
>>> jit.off()
>>> @@ -75,14 +79,11 @@ jit.off()
>>> -- Stack slots are needed for coredump in case of misbehaviour.
>>> -- luacheck: no unused
>>> local r, e = pcall(eat_chunks, 8 * MB)
>>> -collectgarbage()
>>> -collectgarbage('stop')
>>> +janitor()
>>> pcall(eat_chunks, 8 * KB)
>>> -collectgarbage()
>>> -collectgarbage('stop')
>>> +janitor()
>>> pcall(eat_chunks, 8)
>>> -collectgarbage()
>>> -collectgarbage('stop')
>>> +janitor()
>>>
>>> pcall(frame_before_TDUP)
>>> ===================================================================
>>>
>>>
>>> >
>>> > > +collectgarbage()
>>> > > +collectgarbage('stop')
>>> > > +
>>> > > +-- Avoid OOM on traces.
>>> > > +jit.off()
>>> > > +
>>> > > +-- Stack slots are needed for coredump in case of misbehaviour.
>>> > > +-- luacheck: no unused
>>> > > +local r, e = pcall(eat_chunks, 8 * MB)
>>> > > +collectgarbage()
>>> > > +pcall(eat_chunks, 8 * KB)
>>> > > +collectgarbage()
>>> > > +pcall(eat_chunks, 8)
>>> > > +collectgarbage()
>>> > > +
>>> > > +pcall(frame_before_TDUP)
>>> >
>>> > Do we need to check the status of the <pcall> above?
>>>
>>> I suppose no. As we disscussed offline the test may be flaky (false
>>> positive, when we can alloc all tables) due to memory allocations.
>>> Also, this part is very fragile, and we don't want to fix test later by
>>> removing this check the status of the `pcall()`.
>>>
>>> >
>>> > > +
>>> > > +-- Release memory for `tap` functions.
>>> > > +gc_anchor = nil
>>> > > +collectgarbage()
>>> >
>>> > Is this step required? I doubt.
>>>
>>> Yes it is. Without it we may get the OOM error inside `test:ok()` or
>>> `test:check()` functions (I faced it on my local machine, when rewrite
>>> this test).
>>>
>>> >
>>> > > +
>>> > > +test:ok(true, 'correctly throw memory error')
>>> > > +
>>> > > +os.exit(test:check() and 0 or 1)
>>> > > --
>>> > > 2.34.1
>>> > >
>>> >
>>> > --
>>> > Best regards,
>>> > IM
>>>
>>> --
>>> Best regards,
>>> Sergey Kaplun
>>
>>--
>>Best regards,
>>Sergey Kaplun
> 
Best regards,
Maxim Kokryashkin

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

Re: [Tarantool-patches] [PATCH luajit] Ensure correct stack top for OOM error message.

[Igor Munkin via Tarantool-patches]

Sergey,

Thanks for the fixes! LGTM.

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH luajit] Ensure correct stack top for OOM error message.

[Igor Munkin via Tarantool-patches]

Sergey,

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

-- 
Best regards,
IM