[Tarantool-patches] [PATCH luajit] Fix predict_next() in parser (for real now).

[Tarantool-patches] [PATCH luajit] Fix predict_next() in parser (for real now).

[Sergey Kaplun via Tarantool-patches]

From: Mike Pall <mike>

Reported by Sergey Kaplun.

(cherry picked from commit f602f0154b644211283cfeea92a570ca38f71947)

Before the patch `predict_next()` uses the pc allocation limit
(`fs->bclim`) instead of the real limit of the defined bytecodes
(`fs->pc`). This leads to the use of undefined value and possible
crash. This patch fixes the check.

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

Part of tarantool/tarantool#10709
---

Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-1226-fix-predict-next
Related issues:
* https://github.com/LuaJIT/LuaJIT/issues/1226
* https://github.com/tarantool/tarantool/issues/10709

 src/lj_parse.c                                |  6 ++--
 .../lj-1226-fix-predict-next.test.lua         | 31 +++++++++++++++++++
 2 files changed, 33 insertions(+), 4 deletions(-)
 create mode 100644 test/tarantool-tests/lj-1226-fix-predict-next.test.lua

diff --git a/src/lj_parse.c b/src/lj_parse.c
index 9b45b103..ec85ac9b 100644
--- a/src/lj_parse.c
+++ b/src/lj_parse.c
@@ -2527,11 +2527,9 @@ static void parse_for_num(LexState *ls, GCstr *varname, BCLine line)
 */
 static int predict_next(LexState *ls, FuncState *fs, BCPos pc)
 {
-  BCIns ins;
+  BCIns ins = fs->bcbase[pc].ins;
   GCstr *name;
   cTValue *o;
-  if (pc >= fs->bclim) return 0;
-  ins = fs->bcbase[pc].ins;
   switch (bc_op(ins)) {
   case BC_MOV:
     if (bc_d(ins) >= fs->nactvar) return 0;
@@ -2580,7 +2578,7 @@ static void parse_for_iter(LexState *ls, GCstr *indexname)
   assign_adjust(ls, 3, expr_list(ls, &e), &e);
   /* The iterator needs another 3 [4] slots (func [pc] | state ctl). */
   bcreg_bump(fs, 3+LJ_FR2);
-  isnext = (nvars <= 5 && predict_next(ls, fs, exprpc));
+  isnext = (nvars <= 5 && fs->pc > exprpc && predict_next(ls, fs, exprpc));
   var_add(ls, 3);  /* Hidden control variables. */
   lex_check(ls, TK_do);
   loop = bcemit_AJ(fs, isnext ? BC_ISNEXT : BC_JMP, base, NO_JMP);
diff --git a/test/tarantool-tests/lj-1226-fix-predict-next.test.lua b/test/tarantool-tests/lj-1226-fix-predict-next.test.lua
new file mode 100644
index 00000000..3cd2c8f5
--- /dev/null
+++ b/test/tarantool-tests/lj-1226-fix-predict-next.test.lua
@@ -0,0 +1,31 @@
+local tap = require('tap')
+local test = tap.test('lj-1226-fix-predict-next')
+
+test:plan(3)
+
+-- The resulting bytecode is the following:
+--
+-- 0001    KNIL     0   3
+-- 0002    JMP      4 => 0003
+-- 0003 => ITERC    4   2   3
+-- 0004    ITERL    4 => 0003
+--
+-- The parsing of the `for` iterator uses the incorrect check for
+-- `fs->bclim`, which allows the usage of an uninitialized value,
+-- so the test fails under Valgrind.
+local res_f = loadstring([[
+-- This local variable is necessary, because it emits `KPRI`
+-- bytecode, with which the next `KPRI` bytecode will be merged.
+local _
+for _ in nil do end
+]])
+
+test:ok(res_f, 'chunk loaded successfully')
+
+local res, err = pcall(res_f)
+
+-- Check consistency with PUC Rio Lua 5.1 behaviour.
+test:ok(not res, 'loaded function not executed')
+test:like(err, 'attempt to call a nil value', 'correct error message')
+
+test:done(true)
-- 
2.47.1

Re: [Tarantool-patches] [PATCH luajit] Fix predict_next() in parser (for real now).

[Sergey Bronnikov via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 3572 bytes --]

Hi, Sergey,

thanks for the patch!

test is passed with reverted patch.

With original reproducer luajit segfaults.

CMake options:  cmake -S . -B build -DCMAKE_BUILD_TYPE=Debug 
-DLUA_USE_APICHECK=ON -DLUA_USE_ASSERT=ON


Sergey

On 09.01.2025 18:01, Sergey Kaplun wrote:
> From: Mike Pall <mike>
>
> Reported by Sergey Kaplun.
>
> (cherry picked from commit f602f0154b644211283cfeea92a570ca38f71947)
>
> Before the patch `predict_next()` uses the pc allocation limit
> (`fs->bclim`) instead of the real limit of the defined bytecodes
> (`fs->pc`). This leads to the use of undefined value and possible
> crash. This patch fixes the check.
>
> Sergey Kaplun:
> * added the description and the test for the problem
>
> Part of tarantool/tarantool#10709
> ---
>
> Branch:https://github.com/tarantool/luajit/tree/skaplun/lj-1226-fix-predict-next
> Related issues:
> *https://github.com/LuaJIT/LuaJIT/issues/1226
> *https://github.com/tarantool/tarantool/issues/10709
>
>   src/lj_parse.c                                |  6 ++--
>   .../lj-1226-fix-predict-next.test.lua         | 31 +++++++++++++++++++
>   2 files changed, 33 insertions(+), 4 deletions(-)
>   create mode 100644 test/tarantool-tests/lj-1226-fix-predict-next.test.lua
>
> diff --git a/src/lj_parse.c b/src/lj_parse.c
> index 9b45b103..ec85ac9b 100644
> --- a/src/lj_parse.c
> +++ b/src/lj_parse.c
> @@ -2527,11 +2527,9 @@ static void parse_for_num(LexState *ls, GCstr *varname, BCLine line)
>   */
>   static int predict_next(LexState *ls, FuncState *fs, BCPos pc)
>   {
> -  BCIns ins;
> +  BCIns ins = fs->bcbase[pc].ins;
>     GCstr *name;
>     cTValue *o;
> -  if (pc >= fs->bclim) return 0;
> -  ins = fs->bcbase[pc].ins;
>     switch (bc_op(ins)) {
>     case BC_MOV:
>       if (bc_d(ins) >= fs->nactvar) return 0;
> @@ -2580,7 +2578,7 @@ static void parse_for_iter(LexState *ls, GCstr *indexname)
>     assign_adjust(ls, 3, expr_list(ls, &e), &e);
>     /* The iterator needs another 3 [4] slots (func [pc] | state ctl). */
>     bcreg_bump(fs, 3+LJ_FR2);
> -  isnext = (nvars <= 5 && predict_next(ls, fs, exprpc));
> +  isnext = (nvars <= 5 && fs->pc > exprpc && predict_next(ls, fs, exprpc));
>     var_add(ls, 3);  /* Hidden control variables. */
>     lex_check(ls, TK_do);
>     loop = bcemit_AJ(fs, isnext ? BC_ISNEXT : BC_JMP, base, NO_JMP);
> diff --git a/test/tarantool-tests/lj-1226-fix-predict-next.test.lua b/test/tarantool-tests/lj-1226-fix-predict-next.test.lua
> new file mode 100644
> index 00000000..3cd2c8f5
> --- /dev/null
> +++ b/test/tarantool-tests/lj-1226-fix-predict-next.test.lua
> @@ -0,0 +1,31 @@
> +local tap = require('tap')
> +local test = tap.test('lj-1226-fix-predict-next')
> +
> +test:plan(3)
> +
> +-- The resulting bytecode is the following:
> +--
> +-- 0001    KNIL     0   3
> +-- 0002    JMP      4 => 0003
> +-- 0003 => ITERC    4   2   3
> +-- 0004    ITERL    4 => 0003
> +--
> +-- The parsing of the `for` iterator uses the incorrect check for
> +-- `fs->bclim`, which allows the usage of an uninitialized value,
> +-- so the test fails under Valgrind.
> +local res_f = loadstring([[
> +-- This local variable is necessary, because it emits `KPRI`
> +-- bytecode, with which the next `KPRI` bytecode will be merged.
> +local _
> +for _ in nil do end
> +]])
> +
> +test:ok(res_f, 'chunk loaded successfully')
> +
> +local res, err = pcall(res_f)
> +
> +-- Check consistency with PUC Rio Lua 5.1 behaviour.
> +test:ok(not res, 'loaded function not executed')
> +test:like(err, 'attempt to call a nil value', 'correct error message')
> +
> +test:done(true)

[-- Attachment #2: Type: text/html, Size: 4316 bytes --]

Re: [Tarantool-patches] [PATCH luajit] Fix predict_next() in parser (for real now).

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!
Thanks for the review.

On 10.01.25, Sergey Bronnikov wrote:
> Hi, Sergey,
> 
> thanks for the patch!
> 
> test is passed with reverted patch.
> 
> With original reproducer luajit segfaults.
> 
> CMake options:  cmake -S . -B build -DCMAKE_BUILD_TYPE=Debug 
> -DLUA_USE_APICHECK=ON -DLUA_USE_ASSERT=ON

You must run this test with Valgrind enabled to see the failure as it is
mentioned in the commit. The original reproducer is clumsy and unstable
-- any slight change of the parser may break it. This reproducer is much
simpler, robust, and more readable.

> 
> 
> Sergey
> 
> On 09.01.2025 18:01, Sergey Kaplun wrote:

<snipped>

> > +-- The parsing of the `for` iterator uses the incorrect check for
> > +-- `fs->bclim`, which allows the usage of an uninitialized value,
> > +-- so the test fails under Valgrind.

<snipped>

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit] Fix predict_next() in parser (for real now).

[Sergey Bronnikov via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 1145 bytes --]


On 10.01.2025 14:10, Sergey Kaplun wrote:
> Hi, Sergey!
> Thanks for the review.
>
> On 10.01.25, Sergey Bronnikov wrote:
>> Hi, Sergey,
>>
>> thanks for the patch!
>>
>> test is passed with reverted patch.
>>
>> With original reproducer luajit segfaults.
>>
>> CMake options:  cmake -S . -B build -DCMAKE_BUILD_TYPE=Debug
>> -DLUA_USE_APICHECK=ON -DLUA_USE_ASSERT=ON
> You must run this test with Valgrind enabled to see the failure as it is
> mentioned in the commit.


cmake arguments required for reproducing a bug by proposed test:

  cmake -S . -B build -DCMAKE_BUILD_TYPE=Debug -DLUA_USE_APICHECK=ON 
-DLUA_USE_ASSERT=ON -DLUAJIT_USE_VALGRIND=ON -DLUAJIT_ENABLE_GC64=ON 
-DLUAJIT_USE_SYSMALLOC=ON


> The original reproducer is clumsy and unstable
> -- any slight change of the parser may break it. This reproducer is much
> simpler, robust, and more readable.
>
>>
>> Sergey
>>
>> On 09.01.2025 18:01, Sergey Kaplun wrote:
> <snipped>
>
>>> +-- The parsing of the `for` iterator uses the incorrect check for
>>> +-- `fs->bclim`, which allows the usage of an uninitialized value,
>>> +-- so the test fails under Valgrind.
> <snipped>
>

[-- Attachment #2: Type: text/html, Size: 2222 bytes --]

Re: [Tarantool-patches] [PATCH luajit] Fix predict_next() in parser (for real now).

[Sergey Bronnikov via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 3464 bytes --]

Hi, Sergey

thanks for the patch, LGTM with a minor question below.

On 09.01.2025 18:01, Sergey Kaplun wrote:
> From: Mike Pall <mike>
>
> Reported by Sergey Kaplun.
>
> (cherry picked from commit f602f0154b644211283cfeea92a570ca38f71947)
>
> Before the patch `predict_next()` uses the pc allocation limit
> (`fs->bclim`) instead of the real limit of the defined bytecodes
> (`fs->pc`). This leads to the use of undefined value and possible
> crash. This patch fixes the check.
>
> Sergey Kaplun:
> * added the description and the test for the problem
>
> Part of tarantool/tarantool#10709
> ---
>
> Branch:https://github.com/tarantool/luajit/tree/skaplun/lj-1226-fix-predict-next
> Related issues:
> *https://github.com/LuaJIT/LuaJIT/issues/1226
> *https://github.com/tarantool/tarantool/issues/10709
>
>   src/lj_parse.c                                |  6 ++--
>   .../lj-1226-fix-predict-next.test.lua         | 31 +++++++++++++++++++
>   2 files changed, 33 insertions(+), 4 deletions(-)
>   create mode 100644 test/tarantool-tests/lj-1226-fix-predict-next.test.lua
>
> diff --git a/src/lj_parse.c b/src/lj_parse.c
> index 9b45b103..ec85ac9b 100644
> --- a/src/lj_parse.c
> +++ b/src/lj_parse.c
> @@ -2527,11 +2527,9 @@ static void parse_for_num(LexState *ls, GCstr *varname, BCLine line)
>   */
>   static int predict_next(LexState *ls, FuncState *fs, BCPos pc)
>   {
> -  BCIns ins;
> +  BCIns ins = fs->bcbase[pc].ins;
>     GCstr *name;
>     cTValue *o;
> -  if (pc >= fs->bclim) return 0;
> -  ins = fs->bcbase[pc].ins;
>     switch (bc_op(ins)) {
>     case BC_MOV:
>       if (bc_d(ins) >= fs->nactvar) return 0;
> @@ -2580,7 +2578,7 @@ static void parse_for_iter(LexState *ls, GCstr *indexname)
>     assign_adjust(ls, 3, expr_list(ls, &e), &e);
>     /* The iterator needs another 3 [4] slots (func [pc] | state ctl). */
>     bcreg_bump(fs, 3+LJ_FR2);
> -  isnext = (nvars <= 5 && predict_next(ls, fs, exprpc));
> +  isnext = (nvars <= 5 && fs->pc > exprpc && predict_next(ls, fs, exprpc));
>     var_add(ls, 3);  /* Hidden control variables. */
>     lex_check(ls, TK_do);
>     loop = bcemit_AJ(fs, isnext ? BC_ISNEXT : BC_JMP, base, NO_JMP);
> diff --git a/test/tarantool-tests/lj-1226-fix-predict-next.test.lua b/test/tarantool-tests/lj-1226-fix-predict-next.test.lua
> new file mode 100644
> index 00000000..3cd2c8f5
> --- /dev/null
> +++ b/test/tarantool-tests/lj-1226-fix-predict-next.test.lua
> @@ -0,0 +1,31 @@
> +local tap = require('tap')
> +local test = tap.test('lj-1226-fix-predict-next')
> +
> +test:plan(3)
> +
> +-- The resulting bytecode is the following:
> +--
> +-- 0001    KNIL     0   3
> +-- 0002    JMP      4 => 0003
> +-- 0003 => ITERC    4   2   3
> +-- 0004    ITERL    4 => 0003
> +--
> +-- The parsing of the `for` iterator uses the incorrect check for
> +-- `fs->bclim`, which allows the usage of an uninitialized value,
> +-- so the test fails under Valgrind.
> +local res_f = loadstring([[
> +-- This local variable is necessary, because it emits `KPRI`
> +-- bytecode, with which the next `KPRI` bytecode will be merged.
> +local _
> +for _ in nil do end
> +]])
> +
> +test:ok(res_f, 'chunk loaded successfully')
> +
> +local res, err = pcall(res_f)
> +
> +-- Check consistency with PUC Rio Lua 5.1 behaviour.
What for? It is not related to the bug and fix for it.
> +test:ok(not res, 'loaded function not executed')
> +test:like(err, 'attempt to call a nil value', 'correct error message')
> +
> +test:done(true)

[-- Attachment #2: Type: text/html, Size: 4299 bytes --]

Re: [Tarantool-patches] [PATCH luajit] Fix predict_next() in parser (for real now).

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!
Thanks for the review!

On 13.01.25, Sergey Bronnikov wrote:
> Hi, Sergey
> 
> thanks for the patch, LGTM with a minor question below.

Please consider my answers below.

> 
> On 09.01.2025 18:01, Sergey Kaplun wrote:
> > From: Mike Pall <mike>
> >
> > Reported by Sergey Kaplun.
> >
> > (cherry picked from commit f602f0154b644211283cfeea92a570ca38f71947)
> >
> > Before the patch `predict_next()` uses the pc allocation limit
> > (`fs->bclim`) instead of the real limit of the defined bytecodes
> > (`fs->pc`). This leads to the use of undefined value and possible
> > crash. This patch fixes the check.
> >
> > Sergey Kaplun:
> > * added the description and the test for the problem
> >
> > Part of tarantool/tarantool#10709
> > ---
> >
> > Branch:https://github.com/tarantool/luajit/tree/skaplun/lj-1226-fix-predict-next
> > Related issues:
> > *https://github.com/LuaJIT/LuaJIT/issues/1226
> > *https://github.com/tarantool/tarantool/issues/10709

<snipped>

> > +-- The resulting bytecode is the following:
> > +--
> > +-- 0001    KNIL     0   3
> > +-- 0002    JMP      4 => 0003
> > +-- 0003 => ITERC    4   2   3
> > +-- 0004    ITERL    4 => 0003
> > +--
> > +-- The parsing of the `for` iterator uses the incorrect check for
> > +-- `fs->bclim`, which allows the usage of an uninitialized value,
> > +-- so the test fails under Valgrind.
> > +local res_f = loadstring([[
> > +-- This local variable is necessary, because it emits `KPRI`
> > +-- bytecode, with which the next `KPRI` bytecode will be merged.
> > +local _
> > +for _ in nil do end
> > +]])
> > +
> > +test:ok(res_f, 'chunk loaded successfully')
> > +
> > +local res, err = pcall(res_f)
> > +
> > +-- Check consistency with PUC Rio Lua 5.1 behaviour.
> What for? It is not related to the bug and fix for it.

I considered this a simple canary test. The main check is done during
parsing by Valgrind, obviously. But still don't see anything crucial in
checking the parsing behaviour this way too (just to be sure).

> > +test:ok(not res, 'loaded function not executed')
> > +test:like(err, 'attempt to call a nil value', 'correct error message')
> > +
> > +test:done(true)

-- 
Best regards,
Sergey Kaplun