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

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

[Sergey Bronnikov via Tarantool-patches]

From: sergeyb@tarantool.org

Reported by Sergey Bronnikov. #1054

(cherry picked from commit 309fb42b871b6414f53e0e0e708bce0b0d62daff)

The following Lua snippet triggers an out of boundary access to a stack:

```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.

In a function `predict_next` variable `exprpc` looks forward and expects
extra bytecodes on the stack. However, `KPRI` is merged to the `KNIL`
and there is no new bytecode to add, so `exprpc == fs->bclim` and it
leads to out of boundary access.

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
@@ -2511,9 +2511,11 @@ static void parse_for_num(LexState *ls, GCstr *varname, BCLine line)
 */
 static int predict_next(LexState *ls, FuncState *fs, BCPos pc)
 {
-  BCIns ins = fs->bcbase[pc].ins;
+  BCIns ins;
   GCstr *name;
   cTValue *o;
+  if (pc >= fs->bclim) return 0;
+  ins = fs->bcbase[pc].ins;
   switch (bc_op(ins)) {
   case BC_MOV:
     name = gco2str(gcref(var_get(ls, fs, bc_d(ins)).name));
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)
+
+
+-- The test demonstrates a problem with out of boundary access to a stack.
+-- Sample executed in LuaJIT instrumented by ASAN leads to
+-- a heap-buffer-overflow.
+-- See also https://github.com/LuaJIT/LuaJIT/issues/528
+local lua_code = [[
+a, b, c = 1, 2, 3
+local d
+for _ in nil do end
+]]
+
+test:ok(loadstring(lua_code), 'parsing is correct')
+
+test:done(true)
-- 
2.34.1

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

[Sergey Kaplun via Tarantool-patches]

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

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

[Sergey Bronnikov via Tarantool-patches]

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

Hi, Sergey

thanks for review! See my comments.

New changes were force-pushed.


On 8/29/23 16:38, Sergey Kaplun wrote:
> 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. :)

Agree, removed to avoid troubles.


>
>> (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/

Fixed.


>
>> ```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?

Right. Rephrased it:


+-- The test demonstrates a problem with out-of-boundary access
+-- to a stack. The problem can be easily observed on execution
+-- the sample by LuaJIT by ASAN, sanitizer reports a heap-buffer-overflow.


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

Fixed. However "function" was before "predict_next".


>
>> extra bytecodes on the stack. However, `KPRI` is merged to the `KNIL`
> Typo: s/the `KNIL`/`KNIL`

Fixed.


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


Fixed.


>
>> leads to out of boundary access.
> Typo: s/out of boundary/out-of-boundary/


Fixed.


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

Added.


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

Fixed.


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

Fixed.


>
>> +-- Sample executed in LuaJIT instrumented by ASAN leads to
>> +-- a heap-buffer-overflow.
> Minor: IDK why, but suggested varian here is "heap buffer overflow".


ASAN reports error with hyphens, like this:

|==90673==ERROR: AddressSanitizer: heap-buffer-overflow on address 
0x6020000000fb at pc 0x000108868a95 bp 0x7fff573979a0 sp 0x7fff57397998 
READ of size 1 at 0x6020000000fb thread T0|

If you don't like variant "heap-buffer-overflow" then we can use variant 
used in CWE list: "heap-based buffer overflow", see [1].

What variant should

1. https://cwe.mitre.org/data/definitions/122.html

>
>> +-- See alsohttps://github.com/LuaJIT/LuaJIT/issues/528
> I suggest to add an empty line here.
Added.
>
>> +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).

Updated:


TAP version 13
1..3
ok - chunk loaded successfully
ok - loaded function is failed (expected)
ok - correct error message


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

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

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

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!
Thanks for the updates!
LGTM, after fixing several minor comments below.

On 29.08.23, Sergey Bronnikov wrote:
> Hi, Sergey
> 
> thanks for review! See my comments.
> 
> New changes were force-pushed.
> 
> 
> On 8/29/23 16:38, Sergey Kaplun wrote:
> > Hi, Sergey!
> > Thanks for the patch!
> > Please consider my comments below.
> >
> > On 29.08.23, Sergey Bronnikov wrote:
> >> From:sergeyb@tarantool.org
> >>

<snipped>

> >> 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.
> 
> Fixed. However "function" was before "predict_next".
> 

Yes, I understand, its just matter of taste :).

<snipped>

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

I suggest to use predict-next instead in filename and testname to be
consistent with other tests.

> >>   2 files changed, 21 insertions(+), 1 deletion(-)
> >>   create mode 100644 test/tarantool-tests/lj-1054-incorrect-pc-value-in-predict_next.test.lua
> >>

<snipped>

