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

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

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