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

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

[Sergey Kaplun via Tarantool-patches]

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

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

[Sergey Bronnikov via Tarantool-patches]

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)

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

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!
Thanks for the review!

On 08.02.24, Sergey Bronnikov wrote:
> Hi, Sergey
> 
> thanks for the patch
> 
> couldn't reproduce a problem by provided test.

As you can see in the test comment, the test failed under ASAN with a
heap buffer overflow. Unfortunately, I didn't come up with an idea of
how to observe the misbehaviour (too long IRs for the trace) without
ASAN.

> 
> 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:

<snipped>

> > +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

<snipped>

> > +
> > +test:done(true)

-- 
Best regards,
Sergey Kaplun

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

[Sergey Bronnikov via Tarantool-patches]

Finally reproduced with the following CMake flags:

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="TRUE"
LUAJIT_USE_SYSMALLOC:BOOL="TRUE"
LUA_USE_APICHECK:BOOL="TRUE"
   LUA_USE_ASSERT:BOOL="TRUE"

On 2/8/24 12:36, Sergey Kaplun wrote:
> Hi, Sergey!
> Thanks for the review!
>
> On 08.02.24, Sergey Bronnikov wrote:
>> Hi, Sergey
>>
>> thanks for the patch
>>
>> couldn't reproduce a problem by provided test.
> As you can see in the test comment, the test failed under ASAN with a
> heap buffer overflow. Unfortunately, I didn't come up with an idea of
> how to observe the misbehaviour (too long IRs for the trace) without
> ASAN.
>
>> 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:
> <snipped>
>
>>> +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
> <snipped>
>
>>> +
>>> +test:done(true)

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

[Sergey Bronnikov via Tarantool-patches]

Hi, Sergey

thanks for the patch! LGTM

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)

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

[Maxim Kokryashkin via Tarantool-patches]

Hi, Sergey!
Thanks for the patch!
LGTM

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

[Igor Munkin via Tarantool-patches]

Sergey,

I've checked the patchset into all long-term branches in
tarantool/luajit and bumped a new version in master, release/3.0 and
release/2.11.

On 07.02.24, Sergey Kaplun via Tarantool-patches 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
> 

<snipped>

> -- 
> 2.43.0
> 

-- 
Best regards,
IM