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

[Igor Munkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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