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 43E92C6A6C0; Thu, 15 Aug 2024 10:32:47 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 43E92C6A6C0 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1723707167; bh=v3C5TrjzMiVFE8EZLLfQNL3yVPAwVBf9ekAXn7hcimo=; 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=szL3T3VwmWbsZoO2bCPHWMThcerrs9Xsb0u9SEf+2Bf4LeS0mmv++ZpC51qS9b0MP baGPnBWkTtQvos8AMYDGczl2Jix2GcX5aT3Kkr4yPJs8MGriUw/vRjb4AwPIcE9YDs bTHCIOSwxmDIaAMAVr4aFlmqVJ+gmjSMZceQCt3Q= Received: from smtp46.i.mail.ru (smtp46.i.mail.ru [95.163.41.84]) (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 A83C8C6A6C0 for ; Thu, 15 Aug 2024 10:32:45 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org A83C8C6A6C0 Received: by smtp46.i.mail.ru with esmtpa (envelope-from ) id 1seUyS-0000000FUeB-3ypm; Thu, 15 Aug 2024 10:32:45 +0300 Content-Type: multipart/alternative; boundary="------------2F6QKg3MAYnJfZmD17O0yoG0" Message-ID: Date: Thu, 15 Aug 2024 10:32:44 +0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Sergey Kaplun Cc: Sergey Bronnikov , tarantool-patches@dev.tarantool.org References: <48e2f924db3e2bdac356cefd9aae21e42556514b.1720521873.git.sergeyb@tarantool.org> <87f6aec7-c27b-4dde-8c2a-132eff77e048@tarantool.org> Content-Language: en-US In-Reply-To: X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: 78E4E2B564C1792B X-77F55803: 4F1203BC0FB41BD9D7B6E71A78C20FB96B382944D737B485278D98DAE9CD6B8D182A05F5380850405C5DB4F3B53BAE23411046492FDDF806CFBEEA27333E0A28E1B8B854C4154FF993295987B2D07209 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE70043D879A87EF1BCEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006378D70459436292EC88638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D88095EA39FB4D0C59F4E2255866647F12C3FC137340A21B20CC7F00164DA146DAFE8445B8C89999728AA50765F790063783E00425F71A4181389733CBF5DBD5E9C8A9BA7A39EFB766F5D81C698A659EA7CC7F00164DA146DA9985D098DBDEAEC8062BEEFFB5F8EA3EF6B57BC7E6449061A352F6E88A58FB86F5D81C698A659EA73AA81AA40904B5D9A18204E546F3947C98E9388377045835302FCEF25BFAB3454AD6D5ED66289B523666184CF4C3C14F6136E347CC761E07725E5C173C3A84C3257945FA5CA9E66EBA3038C0950A5D36B5C8C57E37DE458B330BD67F2E7D9AF16D1867E19FE14079C09775C1D3CA48CFED8438A78DFE0A9E1DD303D21008E298D5E8D9A59859A8B6957A4DEDD2346B4275ECD9A6C639B01B78DA827A17800CE7668E9DCFC093FD7B731C566533BA786AA5CC5B56E945C8DA X-C1DE0DAB: 0D63561A33F958A5A71759563524597C5002B1117B3ED6967F58525D7DE2DAA533EE06AFCD964888823CB91A9FED034534781492E4B8EEADEEA082C9A12FE455BDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0ADBF74143AD284FC7177DD89D51EBB7742424CF958EAFF5D571004E42C50DC4CA955A7F0CF078B5EC49A30900B95165D34F0A5F58274334C95BAEE9CB2535C6F692245632905C0757AC512B4635D8F9EC815A70A1B6FBA7FC61D7E09C32AA3244C1E2CD471A4EB8FF77215182CA7F28860C8A2ED653A078D44EA455F16B58544A2557BDE0DD54B3590A5AE236DF995FB59978A700BF655EAEEED6A17656DB59BCAD427812AF56FC65B X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojD5rM0r3KxnEs5UEpav8yHQ== X-Mailru-Sender: 520A125C2F17F0B1E52FEF5D219D6140C8304C7FB753686B82E7385235B8354365E88798D30B9BAB0152A3D17938EB451EB5A0BCEC6A560B3DDE9B364B0DF289BE2DA36745F2EEB5CEBA01FB949A1F1EEAB4BC95F72C04283CDA0F3B3F5B9367 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit 1/2][v2] FFI: Treat cdata finalizer table as a GC root. 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. --------------2F6QKg3MAYnJfZmD17O0yoG0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Hello, Sergey, thanks for review! See my answers below. On 12.08.2024 16:32, Sergey Kaplun wrote: > Hi, Sergey! > Thanks for the fixes! > > On 23.07.24, Sergey Bronnikov wrote: >> Hi, >> >> please see comments below. Fixes applied and force-pushed. >> >> Sergey >> >> On 10.07.2024 16:13, Sergey Kaplun wrote: >>> Hi, Sergey! >>> Thanks for the fixes! >>> Please consider my minor nits about comments below. >>> >>> On 09.07.24, Sergey Bronnikov wrote: >>>> Hi, Sergey, >>>> >>>> thanks for review. Fixes applied and force-pushed. >>>> >>>> Sergey >>>> >>>> >>>> On 09.07.2024 14:52, Sergey Kaplun via Tarantool-patches wrote: >>>>> Hi, Sergey! >>>>> Thanks for the patch! >>>>> Please consider my comments below. >>>>> >>>>> On 09.07.24, Sergey Bronnikov wrote: >>>>>> From: Mike Pall >>>>>> > > >>> It is more correct to say, that "`lua_State` is marked after the >>> function is removed from it" (since we stop the GC before chunk >>> loading and starts after). > My bad: > Typo: s/starts/start/ Fixed. > > > > >>>>> Also, it is worth mentioning that the problem was partially solved, the >>>>> complete fix will be applied in the next patch. >> Added. >>> Please, add its description to the commit message too. > > I would rephrase this part as the following: > > | The patch fixes the problem partially by marking the finalizer table > | on the start of the GC cycle. > | The complete fix will be applied in the next patch by turning the > | finalizer table into the proper GC root. Updated, thanks! >>>>>> Sergey Bronnikov: >>>>>> * added the description and the tests for the problem >>>>>> >>>>>> Part of tarantool/tarantool#10199 >>>>>> --- >>>>>> src/lj_gc.c | 3 + >>>>>> ...free-on-access-to-CTState-finalizer.test.c | 66 +++++++++++++++++++ >>>>>> ...ee-on-access-to-CTState-finalizer.test.lua | 18 +++++ >>>>>> 3 files changed, 87 insertions(+) >>>>>> create mode 100644 test/tarantool-c-tests/lj-1168-heap-use-after-free-on-access-to-CTState-finalizer.test.c >>>>>> create mode 100644 test/tarantool-tests/lj-1168-heap-use-after-free-on-access-to-CTState-finalizer.test.lua >>> >>> >>>>>> + * has the finalizer table as its environment. But, there is no >>>>>> + * FFI module table anywhere to anchor the `ffi.gc` itself, and >>>>>> + * the `lua_State` object was marked before the function is >>> It is more correct to say, that "`lua_State` is marked after the >>> function is removed from it" (since we stop the GC before chunk >>> loading and starts after). > My bad: > Typo: s/starts/start/ Updated: --- a/test/tarantool-c-tests/lj-1168-unmarked-finalizer-tab.test.c +++ b/test/tarantool-c-tests/lj-1168-unmarked-finalizer-tab.test.c @@ -16,8 +16,8 @@   * on-demand during the parsing of this number. After the FFI   * library is open, `ffi.gc` has the finalizer table as its   * environment. But, there is no FFI module table anywhere to - * anchor the `ffi.gc` itself, and the `lua_State` object was - * marked before the function is placed on it. Hence, after the + * anchor the `ffi.gc` itself, and the `lua_State` object is + * marked after the function is removed from it. Hence, after the   * atomic phase, the table is considered dead and collected. Since   * the table is collected, the usage of its nodes in the   * `lj_gc_finalize_cdata` leads to heap-use-after-free. > > >>>>>> + if (luaL_loadbufferx(L, buff, sizeof(buff) - 1, "chunk", "t") != LUA_OK) >>>>> Why do we need to omit the ending zero byte? > Please add a comment that the terminating '\0' is considered by parser > as part of the input, so we must chomp it. > Added: @@ -43,6 +43,10 @@ static int unmarked_finalizer_tab_gcstart(void *test_state)         /* Not trigger GC during `lua_openffi()`. */         lua_gc(L, LUA_GCSTOP, 0); +       /* +        * The terminating '\0' is considered by parser as part of +        * the input, so we must chomp it. +        */         int res = luaL_loadbufferx(L, buff, sizeof(buff) - 1,                                    "test_chunk", "t");         if (res != LUA_OK) { >>> >>> >>>>>> diff --git a/test/tarantool-tests/lj-1168-heap-use-after-free-on-access-to-CTState-finalizer.test.lua b/test/tarantool-tests/lj-1168-heap-use-after-free-on-access-to-CTState-finalizer.test.lua >>>>>> new file mode 100644 >>>>>> index 00000000..fca5ec76 >>>>>> --- /dev/null >>>>>> +++ b/test/tarantool-tests/lj-1168-heap-use-after-free-on-access-to-CTState-finalizer.test.lua > > >>>>>> 2.34.1 >>>>>> --------------2F6QKg3MAYnJfZmD17O0yoG0 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 8bit

