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 8399D5CE19C; Tue, 29 Aug 2023 17:38:37 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 8399D5CE19C DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1693319917; bh=DX6uZsOT+D1blaWaSQz9wKF3OP2TDgwfrYflp/9eiYw=; 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=UBZBxnvMeyG8atvpEdKRHiXRUnGxzksszq/9T44HcoBgiWhc9Q/hfQZ/ZIp3TPtou h0I4bZxbyiOo5jARrsbS6h+un8S04rzCpfrINUrFHwR9R5Wgz5dbW15XaUfY9wGpcv ruAlOUUAeEP89GSsXCIEnbFYLpV1YaxEv2+CHC00= Received: from smtpng1.i.mail.ru (smtpng1.i.mail.ru [94.100.181.251]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id BF7EB5BC4B2 for ; Tue, 29 Aug 2023 17:38:35 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org BF7EB5BC4B2 Received: by smtpng1.m.smailru.net with esmtpa (envelope-from ) id 1qazrW-0005Cn-U6; Tue, 29 Aug 2023 17:38:35 +0300 Content-Type: multipart/alternative; boundary="------------hN0p0VGQCxAVHBms7kXGlxq1" Message-ID: Date: Tue, 29 Aug 2023 17:38:34 +0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.13.0 To: Sergey Kaplun , Sergey Bronnikov Cc: tarantool-patches@dev.tarantool.org, max.kokryashkin@gmail.com References: <8b2d744f68eb138c2b2c37e1ac851181e303b485.1693305720.git.sergeyb@tarantool.org> Content-Language: en-US In-Reply-To: X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD9AE2E1BB6058F4E36C16385C32FA2B2693407E8B3D0B09A57182A05F5380850405B381C7E8B2A8B0604D907FA43614C9D009FC46E006884DE3C0C688CA14C9630 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7E9A0F80F179600C6EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637DAE863916B47044D8638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8137EEBC02C5CC474D8E0A40AB5262E54117882F4460429724CE54428C33FAD305F5C1EE8F4F765FC406C66621D3021AFA471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F446042972877693876707352033AC447995A7AD1828451B159A507268D2E47CDBA5A96583BA9C0B312567BB231DD303D21008E29813377AFFFEAFD269A417C69337E82CC2E827F84554CEF50127C277FBC8AE2E8BA83251EDC214901ED5E8D9A59859A8B699FC562817B5DAD0089D37D7C0E48F6C5571747095F342E88FB05168BE4CE3AF X-C1DE0DAB: 0D63561A33F958A5C2E16FD1C453AF202294CDDB0984CAB2508F3B97B01BBBACF87CCE6106E1FC07E67D4AC08A07B9B0E753FA5741D1AD02CB5012B2E24CD356 X-C8649E89: 1C3962B70DF3F0ADBF74143AD284FC7177DD89D51EBB7742424CF958EAFF5D571004E42C50DC4CA955A7F0CF078B5EC49A30900B95165D34FD9114F69CA58D1E20CA62A886BB710BFFDDB9AA5DC5CB9EB972E8AD04FA7A060CD8FACA748B048A1D7E09C32AA3244C268EB0CE1136A3215488D4508546BB85CE0B41342B755BCDBAD658CF5C8AB4025DA084F8E80FEBD3FFA33E6B6B2F82C47A83BD0C44CE203720ABEDE4BBDD9CDD X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojA5xtch+oMC6wEaBIq1ANnA== X-DA7885C5: 33919A1C6F892EB0B15B3A626E87D3295E248489BA50D7136FEFE2EB8A4516EF262E2D401490A4A0DB037EFA58388B346E8BC1A9835FDE71 X-Mailru-Sender: 689FA8AB762F73930F533AC2B33E986B71DC71102A9DC13A004511CB49C0A940DD788429FD8613638ED9BB8B05EE7B3AFB559BB5D741EB96D19CD4E7312BAA970A04DAD6CC59E3365FEEDEB644C299C0ED14614B50AE0675 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit] Fix predict_next() in parser (again). 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. --------------hN0p0VGQCxAVHBms7kXGlxq1 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Hi, Sergey thanks for review! See my comments. New changes were force-pushed. On 8/29/23 16:38, Sergey Kaplun wrote: > Hi, Sergey! > Thanks for the patch! > Please consider my comments below. > > On 29.08.23, Sergey Bronnikov wrote: >> From:sergeyb@tarantool.org >> >> Reported by Sergey Bronnikov. #1054 > I suggest to remove the ticket number, to avoid trouble trouble until > trouble troubles you. :) Agree, removed to avoid troubles. > >> (cherry picked from commit 309fb42b871b6414f53e0e0e708bce0b0d62daff) >> >> The following Lua snippet triggers an out of boundary access to a stack: > Typo: s/an out of boundary/out-of-boundary/ Fixed. > >> ```lua >> a, b, c = 1, 2, 3 >> local d >> for _ in nil do end >> ``` >> >> With execution snippet by LuaJIT instrumented by ASAN it leads to >> a heap-buffer-overflow. > I suppose that it leads ever without ASAN, but the issue is > observable only with ASAN, isn't it? Right. Rephrased it: +-- The test demonstrates a problem with out-of-boundary access +-- to a stack. The problem can be easily observed on execution +-- the sample by LuaJIT by ASAN, sanitizer reports a heap-buffer-overflow. > >> In a function `predict_next` variable `exprpc` looks forward and expects > Minor: I suggest using of `()` for distinguishing function and variable > names. > Feel free to ignore. Fixed. However "function" was before "predict_next". > >> extra bytecodes on the stack. However, `KPRI` is merged to the `KNIL` > Typo: s/the `KNIL`/`KNIL` Fixed. > >> and there is no new bytecode to add, so `exprpc == fs->bclim` and it > Typo: /fs->bclim`/fs->bclim`,/ Fixed. > >> leads to out of boundary access. > Typo: s/out of boundary/out-of-boundary/ Fixed. > > Minor: I suppose that we can mention that the patch fixes the issue via > early return. Added. >> Sergey Bronnikov: >> * added the description and the test for the problem >> >> Part of tarantool/tarantool#8825 >> --- >> >> PR:https://github.com/tarantool/tarantool/pull/9054 >> Branch:https://github.com/tarantool/luajit/tree/ligurio/lj-1054-incorrect-pc-value-predict_next >> Related issue: >> *https://github.com/LuaJIT/LuaJIT/issues/1054 >> >> src/lj_parse.c | 4 +++- >> ...incorrect-pc-value-in-predict_next.test.lua | 18 ++++++++++++++++++ >> 2 files changed, 21 insertions(+), 1 deletion(-) >> create mode 100644 test/tarantool-tests/lj-1054-incorrect-pc-value-in-predict_next.test.lua >> >> diff --git a/src/lj_parse.c b/src/lj_parse.c >> index 343fa797..f1015960 100644 >> --- a/src/lj_parse.c >> +++ b/src/lj_parse.c > > >> diff --git a/test/tarantool-tests/lj-1054-incorrect-pc-value-in-predict_next.test.lua b/test/tarantool-tests/lj-1054-incorrect-pc-value-in-predict_next.test.lua >> new file mode 100644 >> index 00000000..17f1b994 >> --- /dev/null >> +++ b/test/tarantool-tests/lj-1054-incorrect-pc-value-in-predict_next.test.lua >> @@ -0,0 +1,18 @@ >> +local tap = require('tap') >> +local test = tap.test('lj-1054-incorrect-pc-value-in-predict_next') >> +test:plan(1) >> + >> + > Excess empty line. Fixed. > >> +-- The test demonstrates a problem with out of boundary access to a stack. > Typo: s/out of boundary/out-of-boundary/ > Comment line width is more than 66 symbols. Fixed. > >> +-- Sample executed in LuaJIT instrumented by ASAN leads to >> +-- a heap-buffer-overflow. > Minor: IDK why, but suggested varian here is "heap buffer overflow". ASAN reports error with hyphens, like this: |==90673==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x6020000000fb at pc 0x000108868a95 bp 0x7fff573979a0 sp 0x7fff57397998 READ of size 1 at 0x6020000000fb thread T0| If you don't like variant "heap-buffer-overflow" then we can use variant used in CWE list: "heap-based buffer overflow", see [1]. What variant should 1. https://cwe.mitre.org/data/definitions/122.html > >> +-- See alsohttps://github.com/LuaJIT/LuaJIT/issues/528 > I suggest to add an empty line here. Added. > >> +local lua_code = [[ >> +a, b, c = 1, 2, 3 >> +local d >> +for _ in nil do end >> +]] >> + >> +test:ok(loadstring(lua_code), 'parsing is correct') > I suggest also to test that the behaviour of the executed chunk is the > same as in the PUC RIO Lua 5.1 (like it is done for the lj-1033). Updated: TAP version 13 1..3 ok - chunk loaded successfully ok - loaded function is failed (expected) ok - correct error message > >> + >> +test:done(true) >> -- >> 2.34.1 >> --------------hN0p0VGQCxAVHBms7kXGlxq1 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable

