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

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

Hi, Sergey!
Thanks for the patch!
Please consider my comments below.

On 29.08.23, Sergey Bronnikov wrote:
> From: sergeyb@tarantool.org
> 
> Reported by Sergey Bronnikov. #1054

I suggest to remove the ticket number, to avoid trouble trouble until
trouble troubles you. :)

> 
> (cherry picked from commit 309fb42b871b6414f53e0e0e708bce0b0d62daff)
> 
> The following Lua snippet triggers an out of boundary access to a stack:

Typo: s/an out of boundary/out-of-boundary/

> 
> ```lua
> a, b, c = 1, 2, 3
> local d
> for _ in nil do end
> ```
> 
> With execution snippet by LuaJIT instrumented by ASAN it leads to
> a heap-buffer-overflow.

I suppose that it leads ever without ASAN, but the issue is
observable only with ASAN, isn't it?

> 
> In a function `predict_next` variable `exprpc` looks forward and expects

Minor: I suggest using of `()` for distinguishing function and variable
names.
Feel free to ignore.

> extra bytecodes on the stack. However, `KPRI` is merged to the `KNIL`

Typo: s/the `KNIL`/`KNIL`

> and there is no new bytecode to add, so `exprpc == fs->bclim` and it

Typo: /fs->bclim`/fs->bclim`,/

> leads to out of boundary access.

Typo: s/out of boundary/out-of-boundary/

> 

Minor: I suppose that we can mention that the patch fixes the issue via
early return.

> Sergey Bronnikov:
> * added the description and the test for the problem
> 
> Part of tarantool/tarantool#8825
> ---
> 
> PR: https://github.com/tarantool/tarantool/pull/9054
> Branch: https://github.com/tarantool/luajit/tree/ligurio/lj-1054-incorrect-pc-value-predict_next
> Related issue:
> * https://github.com/LuaJIT/LuaJIT/issues/1054
> 
>  src/lj_parse.c                                 |  4 +++-
>  ...incorrect-pc-value-in-predict_next.test.lua | 18 ++++++++++++++++++
>  2 files changed, 21 insertions(+), 1 deletion(-)
>  create mode 100644 test/tarantool-tests/lj-1054-incorrect-pc-value-in-predict_next.test.lua
> 
> diff --git a/src/lj_parse.c b/src/lj_parse.c
> index 343fa797..f1015960 100644
> --- a/src/lj_parse.c
> +++ b/src/lj_parse.c

<snipped>

> diff --git a/test/tarantool-tests/lj-1054-incorrect-pc-value-in-predict_next.test.lua b/test/tarantool-tests/lj-1054-incorrect-pc-value-in-predict_next.test.lua
> new file mode 100644
> index 00000000..17f1b994
> --- /dev/null
> +++ b/test/tarantool-tests/lj-1054-incorrect-pc-value-in-predict_next.test.lua
> @@ -0,0 +1,18 @@
> +local tap = require('tap')
> +local test = tap.test('lj-1054-incorrect-pc-value-in-predict_next')
> +test:plan(1)
> +
> +

Excess empty line.

> +-- The test demonstrates a problem with out of boundary access to a stack.

Typo: s/out of boundary/out-of-boundary/
Comment line width is more than 66 symbols.

> +-- Sample executed in LuaJIT instrumented by ASAN leads to
> +-- a heap-buffer-overflow.

Minor: IDK why, but suggested varian here is "heap buffer overflow".

> +-- See also https://github.com/LuaJIT/LuaJIT/issues/528

I suggest to add an empty line here.

> +local lua_code = [[
> +a, b, c = 1, 2, 3
> +local d
> +for _ in nil do end
> +]]
> +
> +test:ok(loadstring(lua_code), 'parsing is correct')

I suggest also to test that the behaviour of the executed chunk is the
same as in the PUC RIO Lua 5.1 (like it is done for the lj-1033).

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

-- 
Best regards,
Sergey Kaplun