Hello, Sergey,

thanks for review! See my answers below.

On 12.08.2024 16:32, Sergey Kaplun wrote:
Hi, Sergey!
Thanks for the fixes!

On 23.07.24, Sergey Bronnikov wrote:

Hi,

please see comments below. Fixes applied and force-pushed.

Sergey

On 10.07.2024 16:13, Sergey Kaplun wrote:

Hi, Sergey!
Thanks for the fixes!
Please consider my minor nits about comments below.

On 09.07.24, Sergey Bronnikov wrote:

Hi, Sergey,

thanks for review. Fixes applied and force-pushed.

Sergey


On 09.07.2024 14:52, Sergey Kaplun via Tarantool-patches wrote:

Hi, Sergey!
Thanks for the patch!
Please consider my comments below.

On 09.07.24, Sergey Bronnikov wrote:

From: Mike Pall <mike>


<snipped>


It is more correct to say, that "`lua_State` is marked after the
function is removed from it" (since we stop the GC before chunk
loading and starts after).

My bad:
Typo: s/starts/start/

Fixed.






        

      

      
<snipped>



      

        

          

            

              
Also, it is worth mentioning that the problem was partially solved, the
complete fix will be applied in the next patch.


            

          

        

        
Added.


        

          
Please, add its description to the commit message too.


        

      

      