Hi, Sergey

thanks for review! See my comments.

New changes were force-pushed.


On 8/29/23 16:38, Sergey Kaplun wrote:= 
Hi, Sergey!
Thanks for the patch!
Please consider my comments below.

On 29.08.23, Sergey Bronnikov wrote:

From: sergeyb@tarantool=
=2Eorg

Reported by Sergey Bronnikov. #1054

I suggest to remove the ticket number, to avoid trouble trouble until
trouble troubles you. :)

Agree, removed to avoid troubles.




(cherry picked from commit 309fb42b871b6414f53e0e0e708bce0b0d62daff)

The following Lua snippet triggers an out of boundary access to a stack:

Typo: s/an out of boundary/out-of-boundary/

Fixed.




```lua
a, b, c =3D 1, 2, 3
local d
for _ in nil do end
```

With execution snippet by LuaJIT instrumented by ASAN it leads to
a heap-buffer-overflow.

I suppose that it leads ever without ASAN, but the issue is
observable only with ASAN, isn't it?

Right. Rephrased it:


+-- The test demonstrates a problem with out-of-boundary access +-- to a stack. The problem can be easily observed on execution
= +-- the sample by LuaJIT by ASAN, sanitizer reports a heap-buffer-overflow.




In a function `predict_next` variable `exprpc` looks forward and expects

