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 4171BF44B21; Fri, 10 Jan 2025 14:06:30 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 4171BF44B21 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1736507190; bh=SgQ9YPZmCWt0sbw1TEH3LrofFe3tHBK0s+TjWss9WAI=; 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=wECtd00MEqJ+1WhygUBGr2OhfvkNudysES/AR235wYc0mN2EHlyAi+Q+YQpC5dyFD 6mw7ZGxBC9lTYqqVyg0FNz+1SM8Cm5pCI8AtXl2GfrLj9i5Rsid6+ZG29aXierQi7A xLE5lnyfkcct8r83/6ggrYJ0VuC7m3IOW/A0h65I= Received: from send129.i.mail.ru (send129.i.mail.ru [89.221.237.224]) (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 F1A9242080C for ; Fri, 10 Jan 2025 14:06:28 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org F1A9242080C Received: by exim-smtp-bcf9586c5-ttk6t with esmtpa (envelope-from ) id 1tWCqR-00000000D4j-3JIX; Fri, 10 Jan 2025 14:06:28 +0300 Content-Type: multipart/alternative; boundary="------------70MGmKoF8IdpS2d4uBQ0JJup" Message-ID: <847b75f4-07a9-4c99-83b1-d9833197a82d@tarantool.org> Date: Fri, 10 Jan 2025 14:06:27 +0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Sergey Kaplun Cc: tarantool-patches@dev.tarantool.org References: <20250109150124.23841-1-skaplun@tarantool.org> Content-Language: en-US In-Reply-To: <20250109150124.23841-1-skaplun@tarantool.org> X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD97C62A2C8925CC647863DC6994F2EC40DB3D6979DDC943C06182A05F53808504006982FCFABD666CC3DE06ABAFEAF67053CE70BA8DBDF39017F7288B95132212509E014349702AB77 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7194E3A96294A28ABEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F790063764FE777F378F21448638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8797131878ABA693421DEB5C07E3E4AB1F4E87FDF26976F8FCC7F00164DA146DAFE8445B8C89999728AA50765F79006372A3B24BF85B2E607389733CBF5DBD5E9C8A9BA7A39EFB766F5D81C698A659EA7CC7F00164DA146DA9985D098DBDEAEC8A9FF340AA05FB58CF6B57BC7E6449061A352F6E88A58FB86F5D81C698A659EA73AA81AA40904B5D9A18204E546F3947C98E93883770458359735652A29929C6C4AD6D5ED66289B523666184CF4C3C14F6136E347CC761E07725E5C173C3A84C31DEDF2E2F39E2AF1BA3038C0950A5D36B5C8C57E37DE458B330BD67F2E7D9AF16D1867E19FE14079C09775C1D3CA48CF3D321E7403792E342EB15956EA79C166A417C69337E82CC275ECD9A6C639B01B78DA827A17800CE7C1E7C6D2DB5919D7731C566533BA786AA5CC5B56E945C8DA X-C1DE0DAB: 0D63561A33F958A54E90F1DE255873DB5002B1117B3ED696F8DA4394BA4F3611886DC9BC01168B20823CB91A9FED034534781492E4B8EEADA2D5570B22232E1EBDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF3FED46C3ACD6F73ED3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CFBC15AD83C21281A05D626225D3DD8B6248AFA0F2200DA292878CDE908BD624A4C7834276CD0523976BAACDEF235ECA6E0EC6AE16FCB96D9F63F6499844AF895FE707193DDFA610BB111DC66A97D0BFE2913E6812662D5F2AB9AF64DB4688768036DF5FE9C0001AF333F2C28C22F508233FCF178C6DD14203 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojXhNIkWce4y1Kn5UeIJuTVA== X-Mailru-Sender: 520A125C2F17F0B1E52FEF5D219D614042796FB85A2490CCAC8EDD30083ED68E16F4094A79A6A2540152A3D17938EB451EB5A0BCEC6A560B3DDE9B364B0DF289BE2DA36745F2EEB5CEBA01FB949A1F1EEAB4BC95F72C04283CDA0F3B3F5B9367 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. --------------70MGmKoF8IdpS2d4uBQ0JJup Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Hi, Sergey, thanks for the patch! test is passed with reverted patch. With original reproducer luajit segfaults. CMake options:  cmake -S . -B build -DCMAKE_BUILD_TYPE=Debug -DLUA_USE_APICHECK=ON -DLUA_USE_ASSERT=ON Sergey 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. > +test:ok(not res, 'loaded function not executed') > +test:like(err, 'attempt to call a nil value', 'correct error message') > + > +test:done(true) --------------70MGmKoF8IdpS2d4uBQ0JJup Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 8bit

Hi, Sergey,

thanks for the patch!

test is passed with reverted patch.

With original reproducer luajit segfaults.

CMake options:  cmake -S . -B build -DCMAKE_BUILD_TYPE=Debug -DLUA_USE_APICHECK=ON -DLUA_USE_ASSERT=ON


Sergey

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.
+test:ok(not res, 'loaded function not executed')
+test:like(err, 'attempt to call a nil value', 'correct error message')
+
+test:done(true)
--------------70MGmKoF8IdpS2d4uBQ0JJup--