Re: [Tarantool-patches] [PATCH luajit] Avoid out-of-range number of results when compiling select(k, ...).

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

Hi, Sergey

thanks for the patch

couldn't reproduce a problem by provided test.

What compilation options I've used:


1st attempt:

cmake -S . -B build -DLUA_USE_ASSERT=ON -DLUA_USE_APICHECK=ON


2nd attempt:

CMAKE_BUILD_TYPE="Debug"
CMAKE_C_COMPILER="clang"
CMAKE_EXPORT_COMPILE_COMMANDS:BOOL="TRUE"
LUAJIT_ENABLE_COVERAGE:BOOL="FALSE"
LUAJIT_ENABLE_GC64:BOOL="TRUE"
LUAJIT_USE_ASAN:BOOL="FALSE"
LUAJIT_USE_SYSMALLOC:BOOL="FALSE"
LUA_USE_APICHECK:BOOL="TRUE"
   LUA_USE_ASSERT:BOOL="TRUE"


Sergey

On 2/7/24 15:06, Sergey Kaplun wrote:
> 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)