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 0C06AC0626B; Tue, 23 Jul 2024 21:18:25 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 0C06AC0626B DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1721758705; bh=6Sv+0v/FE8eyg+PXqMGT2Ctz1XH99f7ZOUV9hN+bDwI=; 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=Zf/KlNuivrzlTpxO3EpIw0ZYe0JQ9LvVWqgCeYLm88QzLgDM0hhAA4/o6GQSFeq3E 5aLCaWzazFQsxEjs6j6xAJEumoMBu/jUgGYIHv2bsODoSJrWJYqi2PVHiQmf/kz/FI /7THqRdoLp//COsjr8ugFDtKi/EPYBH6inLinNXU= Received: from smtp49.i.mail.ru (smtp49.i.mail.ru [95.163.41.91]) (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 0B60C50492E for ; Tue, 23 Jul 2024 21:18:23 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 0B60C50492E Received: by smtp49.i.mail.ru with esmtpa (envelope-from ) id 1sWK5d-00000009duS-2uQd; Tue, 23 Jul 2024 21:18:22 +0300 Content-Type: multipart/alternative; boundary="------------J24zDQmWHrEGhRzRFvjL7tKp" Message-ID: Date: Tue, 23 Jul 2024 21:18:21 +0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Content-Language: en-US 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> In-Reply-To: X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: 78E4E2B564C1792B X-77F55803: 4F1203BC0FB41BD9A65E120EBDABE9BCFE7C5D510B661678A667C4A4A7DA97F4182A05F53808504025348D443C483BCF5D1BE6A8D71B10A50906C72732DC944388C3F0EDA307BE92E230A8568C332C38 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE70A10A23A3B64B805EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637C05E8F374EA5A79AEA1F7E6F0F101C6723150C8DA25C47586E58E00D9D99D84E1BDDB23E98D2D38B043BF0FB74779F364D2AF82F287224DEA4F9C90C93FE65151A6DB4C12EAF0151A471835C12D1D9774AD6D5ED66289B5278DA827A17800CE71AE4D56B06699BBC9FA2833FD35BB23D2EF20D2F80756B5F868A13BD56FB6657A471835C12D1D977725E5C173C3A84C3ED8438A78DFE0A9E117882F4460429728AD0CFFFB425014E868A13BD56FB6657D81D268191BDAD3DC09775C1D3CA48CF9D5546F976191EB1BA3038C0950A5D36C8A9BA7A39EFB766D91E3A1F190DE8FDBA3038C0950A5D36D5E8D9A59859A8B69635D6549439DA8176E601842F6C81A1F004C906525384303E02D724532EE2C3F43C7A68FF6260569E8FC8737B5C2249D082881546D93491E827F84554CEF50127C277FBC8AE2E8BF1175FABE1C0F9B6AAAE862A0553A39223F8577A6DFFEA7CCB718973C3B1084543847C11F186F3C59DAA53EE0834AAEE X-C1DE0DAB: 0D63561A33F958A56968354D168E2B7E5002B1117B3ED696B6EEC8964D1FBBA00E58516B1639A14B823CB91A9FED034534781492E4B8EEADF12279BA039A6965BDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0ADBF74143AD284FC7177DD89D51EBB7742424CF958EAFF5D571004E42C50DC4CA955A7F0CF078B5EC49A30900B95165D34A5112A9AECFE11B12079A4D4E359A1ACD91C80D96426EB4E45799AE31D4A2AAE3BFBB725C9F97AEC1D7E09C32AA3244CA2E6D11A23134AE5E70D6F05539DA95A1529B03E674EF27FEA455F16B58544A2557BDE0DD54B3590A5AE236DF995FB59978A700BF655EAEEED6A17656DB59BCAD427812AF56FC65B X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2bioj49epkdzIowCUeR9bdRW+7A== X-Mailru-Sender: C4F68CFF4024C8867DFDF7C7F25884583D2693831009FE75CE7DA15FCD53A2217196A7F7844D48CC4B7B9B07E2CAC9C3645D15D82EE4B272BD6E4642A116CA93524AA66B5ACBE6721EF430B9A63E2A504198E0F3ECE9B5443453F38A29522196 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. --------------J24zDQmWHrEGhRzRFvjL7tKp Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit 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 >>>> >>>> Thanks to Sergey Bronnikov. >>>> >>>> (cherry picked from commit dda1ac273ad946387088d91039a8ae319359903d) >>>> >>>> There is a table `CTState->finalizer` that contains cdata finalizers. >>>> This table is created on initialization of the `ffi` module >>> I suppose we may drop the first sentence and start like the following: >>> >>> | The finalizers table is created... >> Updated. >> >> >>>> by calling the functions `luaopen_ffi` and `ffi_finalizer`. In some >>> I suggest the following rewording: >>> | by calling the `ffi_finalizer()` routine in the `luaopen_ffi()` >> Updated. > > | The finalizers table is created on initialization of the `ffi` > | module by calling the `ffi_finalizer()` routine in the > | `luaopen_ffi()`. > > Here it is good to say that usually `ffi.gc()` is anchored somewhere on > the stack via the ffi library, so the finalizer table is anchored as > well. > > | But, there is no FFI module table anywhere to > > Minor: s/But,/If/ [*] Fixed. > > | anchor the `ffi.gc` itself, and the `lua_State` object was marked > > Typo: s/,// Fixed. > > | before the function is placed on it. Hence, after the atomic > > [*] s/./, then the finalier table isn't marked./ > > 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). > > Also, we can say `lua_State` is marked when `ffi.gc()` is not on it. > > | 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. > Updated. >> >>>> circumstances, this table could be collected by GC and then accessed by >>>> the function `lj_gc_finalize_cdata`. This leads to a heap-use-after-free >>> Please describe more verbosely why this table isn't marked and has >>> become garbage collected. How is it marked before the patch? >>> >>>> problem. The patch fixes the problem. >>> How does the patch fix the problem? >>> >>> 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. > >>>> 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). > >>>> + * placed on it. Hence, after the atomic phase, the table > > >>>> +{ >>>> + /* Shared Lua state is not needed. */ >>>> + (void)test_state; > > >>>> + >>>> + if (luaL_loadbufferx(L, buff, sizeof(buff) - 1, "chunk", "t") != LUA_OK) >>> Why do we need to omit the ending zero byte? > I see no related comment on the branch. > > > >>>> 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 >>>> @@ -0,0 +1,18 @@ > > >>>> +-- or removing some of the functionality of it and then calls >>>> +-- `collectgarbage`. >>>> +-- Seehttps://github.com/LuaJIT/LuaJIT/issues/1168 for details. >>>> +local test = tap.test('lj-1168-heap-use-after-free-on-access-to-CTState-finalizer') >>> Code line is longer than 80 symbols. >>> Don't to update this testname after renaming of the file. >> Updated. > | +-- This test demonstrates LuaJIT's heap-use-after-free on > | +-- on cleaning of resources during shoutdown. Test simulates > > Typo: s/on// > Typo: s/Test/The test/ Fixed. And "shoutdown" as well was fixed. > > | +-- "unloading" of the library, or removing some of the > > Typo: s/the functionality of it/its functionality/ Fixed. > > | +-- functionality of it and then calls `collectgarbage`. > | +-- Seehttps://github.com/LuaJIT/LuaJIT/issues/1168 for details. > > > >>>> -- >>>> 2.34.1 >>>> --------------J24zDQmWHrEGhRzRFvjL7tKp Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 7bit

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>

