Hi, Sergey
thanks for the patch, LGTM with a minor question below.
What for? It is not related to the bug and fix for it.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 src/lj_parse.c | 6 ++-- .../lj-1226-fix-predict-next.test.lua | 31 +++++++++++++++++++ 2 files changed, 33 insertions(+), 4 deletions(-) create mode 100644 test/tarantool-tests/lj-1226-fix-predict-next.test.lua diff --git a/src/lj_parse.c b/src/lj_parse.c index 9b45b103..ec85ac9b 100644 --- a/src/lj_parse.c +++ b/src/lj_parse.c @@ -2527,11 +2527,9 @@ static void parse_for_num(LexState *ls, GCstr *varname, BCLine line) */ static int predict_next(LexState *ls, FuncState *fs, BCPos pc) { - BCIns ins; + BCIns ins = fs->bcbase[pc].ins; GCstr *name; cTValue *o; - if (pc >= fs->bclim) return 0; - ins = fs->bcbase[pc].ins; switch (bc_op(ins)) { case BC_MOV: if (bc_d(ins) >= fs->nactvar) return 0; @@ -2580,7 +2578,7 @@ static void parse_for_iter(LexState *ls, GCstr *indexname) assign_adjust(ls, 3, expr_list(ls, &e), &e); /* The iterator needs another 3 [4] slots (func [pc] | state ctl). */ bcreg_bump(fs, 3+LJ_FR2); - isnext = (nvars <= 5 && predict_next(ls, fs, exprpc)); + isnext = (nvars <= 5 && fs->pc > exprpc && predict_next(ls, fs, exprpc)); var_add(ls, 3); /* Hidden control variables. */ lex_check(ls, TK_do); loop = bcemit_AJ(fs, isnext ? BC_ISNEXT : BC_JMP, base, NO_JMP); diff --git a/test/tarantool-tests/lj-1226-fix-predict-next.test.lua b/test/tarantool-tests/lj-1226-fix-predict-next.test.lua new file mode 100644 index 00000000..3cd2c8f5 --- /dev/null +++ b/test/tarantool-tests/lj-1226-fix-predict-next.test.lua @@ -0,0 +1,31 @@ +local tap = require('tap') +local test = tap.test('lj-1226-fix-predict-next') + +test:plan(3) + +-- 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.
+test:ok(not res, 'loaded function not executed') +test:like(err, 'attempt to call a nil value', 'correct error message') + +test:done(true)