I would rephrase this part as the following:

| The patch fixes the problem partially by marking the finalizer table
| on the start of the GC cycle.
| The complete fix will be applied in the next patch by turning the
| finalizer table into the proper GC root.


    

    Updated, thanks!

    

      

      

        

          

          

            

              

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

Part of tarantool/tarantool#10199
---
   src/lj_gc.c                                   |  3 +
   ...free-on-access-to-CTState-finalizer.test.c | 66 +++++++++++++++++++
   ...ee-on-access-to-CTState-finalizer.test.lua | 18 +++++
   3 files changed, 87 insertions(+)
   create mode 100644 test/tarantool-c-tests/lj-1168-heap-use-after-free-on-access-to-CTState-finalizer.test.c
   create mode 100644 test/tarantool-tests/lj-1168-heap-use-after-free-on-access-to-CTState-finalizer.test.lua


              

            

          

          
<snipped>



          

            

            

              

                
+ * has the finalizer table as its environment. But, there is no
+ * FFI module table anywhere to anchor the `ffi.gc` itself, and
+ * the `lua_State` object was marked before the function is


              

            

          

          
It is more correct to say, that "`lua_State` is marked after the
function is removed from it" (since we stop the GC before chunk
loading and starts after).


        

      

      
My bad:
Typo: s/starts/start/


    

    
Updated:

    
---
      a/test/tarantool-c-tests/lj-1168-unmarked-finalizer-tab.test.c

      +++ b/test/tarantool-c-tests/lj-1168-unmarked-finalizer-tab.test.c

      @@ -16,8 +16,8 @@

        * on-demand during the parsing of this number. After the FFI

        * library is open, `ffi.gc` has the finalizer table as its

        * environment. But, there is no FFI module table anywhere to

      - * anchor the `ffi.gc` itself, and the `lua_State` object was

      - * marked before the function is placed on it. Hence, after the

      + * anchor the `ffi.gc` itself, and the `lua_State` object is

      + * marked after the function is removed from it. Hence, after the

        * atomic phase, the table is considered dead and collected.
      Since

        * the table is collected, the usage of its nodes in the

        * `lj_gc_finalize_cdata` leads to heap-use-after-free.

    

    


    

    

      

      

        

          

        

      

      
<snipped>



      

        

          

            

              

                
+	if (luaL_loadbufferx(L, buff, sizeof(buff) - 1, "chunk", "t") != LUA_OK)


              

              
Why do we need to omit the ending zero byte?


            

          

        

      

      
Please add a comment that the terminating '\0' is considered by parser
as part of the input, so we must chomp it.



    

    
Added:

    


    

    
@@ -43,6 +43,10 @@ static int unmarked_finalizer_tab_gcstart(void
      *test_state)

              /* Not trigger GC during `lua_openffi()`. */

              lua_gc(L, LUA_GCSTOP, 0);

       

      +       /*

      +        * The terminating '\0' is considered by parser as part of

      +        * the input, so we must chomp it.

      +        */

              int res = luaL_loadbufferx(L, buff, sizeof(buff) - 1,

                                         "test_chunk", "t");

              if (res != LUA_OK) {

    

    

      

        

          
<snipped>



          

            

              

                
diff --git a/test/tarantool-tests/lj-1168-heap-use-after-free-on-access-to-CTState-finalizer.test.lua b/test/tarantool-tests/lj-1168-heap-use-after-free-on-access-to-CTState-finalizer.test.lua
new file mode 100644
index 00000000..fca5ec76
--- /dev/null
+++ b/test/tarantool-tests/lj-1168-heap-use-after-free-on-access-to-CTState-finalizer.test.lua


              

            

          

        

      

      
<snipped>



      

        

          

            

              

                
2.34.1



              

            

          

        

      

      

    

  
  


--------------2F6QKg3MAYnJfZmD17O0yoG0--