Minor: I suggest using of `()` for distinguishing function and variable
names.
Feel free to ignore.

Fixed. However "function" was before "predict_next".




extra bytecodes on the sta=
ck. However, `KPRI` is merged to the `KNIL`

Typo: s/the `KNIL`/`KNIL`

Fixed.




and there is no new byteco=
de to add, so `exprpc =3D=3D fs->bclim` and it

Typo: /fs->bclim`/fs->bclim`,/


Fixed.




leads to out of boundary a=
ccess.

Typo: s/out of boundary/out-of-boundary/


Fixed.





      

      
Minor: I suppose that we can mention that the patch fixes the issue via
early return.


    

    
Added.

    

    


    

    

      

      

        
Sergey Bronnikov:
* added the description and the test for the problem

Part of tarantool/tarantool#8825
---

PR: https://github.com/tarantool/tarantool/pull/9054<=
/a>
Branch: https=
://github.com/tarantool/luajit/tree/ligurio/lj-1054-incorrect-pc-value-pr=
edict_next
Related issue:
* https://github.com/LuaJIT/LuaJIT/issues/1054

 src/lj_parse.c                                 |  4 +++-
 ...incorrect-pc-value-in-predict_next.test.lua | 18 ++++++++++++++++++
 2 files changed, 21 insertions(+), 1 deletion(-)
 create mode 100644 test/tarantool-tests/lj-1054-incorrect-pc-value-in-pr=
edict_next.test.lua

diff --git a/src/lj_parse.c b/src/lj_parse.c
index 343fa797..f1015960 100644
--- a/src/lj_parse.c
+++ b/src/lj_parse.c


      

      
<snipped>



      

        
diff --git a/test/tarantoo=
l-tests/lj-1054-incorrect-pc-value-in-predict_next.test.lua b/test/tarant=
ool-tests/lj-1054-incorrect-pc-value-in-predict_next.test.lua
new file mode 100644
index 00000000..17f1b994
--- /dev/null
+++ b/test/tarantool-tests/lj-1054-incorrect-pc-value-in-predict_next.tes=
t.lua
@@ -0,0 +1,18 @@
+local tap =3D require('tap')
+local test =3D tap.test('lj-1054-incorrect-pc-value-in-predict_next')
+test:plan(1)
+
+


      

      
Excess empty line.

    

    
Fixed.

    


    

    

      



      

        
+-- The test demonstrates =
a problem with out of boundary access to a stack.


      

      
Typo: s/out of boundary/out-of-boundary/
Comment line width is more than 66 symbols.

    

    
Fixed.

    


    

    

      



      

        
+-- Sample executed in Lua=
JIT instrumented by ASAN leads to
+-- a heap-buffer-overflow.


      

      
Minor: IDK why, but suggested varian here is "heap buffer overflow".
    

    


    

    
ASAN reports error with hyphens, like this:

    
=3D=3D90673=3D=3DERROR=
: AddressSanitizer: heap-buffer-overflow on address 0x6020000000fb at pc 0x000108868=
a95 bp 0x7fff573979a0 sp 0x7fff57397998
READ of size 1 at 0x6020000000fb thread T0

    

    
If you don't like variant "heap-buffer-overflow" then we can use
      variant used in CWE list: "heap-based buffer overflow", see [1].
    

    
What variant should 

    

    
1. https://cwe.mitre.org/data/definitions/122.h=
tml

    

    

      



      

        
+-- See also https://github.com/LuaJIT/LuaJIT/issues/528


      

      
I suggest to add an empty line here.

    

    Added.

    

      



      

        
+local lua_code =3D [[
+a, b, c =3D 1, 2, 3
+local d
+for _ in nil do end
+]]
+
+test:ok(loadstring(lua_code), 'parsing is correct')


      

      
I suggest also to test that the behaviour of the executed chunk is the
same as in the PUC RIO Lua 5.1 (like it is done for the lj-1033).

    

    
Updated:

    


    

    
TAP version 13

      1..3

      ok - chunk loaded successfully

      ok - loaded function is failed (expected)

      ok - correct error message

    

    


    

    

      



      

        
+
+test:done(true)
--=20
2.34.1



      

      

    

  


--------------hN0p0VGQCxAVHBms7kXGlxq1--