Re: [Tarantool-patches] [PATCH luajit] Prevent loop in snap_usedef().

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

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

Hi, Maxim,

thanks for review! Please see my comments below.

Fixes applied and force-pushed.

On 26.03.2024 19:04, Maxim Kokryashkin wrote:
> Hi, Sergey!
> Thanks for the patch!
> Please consider my comments below.
> On Tue, Mar 26, 2024 at 06:29:11PM +0300, Sergey Bronnikov wrote:
>> From: Sergey Bronnikov<sergeyb@tarantool.org>
>>
>> Reported by XmiliaH.
>>
>> (cherry picked from commit 0e66fc96377853d898390f1a02723c54ec3a42f7)
>>
>> It is possible to get an infinite loop in a function `snap_usedef`
>> when a `UCLO` makes a tight loop.
> The description should include explanation for the cause of the issue
> and should explain how it was resolved.

Added more details about patch to commit message:

Updated description:

     It is possible to get an infinite loop in a function `snap_usedef`
     when a `UCLO` makes a tight loop. This infinite loop could happen
     when `snap_usedef()` is called on trace exit processes UCLO
     bytecode instruction and this instruction attempts a jump with
     negative value. The patch fixes the problem by checking a number
     of slots in a jump argument and replace this value my `maxslot` if
     a value is negative.

>> Sergey Bronnikov:
>> * added the description and the test for the problem
>>
>> Part of tarantool/tarantool#9595
>> ---
>> Branch:https://github.com/tarantool/luajit/tree/ligurio/lj-736-prevent-loop-in-snap_usedef
>> Issues:
>> -https://github.com/LuaJIT/LuaJIT/issues/736
>> -https://github.com/tarantool/tarantool/issues/9595
>>
>>   src/lj_snap.c                                 |  7 ++-
>>   .../lj-736-BC_UCLO-triggers-infinite-loop.lua | 59 +++++++++++++++++++
>>   2 files changed, 65 insertions(+), 1 deletion(-)
>>   create mode 100644 test/tarantool-tests/lj-736-BC_UCLO-triggers-infinite-loop.lua
>>
>> diff --git a/src/lj_snap.c b/src/lj_snap.c
>> index 5a00b5cd..0710e1f0 100644
>> --- a/src/lj_snap.c
>> +++ b/src/lj_snap.c
>> @@ -252,7 +252,12 @@ static BCReg snap_usedef(jit_State *J, uint8_t *udf,
>>         BCReg minslot = bc_a(ins);
>>         if (op >= BC_FORI && op <= BC_JFORL) minslot += FORL_EXT;
>>         else if (op >= BC_ITERL && op <= BC_JITERL) minslot += bc_b(pc[-2])-1;
>> -      else if (op == BC_UCLO) { pc += bc_j(ins); break; }
>> +      else if (op == BC_UCLO) {
>> +	ptrdiff_t delta = bc_j(ins);
>> +	if (delta < 0) return maxslot;  /* Prevent loop. */
>> +	pc += delta;
>> +	break;
>> +      }
>>         for (s = minslot; s < maxslot; s++) DEF_SLOT(s);
>>         return minslot < maxslot ? minslot : maxslot;
>>         }
>> diff --git a/test/tarantool-tests/lj-736-BC_UCLO-triggers-infinite-loop.lua b/test/tarantool-tests/lj-736-BC_UCLO-triggers-infinite-loop.lua
>> new file mode 100644
>> index 00000000..28a2b61b
>> --- /dev/null
>> +++ b/test/tarantool-tests/lj-736-BC_UCLO-triggers-infinite-loop.lua
>> @@ -0,0 +1,59 @@
>> +local tap = require('tap')
>> +local test = tap.test('lj-736-BC_UCLO-triggers-infinite-loop'):skipcond({
>> +  ['Test requires JIT enabled'] = not jit.status(),
>> +})
>> +
>> +test:plan(1)
>> +
>> +-- Test reproduces an issue when BC_UCLO triggers an infinite loop.
>> +-- See details inhttps://github.com/LuaJIT/LuaJIT/issues/736.
>> +--
>> +-- Listing below demonstrates a problem -
>> +-- the bytecode UCLO on the line 13 makes a loop at 0013-0014:
>> +--
>> +-- - BYTECODE -- bc_uclo.lua:0-20
>> +-- 0001    KPRI     0   0
>> +-- 0002    FNEW     1   0      ; bc_uclo.lua:5
>> +-- 0003    KSHORT   2   1
>> +-- 0004    KSHORT   3   4
>> +-- 0005    KSHORT   4   1
>> +-- 0006    FORI     2 => 0011
>> +-- 0007 => ISNEN    5   0      ; 2
>> +-- 0008    JMP      6 => 0010
>> +-- 0009    UCLO     0 => 0012
>> +-- 0010 => FORL     2 => 0007
>> +-- 0011 => UCLO     0 => 0012
>> +-- 0012 => KPRI     0   0
>> +-- 0013    UCLO     0 => 0012
>> +-- 0014    FNEW     1   1      ; bc_uclo.lua:18
>> +-- 0015    UCLO     0 => 0016
>> +-- 0016 => RET0     0   1
>> +
>> +jit.opt.start('hotloop=1')
>> +
>> +do
>> +  local uv = 0
>> +  local w = function() return uv end -- luacheck: no unused
>> +  for i = 1, 2 do
> Add a comment that we have two iterations only because we only
> need to record the trace.
Added.
>> +    -- Infinite loop is here.
>> +    if i == 2 then
>> +      if i == 2 then
>> +        goto pass
>> +      end
>> +      goto unreachable
>> +    end
>> +  end
>> +end
>> +
>> +::unreachable::
>> +-- Lua chunk below is required for reproducing a bug.
>> +do
>> +  local uv = 0 -- luacheck: no unused
>> +  goto unreachable
>> +  local w = function() return uv end -- luacheck: ignore
>> +end
>> +
>> +::pass::
> Please add a comment explaining why do we need a goto statement
> and an unreachable code segment.
Added more comments to the test.
>> +
>> +test:ok(true, 'BC_UCLO does not trigger an infinite loop')
>> +os.exit(test:check() and 0 or 1)
> The test executes without any failures for the x86 non-GC64
> build before the patch. The exact command:
> $ cmake .. -DCMAKE_BUILD_TYPE=Debug -DLUA_USE_ASSERT=ON -DLUA_USE_APICHECK=ON && make -j && make test


Fixed that. Now without patch test hangs and passed with applied patch.

>> --
>> 2.34.1
>>

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