> >
> >> +-- Sample executed in LuaJIT instrumented by ASAN leads to
> >> +-- a heap-buffer-overflow.
> > Minor: IDK why, but suggested varian here is "heap buffer overflow".
> 
> 
> ASAN reports error with hyphens, like this:
> 
> |==90673==ERROR: AddressSanitizer: heap-buffer-overflow on address 
> 0x6020000000fb at pc 0x000108868a95 bp 0x7fff573979a0 sp 0x7fff57397998 
> READ of size 1 at 0x6020000000fb thread T0|
> 
> If you don't like variant "heap-buffer-overflow" then we can use variant 
> used in CWE list: "heap-based buffer overflow", see [1].
> 
> What variant should
> 
> 1. https://cwe.mitre.org/data/definitions/122.html

Yes, lets used it.

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

Nit: Missed dot at the end of the sentence.
Typo: s/528/1054./

<snipped>

-- 
Best regards,
Sergey Kaplun

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

[Sergey Bronnikov via Tarantool-patches]

Hi,


On 8/29/23 17:43, Sergey Kaplun wrote:
> Hi, Sergey!
> Thanks for the updates!
> LGTM, after fixing several minor comments below.
>
> On 29.08.23, Sergey Bronnikov wrote:

<snipped>


> 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 ++++++++++++++++++
> I suggest to use predict-next instead in filename and testname to be
> consistent with other tests.

Fixed:


--- 
a/test/tarantool-tests/lj-1054-fix-incorrect-pc-value-in-predict_next.test.lua
+++ 
b/test/tarantool-tests/lj-1054-fix-incorrect-pc-value-in-predict_next.test.lua
@@ -1,12 +1,12 @@
  local tap = require('tap')
-local test = tap.test('lj-1054-incorrect-pc-value-in-predict_next')
+local test = tap.test('lj-1054-fix-incorrect-pc-value-in-predict_next')
  test:plan(3)


<snipped>


>>>> +-- See alsohttps://github.com/LuaJIT/LuaJIT/issues/528
> Nit: Missed dot at the end of the sentence.
> Typo: s/528/1054./
>
> <snipped>
>

  -- The test demonstrates a problem with out-of-boundary
  -- access to a stack. The problem can be easily observed
  -- on execution the sample by LuaJIT by ASAN, sanitizer
  -- reports a heap-based buffer overflow.
