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

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

[-- 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 --]