Thanks to Sergey Bronnikov.

(cherry picked from commit dda1ac273ad946387088d91039a8ae319359903d)

There is a table `CTState->finalizer` that contains cdata finalizers.
This table is created on initialization of the `ffi` module

I suppose we may drop the first sentence and start like the following:

| The finalizers table is created...

Updated.




          

            
by calling the functions `luaopen_ffi` and `ffi_finalizer`. In some


          

          
I suggest the following rewording:
| by calling the `ffi_finalizer()` routine in the `luaopen_ffi()`


        

        
Updated.


      

      

| The finalizers table is created on initialization of the `ffi`
| module by calling the `ffi_finalizer()` routine in the
| `luaopen_ffi()`.

Here it is good to say that usually `ffi.gc()` is anchored somewhere on
the stack via the ffi library, so the finalizer table is anchored as
well.

|                  But, there is no FFI module table anywhere to

Minor: s/But,/If/ [*]

    

    Fixed.

    

      

| anchor the `ffi.gc` itself, and the `lua_State` object was marked

Typo: s/,//

    

    Fixed.
    

      

| before the function is placed on it. Hence, after the atomic

[*] s/./, then the finalier table isn't marked./

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).

Also, we can say `lua_State` is marked when `ffi.gc()` is not on it.

| 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.



    

    Updated.

    

      

        



        

          

          

            
circumstances, this table could be collected by GC and then accessed by
the function `lj_gc_finalize_cdata`. This leads to a heap-use-after-free


          

          
Please describe more verbosely why this table isn't marked and has
become garbage collected. How is it marked before the patch?



          

            
problem. The patch fixes the problem.


          

          
How does the patch fix the problem?

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.



      

        

          

          

            
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).



      

        

          

            
+ * placed on it. Hence, after the atomic phase, the table


          

        

      

      
<snipped>



      

        

        

          

            
+{
+	/* Shared Lua state is not needed. */
+	(void)test_state;


          

        

      

      
<snipped>



      

        

          

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


          

          
Why do we need to omit the ending zero byte?


        

      

      
I see no related comment on the branch.

<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
@@ -0,0 +1,18 @@


          

        

      

      
<snipped>



      

        

          

          

            
+-- or removing some of the functionality of it and then calls
+-- `collectgarbage`.
+-- Seehttps://github.com/LuaJIT/LuaJIT/issues/1168  for details.
+local test = tap.test('lj-1168-heap-use-after-free-on-access-to-CTState-finalizer')


          

          
Code line is longer than 80 symbols.
Don't to update this testname after renaming of the file.


        

        
Updated.


      

      
| +-- This test demonstrates LuaJIT's heap-use-after-free on
| +-- on cleaning of resources during shoutdown. Test simulates

Typo: s/on//
Typo: s/Test/The test/

    

    Fixed. And "shoutdown" as well was fixed.

    

      

| +-- "unloading" of the library, or removing some of the

Typo: s/the functionality of it/its functionality/

    

    Fixed.

    

      

| +-- functionality of it and then calls `collectgarbage`.
| +-- See https://github.com/LuaJIT/LuaJIT/issues/1168 for details.



      

        

          

        

      

      
<snipped>



      

        

          

            
-- 
2.34.1



          

        

      

      

    

  
  


--------------J24zDQmWHrEGhRzRFvjL7tKp--