Tarantool development patches archive
 help / color / mirror / Atom feed
From: Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Maxim Kokryashkin <m.kokryashkin@tarantool.org>,
	Sergey Bronnikov <sergeyb@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: [Tarantool-patches] [PATCH luajit] Avoid out-of-range number of results when compiling select(k, ...).
Date: Wed,  7 Feb 2024 15:06:48 +0300	[thread overview]
Message-ID: <20240207120648.12416-1-skaplun@tarantool.org> (raw)

From: Mike Pall <mike>

The interpreter will throw and abort the trace, anyway.

(cherry picked from commit 6ca580155b035fd369f193cdee59391b594a5028)

The `recff_select()` sets the amount of `RecordFFData` structure even
for a negative first argument when trace is not recording (since the
interpreter will throw an error anyway). This leads to excess IR
emission and possible reads of dirty memory.

This patch updates the `rd->nres` only in the case when a trace will be
recorded.

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

Part of tarantool/tarantool#9595
---

Branch: https://github.com/tarantool/luajit/tree/skaplun/fix-ff-select-recording
Tarantool PR: https://github.com/tarantool/tarantool/pull/9659
Related issue: https://github.com/tarantool/tarantool/issues/9595

 src/lj_ffrecord.c                             |  2 +-
 .../fix-ff-select-recording.test.lua          | 44 +++++++++++++++++++
 2 files changed, 45 insertions(+), 1 deletion(-)
 create mode 100644 test/tarantool-tests/fix-ff-select-recording.test.lua

diff --git a/src/lj_ffrecord.c b/src/lj_ffrecord.c
index 99a6b918..cbba9524 100644
--- a/src/lj_ffrecord.c
+++ b/src/lj_ffrecord.c
@@ -317,9 +317,9 @@ static void LJ_FASTCALL recff_select(jit_State *J, RecordFFData *rd)
       ptrdiff_t n = (ptrdiff_t)J->maxslot;
       if (start < 0) start += n;
       else if (start > n) start = n;
-      rd->nres = n - start;
       if (start >= 1) {
 	ptrdiff_t i;
+	rd->nres = n - start;
 	for (i = 0; i < n - start; i++)
 	  J->base[i] = J->base[start+i];
       }  /* else: Interpreter will throw. */
diff --git a/test/tarantool-tests/fix-ff-select-recording.test.lua b/test/tarantool-tests/fix-ff-select-recording.test.lua
new file mode 100644
index 00000000..8e0b4983
--- /dev/null
+++ b/test/tarantool-tests/fix-ff-select-recording.test.lua
@@ -0,0 +1,44 @@
+local tap = require('tap')
+local test = tap.test('fix-ff-select-recording'):skipcond({
+  ['Test requires JIT enabled'] = not jit.status(),
+})
+
+test:plan(2)
+
+jit.opt.start('hotloop=1')
+
+-- XXX: simplify `jit.dump()` output.
+local select = select
+
+local recording = false
+
+-- `start` is the constant on trace, see below.
+local function varg_frame(start, ...)
+  select(start, ...)
+end
+
+local LJ_MAX_JSLOTS = 250
+
+local function varg_frame_wp()
+  -- XXX: Need some constant negative value as the first argument
+  -- of `select()` when recording the trace.
+  -- Also, it should be huge enough to be greater than
+  -- `J->maxslot`. The value on the first iteration is ignored.
+  -- This will fail under ASAN due to a heap buffer overflow.
+  varg_frame(recording and -(LJ_MAX_JSLOTS + 1) or 1)
+end
+
+jit.opt.start('hotloop=1')
+
+-- Make the function hot.
+varg_frame_wp()
+
+-- Try to record `select()` with a negative first argument.
+recording = true
+local res, err = pcall(varg_frame_wp)
+
+test:ok(not res, 'correct status')
+test:like(err, "bad argument #1 to 'select' %(index out of range%)",
+          'correct error message')
+
+test:done(true)
-- 
2.43.0


             reply	other threads:[~2024-02-07 12:10 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-02-07 12:06 Sergey Kaplun via Tarantool-patches [this message]
2024-02-08  9:30 ` Sergey Bronnikov via Tarantool-patches
2024-02-08  9:36   ` Sergey Kaplun via Tarantool-patches
2024-02-08 13:05     ` Sergey Bronnikov via Tarantool-patches
2024-02-08 13:57 ` Sergey Bronnikov via Tarantool-patches
2024-02-09 16:41 ` Maxim Kokryashkin via Tarantool-patches
2024-02-15 13:47 ` Igor Munkin via Tarantool-patches

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20240207120648.12416-1-skaplun@tarantool.org \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=m.kokryashkin@tarantool.org \
    --cc=sergeyb@tarantool.org \
    --cc=skaplun@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH luajit] Avoid out-of-range number of results when compiling select(k, ...).' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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