From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id BB148EDCC56; Mon, 13 Jan 2025 18:18:49 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org BB148EDCC56 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1736781529; bh=tYczOkeAisSBoCFVhKLhgKeo/c5xphzqz+kPvY6YVMA=; h=Date:To:Cc:References:In-Reply-To:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=RS6FI6+4sJ59nuP7lQ4CxaaD+CJhCujTpqS4C3F5X3WEWuTOdnU7BghDHGmyDduaA ztcOfn7Lhic+t5afJ0QbwhbaMMszIJvwf18NGkRe72cFDYVAv2XADNwMiqrDf6aLET 30EOCLsSBOZ+x0te957Jupj2gTsr/xhCH8cUGUD8= Received: from send242.i.mail.ru (send242.i.mail.ru [95.163.59.81]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 241CB43D1A9 for ; Mon, 13 Jan 2025 18:18:48 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 241CB43D1A9 Received: by exim-smtp-bcf9586c5-k8hdp with esmtpa (envelope-from ) id 1tXMDH-000000009fK-0uwz; Mon, 13 Jan 2025 18:18:47 +0300 Content-Type: multipart/alternative; boundary="------------XwKzLJeybMMGd7EfITMAu3Rs" Message-ID: <64c5cff1-e038-4844-8950-f255df8de717@tarantool.org> Date: Mon, 13 Jan 2025 18:18:47 +0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Content-Language: en-US To: Sergey Kaplun Cc: tarantool-patches@dev.tarantool.org References: <20250109150124.23841-1-skaplun@tarantool.org> In-Reply-To: <20250109150124.23841-1-skaplun@tarantool.org> X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD97C62A2C8925CC6475EB1DE9CB11F615261801260394DC45D182A05F538085040FC0653EF563533BE3DE06ABAFEAF6705F8495B8BD90ED1EEC9CABDAFE3DBE4E322AF0AD8BA8DF889 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7A25B432396ED6D32EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006374DF0C582D42FCA168638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D88EEB0DB1451279316908A4FDD9FD6234B6826919C4231AB5CC7F00164DA146DAFE8445B8C89999728AA50765F7900637F3E38EE449E3E2AE389733CBF5DBD5E9C8A9BA7A39EFB766F5D81C698A659EA7CC7F00164DA146DA9985D098DBDEAEC8B861051D4BA689FCF6B57BC7E6449061A352F6E88A58FB86F5D81C698A659EA73AA81AA40904B5D9A18204E546F3947C4E7D9683544204AF6E0066C2D8992A164AD6D5ED66289B523666184CF4C3C14F6136E347CC761E07725E5C173C3A84C3B74263D4D5690889BA3038C0950A5D36B5C8C57E37DE458B330BD67F2E7D9AF16D1867E19FE14079C09775C1D3CA48CF3D321E7403792E342EB15956EA79C166A417C69337E82CC275ECD9A6C639B01B78DA827A17800CE73A6989AD488FD87D731C566533BA786AA5CC5B56E945C8DA X-C1DE0DAB: 0D63561A33F958A5116BC350DB19C7FF5002B1117B3ED69656C092ECC8483A18F5FEB6EB1EB183FD823CB91A9FED034534781492E4B8EEAD2B25D9E4C92BC8ACBDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF3FED46C3ACD6F73ED3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CF04C7E0C212748E2E2CA46FECC383555DAC54C711F120F47C5025DD714B53D77ED753EA1A396000B069A713A3E5A0090D51EECF70BCA541AA71E4EF79D846489FCD0692ED11E018DF111DC66A97D0BFE2913E6812662D5F2AB9AF64DB4688768036DF5FE9C0001AF333F2C28C22F508233FCF178C6DD14203 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojtuBC1linLcihw4re1D7JCQ== X-Mailru-Sender: 520A125C2F17F0B1E52FEF5D219D6140B85916601F6C20405D1BE6A8D71B10A5BFDA984555D6064A0152A3D17938EB451EB5A0BCEC6A560B3DDE9B364B0DF289BE2DA36745F2EEB5CEBA01FB949A1F1EEAB4BC95F72C04283CDA0F3B3F5B9367 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit] Fix predict_next() in parser (for real now). X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Sergey Bronnikov via Tarantool-patches Reply-To: Sergey Bronnikov Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" This is a multi-part message in MIME format. --------------XwKzLJeybMMGd7EfITMAu3Rs Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Hi, Sergey thanks for the patch, LGTM with a minor question below. On 09.01.2025 18:01, Sergey Kaplun wrote: > From: Mike Pall > > 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. What for? It is not related to the bug and fix for it. > +test:ok(not res, 'loaded function not executed') > +test:like(err, 'attempt to call a nil value', 'correct error message') > + > +test:done(true) --------------XwKzLJeybMMGd7EfITMAu3Rs Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 7bit

Hi, Sergey

thanks for the patch, LGTM with a minor question 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

 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.
What for? It is not related to the bug and fix for it.
+test:ok(not res, 'loaded function not executed')
+test:like(err, 'attempt to call a nil value', 'correct error message')
+
+test:done(true)
--------------XwKzLJeybMMGd7EfITMAu3Rs--