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

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

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