Tarantool development patches archive
 help / color / mirror / Atom feed
* [Tarantool-patches] [PATCH luajit] Correct fix for stack check when recording BC_VARG.
@ 2024-04-04 16:14 Sergey Kaplun via Tarantool-patches
  2024-04-09 11:00 ` Maxim Kokryashkin via Tarantool-patches
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2024-04-04 16:14 UTC (permalink / raw)
  To: Maxim Kokryashkin, Sergey Bronnikov; +Cc: 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


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2024-07-09  8:14 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-04 16:14 [Tarantool-patches] [PATCH luajit] Correct fix for stack check when recording BC_VARG Sergey Kaplun via Tarantool-patches
2024-04-09 11:00 ` Maxim Kokryashkin via Tarantool-patches
2024-04-12 11:35 ` Sergey Bronnikov via Tarantool-patches
2024-04-12 11:39   ` Sergey Kaplun via Tarantool-patches
2024-07-09  8:14 ` Sergey Kaplun via Tarantool-patches

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox