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

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

Hi, Sergey!
Thanks for the review!

On 13.01.25, Sergey Bronnikov wrote:
> Hi, Sergey
> 
> thanks for the patch, LGTM with a minor question below.

Please consider my answers below.

> 
> On 09.01.2025 18:01, Sergey Kaplun wrote:
> > From: Mike Pall <mike>
> >
> > Reported by Sergey Kaplun.
> >
> > (cherry picked from commit f602f0154b644211283cfeea92a570ca38f71947)
> >
> > Before the patch `predict_next()` uses the pc allocation limit
> > (`fs->bclim`) instead of the real limit of the defined bytecodes
> > (`fs->pc`). This leads to the use of undefined value and possible
> > crash. This patch fixes the check.
> >
> > Sergey Kaplun:
> > * added the description and the test for the problem
> >
> > Part of tarantool/tarantool#10709
> > ---
> >
> > Branch:https://github.com/tarantool/luajit/tree/skaplun/lj-1226-fix-predict-next
> > Related issues:
> > *https://github.com/LuaJIT/LuaJIT/issues/1226
> > *https://github.com/tarantool/tarantool/issues/10709

<snipped>

> > +-- The resulting bytecode is the following:
> > +--
> > +-- 0001    KNIL     0   3
> > +-- 0002    JMP      4 => 0003
> > +-- 0003 => ITERC    4   2   3
> > +-- 0004    ITERL    4 => 0003
> > +--
> > +-- The parsing of the `for` iterator uses the incorrect check for
> > +-- `fs->bclim`, which allows the usage of an uninitialized value,
> > +-- so the test fails under Valgrind.
> > +local res_f = loadstring([[
> > +-- This local variable is necessary, because it emits `KPRI`
> > +-- bytecode, with which the next `KPRI` bytecode will be merged.
> > +local _
> > +for _ in nil do end
> > +]])
> > +
> > +test:ok(res_f, 'chunk loaded successfully')
> > +
> > +local res, err = pcall(res_f)
> > +
> > +-- Check consistency with PUC Rio Lua 5.1 behaviour.
> What for? It is not related to the bug and fix for it.

I considered this a simple canary test. The main check is done during
parsing by Valgrind, obviously. But still don't see anything crucial in
checking the parsing behaviour this way too (just to be sure).

> > +test:ok(not res, 'loaded function not executed')
> > +test:like(err, 'attempt to call a nil value', 'correct error message')
> > +
> > +test:done(true)

-- 
Best regards,
Sergey Kaplun