Re: [Tarantool-patches] [PATCH luajit] Correct fix for stack check when recording BC_VARG.

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

Hi, Sergey


thanks for the patch! LGTM wit h a minor comment.



On 04.04.2024 19:14, Sergey Kaplun wrote:
> From: Mike Pall <mike>
>
> Reported by Yichun Zhang.
>
> (cherry picked from commit b2791179ef96d652d00d78d2a8780af690537f6a)
>
> This patch is a follow-up for the commit
'for' -> 'to'
> 5f0a43ace8cfe98c3e8cd313bf809b4405ba395d ("bugfix: fixed assertion
> failure "lj_record.c:92: rec_check_slots: Assertion `nslots <= 250'
> failed" found by stressing our edgelang compiler."), which is identical
> to the commit e0388e6c00866c9ee1c7c9aab8a3ba9e15186b5c ("Fix stack check
> when recording BC_VARG.)" from the upstream. The error is raised too
> late, when buffer overflow of `J->slot` has already occurred and data in
> the `jit_State` structure is corrupted.
>
> This patch moves the corresponding check before using the `J->slot`
> buffer. The `J->maxslot` may overflow the buffer only in cases where the
> amount of the vararg results is unknown. The check is used only in this
> case since the trace recording for the undefined-on-trace varargs is not
> yet implemented for an unknown amount of varargs.
>
> Sergey Kaplun:
> * added the description and the test for the problem
>
> Part of tarantool/tarantool#9595
> ---
>
> Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-noticket-fix-slots-overflow-for-varg-record
> Issue: https://github.com/tarantool/tarantool/issues/9595
>
>   src/lj_record.c                               |  4 +-
>   ...ix-slots-overflow-for-varg-record.test.lua | 99 +++++++++++++++++++
>   2 files changed, 101 insertions(+), 2 deletions(-)
>   create mode 100644 test/tarantool-tests/fix-slots-overflow-for-varg-record.test.lua
>
> diff --git a/src/lj_record.c b/src/lj_record.c
> index 7fa56834..96fe26d8 100644
> --- a/src/lj_record.c
> +++ b/src/lj_record.c
> @@ -1830,6 +1830,8 @@ static void rec_varg(jit_State *J, BCReg dst, ptrdiff_t nresults)
>       } else if (dst >= J->maxslot) {
>         J->maxslot = dst + 1;
>       }
> +    if (J->baseslot + J->maxslot >= LJ_MAX_JSLOTS)
> +      lj_trace_err(J, LJ_TRERR_STACKOV);
>       for (i = 0; i < nresults; i++)
>         J->base[dst+i] = i < nvararg ? getslot(J, i - nvararg - 1 - LJ_FR2) : TREF_NIL;
>     } else {  /* Unknown number of varargs passed to trace. */
> @@ -1913,8 +1915,6 @@ static void rec_varg(jit_State *J, BCReg dst, ptrdiff_t nresults)
>         lj_trace_err_info(J, LJ_TRERR_NYIBC);
>       }
>     }
> -  if (J->baseslot + J->maxslot >= LJ_MAX_JSLOTS)
> -    lj_trace_err(J, LJ_TRERR_STACKOV);
>   }
>   
>   /* -- Record allocations -------------------------------------------------- */
> diff --git a/test/tarantool-tests/fix-slots-overflow-for-varg-record.test.lua b/test/tarantool-tests/fix-slots-overflow-for-varg-record.test.lua
> new file mode 100644
> index 00000000..b09a722d
> --- /dev/null
> +++ b/test/tarantool-tests/fix-slots-overflow-for-varg-record.test.lua
> @@ -0,0 +1,99 @@
> +local tap = require('tap')
> +
> +-- Test file to demonstrate `J->slots` buffer overflow when
> +-- recording the `BC_VARG` bytecode.
> +
> +local test = tap.test('fix-slots-overflow-for-varg-record'):skipcond({
> +  ['Test requires JIT enabled'] = not jit.status(),
> +  ['Disabled on *BSD due to #4819'] = jit.os == 'BSD',
> +})
> +
> +test:plan(1)
> +
> +-- Before the patch, the JIT compiler checked the slots overflow
> +-- for recording of the `BC_VARG` bytecode too late, when the
> +-- overflow of the `slot` buffer had already occurred. Hence, some
> +-- fields of the `jit_State` structure (see <src/lj_jit.h> for
> +-- details) may be overwritten. Unfortunately, neither UBSAN, nor
> +-- ASAN, nor Valgrind can catch such misbehaviour for now. So we
> +-- should observe the results of overwritten fields in the
> +-- structure.
> +--
> +-- In particular, the content of the `params` array with the JIT
> +-- engine settings is corrupted. One of the options to be
> +-- overwritten is the `maxirconst` that overflows, so no trace can
> +-- be compiled. An attempt to compile any trace will fail.
> +--
> +-- The test fails before the commit on the GC64 build and may lead
> +-- to the core dump for the non-GC64 build.
> +
> +local traceinfo = require('jit.util').traceinfo
> +
> +-- XXX: Use a vararg function here to prevent its compilation
> +-- after failing recording for the main trace.
> +-- luacheck: no unused
> +local function empty(...)
> +end
> +
> +local function varg_with_slot_overflow(...)
> +  -- Try to record `BC_VARG`. It should fail due to slots overflow
> +  -- when trying to copy varargs from slots of the incoming
> +  -- parameters to slots of arguments for the call.
> +  empty(...)
> +end
> +_G.varg_with_slot_overflow = varg_with_slot_overflow
> +
> +-- Prevent the compilation of unrelated traces.
> +jit.off()
> +jit.flush()
> +
> +-- XXX: Generate the function with the maximum possible (to
> +-- preserve JIT compilation of the call) arguments given to the
> +-- vararg function to call. Use sizings for the GC64 mode since it
> +-- occupies more slots for the frame.
> +-- Reproduce is still valid for non-GC64 mode since the difference
> +-- is only several additional slots and buffer overflow is still
> +-- observed.
> +local LJ_MAX_JSLOTS = 250
> +-- Amount of slots occupied for a call of a vararg function for
> +-- GC64 mode.
> +local MAX_VARG_FRAME_SZ = 4
> +-- `J->baseslot` at the start of the recording of the call to the
> +-- vararg function for GC64 mode.
> +local MAX_BASESLOT = 8
> +-- The maximum possible number of slots to occupy is
> +-- `LJ_MAX_JSLOTS` without:
> +-- * `J->baseslot` offset at the moment of the call,
> +-- * 2 vararg frames,
> +-- * 2 slots for the functions to call.
> +local chunk = 'varg_with_slot_overflow(1'
> +for i = 2, LJ_MAX_JSLOTS - MAX_BASESLOT - 2 * MAX_VARG_FRAME_SZ - 2 do
> +  chunk = chunk .. (', %d'):format(i)
> +end
> +chunk = chunk .. ')'
> +
> +local caller_of_varg = assert(loadstring(chunk))
> +
> +-- Use an additional frame to simplify the `J->baseslot`
> +-- calculation.
> +local function wrapper()
> +  for _ = 1, 4 do
> +    caller_of_varg()
> +  end
> +end
> +
> +jit.on()
> +jit.opt.start('hotloop=1')
> +
> +wrapper()
> +
> +assert(not traceinfo(1), 'no traces recorded')
> +
> +-- The simplest trace to compile.
> +for _ = 1, 4 do end
> +
> +jit.off()
> +
> +test:ok(traceinfo(1), 'trace is recorded, so structure is not corrupted')
> +
> +test:done(true)