--- See also https://github.com/LuaJIT/LuaJIT/issues/528
+-- See also https://github.com/LuaJIT/LuaJIT/issues/1054.

  local res_f = loadstring([[
  a, b, c = 1, 2, 3

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

[Maxim Kokryashkin via Tarantool-patches]

Hi, Sergey!
Thanks for the patch!
LGTM, except for a few nits below.
On Tue, Aug 29, 2023 at 01:42:40PM +0300, Sergey Bronnikov via Tarantool-patches wrote:
> From: sergeyb@tarantool.org
> 
> Reported by Sergey Bronnikov. #1054
> 
> (cherry picked from commit 309fb42b871b6414f53e0e0e708bce0b0d62daff)
> 
> The following Lua snippet triggers an out of boundary access to a stack:
> 
> ```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 suggest the following rephrasing with grammar fixes:
| During the execution of this snippet with LuaJIT instrumented by ASAN,
| it leads to a heap buffer overflow.
> 
> In a function `predict_next` variable `exprpc` looks forward and expects
Typo: s/In a/In/
> extra bytecodes on the stack. However, `KPRI` is merged to the `KNIL`
Typo: s/to the/to/
> and there is no new bytecode to add, so `exprpc == fs->bclim` and it
> leads to out of boundary access.

The last sentence that you don't have here, but have on GitHub should look like
the following:
| The issue has been fixed by an early return when `pc >= fs->bclim`.
> 
> 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
> @@ -2511,9 +2511,11 @@ static void parse_for_num(LexState *ls, GCstr *varname, BCLine line)
>  */
>  static int predict_next(LexState *ls, FuncState *fs, BCPos pc)
>  {
> -  BCIns ins = fs->bcbase[pc].ins;
> +  BCIns ins;
>    GCstr *name;
>    cTValue *o;
> +  if (pc >= fs->bclim) return 0;
> +  ins = fs->bcbase[pc].ins;
>    switch (bc_op(ins)) {
>    case BC_MOV:
>      name = gco2str(gcref(var_get(ls, fs, bc_d(ins)).name));
> 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)
> +
> +
> +-- The test demonstrates a problem with out of boundary access to a stack.
> +-- Sample executed in LuaJIT instrumented by ASAN leads to
> +-- a heap-buffer-overflow.
> +-- See also https://github.com/LuaJIT/LuaJIT/issues/528
This chunk is a bit dated and I don't really want to bother with
going through a bunch of emails and sequential diffs, so I'll just
bring the actual one here by myself.

Here it is:
-- The test demonstrates a problem with out-of-boundary
-- access to a stack. The problem can be easily observed
-- on execution the sample by LuaJIT by ASAN, sanitizer
Typo: s/execution/execution of/
Typo: s/sanitizer/where the sanitizer/
-- reports a heap-based buffer overflow.
-- See also https://github.com/LuaJIT/LuaJIT/issues/1054.

Otherwise, considering the changes you've already made after
Sergey's comments, this part is ok.
> +local lua_code = [[
> +a, b, c = 1, 2, 3
> +local d
> +for _ in nil do end
> +]]
> +
> +test:ok(loadstring(lua_code), 'parsing is correct')
> +
> +test:done(true)
> -- 
> 2.34.1
> 

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

[Sergey Bronnikov via Tarantool-patches]

Hi, Max


thanks for review! See my answers.

Updated branch force-pushed.


Sergey


On 8/30/23 13:53, Maxim Kokryashkin via Tarantool-patches wrote:
> Hi, Sergey!
> Thanks for the patch!
> LGTM, except for a few nits below.
> On Tue, Aug 29, 2023 at 01:42:40PM +0300, Sergey Bronnikov via Tarantool-patches wrote:
>> From: sergeyb@tarantool.org
>>
>> Reported by Sergey Bronnikov. #1054
>>
>> (cherry picked from commit 309fb42b871b6414f53e0e0e708bce0b0d62daff)
>>
>> The following Lua snippet triggers an out of boundary access to a stack:
>>
>> ```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 suggest the following rephrasing with grammar fixes:
> | During the execution of this snippet with LuaJIT instrumented by ASAN,
> | it leads to a heap buffer overflow.

Updated, but replaced "heap buffer overflow" with "heap buffer overflow" 
(same wording is used in CWE [1]).


1. https://cwe.mitre.org/data/definitions/122.html


>> In a function `predict_next` variable `exprpc` looks forward and expects
> Typo: s/In a/In/

Fixed.


>> extra bytecodes on the stack. However, `KPRI` is merged to the `KNIL`
> Typo: s/to the/to/

Fixed.


>> and there is no new bytecode to add, so `exprpc == fs->bclim` and it
>> leads to out of boundary access.
> The last sentence that you don't have here, but have on GitHub should look like
> the following:
> | The issue has been fixed by an early return when `pc >= fs->bclim`.


Fixed.


>> 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
>> @@ -2511,9 +2511,11 @@ static void parse_for_num(LexState *ls, GCstr *varname, BCLine line)
>>   */
>>   static int predict_next(LexState *ls, FuncState *fs, BCPos pc)
>>   {
>> -  BCIns ins = fs->bcbase[pc].ins;
>> +  BCIns ins;
>>     GCstr *name;
>>     cTValue *o;
>> +  if (pc >= fs->bclim) return 0;
>> +  ins = fs->bcbase[pc].ins;
>>     switch (bc_op(ins)) {
>>     case BC_MOV:
>>       name = gco2str(gcref(var_get(ls, fs, bc_d(ins)).name));
>> 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)
>> +
>> +
>> +-- The test demonstrates a problem with out of boundary access to a stack.
>> +-- Sample executed in LuaJIT instrumented by ASAN leads to
>> +-- a heap-buffer-overflow.
>> +-- See also https://github.com/LuaJIT/LuaJIT/issues/528
> This chunk is a bit dated and I don't really want to bother with
> going through a bunch of emails and sequential diffs, so I'll just
> bring the actual one here by myself.
>
> Here it is:
> -- The test demonstrates a problem with out-of-boundary
> -- access to a stack. The problem can be easily observed
> -- on execution the sample by LuaJIT by ASAN, sanitizer
> Typo: s/execution/execution of/
> Typo: s/sanitizer/where the sanitizer/
> -- reports a heap-based buffer overflow.
> -- See also https://github.com/LuaJIT/LuaJIT/issues/1054.
>
> Otherwise, considering the changes you've already made after
> Sergey's comments, this part is ok.

Updated comment:


--- 
a/test/tarantool-tests/lj-1054-fix-incorrect-pc-value-in-predict_next.test.lua
+++ 
b/test/tarantool-tests/lj-1054-fix-incorrect-pc-value-in-predict_next.test.lua
@@ -4,8 +4,8 @@ test:plan(3)

  -- The test demonstrates a problem with out-of-boundary
  -- access to a stack. The problem can be easily observed
--- on execution the sample by LuaJIT by ASAN, sanitizer
--- reports a heap-based buffer overflow.
+-- on execution of the sample by LuaJIT instrumented by ASAN,
+-- where the sanitizer reports a heap-based buffer overflow.
  -- See also https://github.com/LuaJIT/LuaJIT/issues/1054.

  local res_f = loadstring([[

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

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

[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/2.11 and
release/2.10.

On 29.08.23, Sergey Bronnikov via Tarantool-patches wrote:
> From: sergeyb@tarantool.org
> 
> Reported by Sergey Bronnikov. #1054
> 
> (cherry picked from commit 309fb42b871b6414f53e0e0e708bce0b0d62daff)
> 
> The following Lua snippet triggers an out of boundary access to a stack:
> 
> ```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.
> 
> In a function `predict_next` variable `exprpc` looks forward and expects
> extra bytecodes on the stack. However, `KPRI` is merged to the `KNIL`
> and there is no new bytecode to add, so `exprpc == fs->bclim` and it
> leads to out of boundary access.
> 
> 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
> 

<snipped>

> -- 
> 2.34.1
> 

-- 
Best regards,
IM