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

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

[Sergey Kaplun via Tarantool-patches]

From: Mike Pall <mike>

Reported by Yichun Zhang.

(cherry picked from commit b2791179ef96d652d00d78d2a8780af690537f6a)

This patch is a follow-up for the commit
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)
-- 
2.44.0

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

[Maxim Kokryashkin via Tarantool-patches]

Hi, Sergey!
Thanks for the patch!
LGTM

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

[Sergey Bronnikov via Tarantool-patches]

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)

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

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!
Thanks for the review!
Fixed your comment and rebased the branch to the current master.
Also, fixed the ticket number to part 8.

On 12.04.24, Sergey Bronnikov wrote:
> 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'

Fixed, thanks!

> > 5f0a43ace8cfe98c3e8cd313bf809b4405ba395d ("bugfix: fixed assertion

<snipped>

-- 
Best regards,
Sergey Kaplun

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

[Sergey Kaplun via Tarantool-patches]

I've checked the patchset into all long-term branches in
tarantool/luajit and bumped a new version in master [1], release/3.1 [2]
and release/2.11 [3].

[1]: https://github.com/tarantool/tarantool/pull/10200
[2]: https://github.com/tarantool/tarantool/pull/10201
[3]: https://github.com/tarantool/tarantool/pull/10202

-- 
Best regards,
Sergey Kaplun