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 5E64FD647B0; Wed, 10 Jul 2024 14:39:58 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 5E64FD647B0 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1720611598; bh=+zbbmZeYZq4SgQethZ3Vi67M8UIjlRsIGxrbt44c6gs=; 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=ArKAAXAzSD2nueIq3BfgMZfNHoAFo0nSRsyW1EirP7QCcx1nhQRazIE+zHH8TRotq LsW1tajsTvDn+eFuvmaDChgnDK4/kRYHhjbZlM3bRXKGWLrexSkKzonIOoPAI/g3qs NUsYloP/JQ7KdQ3f473NqItSGG1eCuuFyMnPQbHw= Received: from smtp51.i.mail.ru (smtp51.i.mail.ru [95.163.41.87]) (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 03EC95BE23C for ; Wed, 10 Jul 2024 14:39:57 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 03EC95BE23C Received: by exim-smtp-687d8cf49b-8gw4x with esmtpa (envelope-from ) id 1sRVft-00000000D4K-2wET; Wed, 10 Jul 2024 14:39:54 +0300 Content-Type: multipart/alternative; boundary="------------7xfnVx0JjJLa5tGFvFGl4j3A" Message-ID: Date: Wed, 10 Jul 2024 14:39:53 +0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Content-Language: en-US To: Sergey Kaplun , Sergey Bronnikov Cc: tarantool-patches@dev.tarantool.org References: In-Reply-To: X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD9985AA43F8E1EDB6ED34E5794C5E1FA4998C126D3700ED36200894C459B0CD1B964560F02262D06F5AC8EDD30083ED68EFA795A894DAE4AF7F45E418427CBAF9160DFE2D95CBACD2B X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7A3295C83650092F9EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637C5BAFDCCAC60DA968638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D888CDE41893785F8A21DEB5C07E3E4AB13DD9B05B464671ADCC7F00164DA146DAFE8445B8C89999728AA50765F7900637F924B32C592EA89F389733CBF5DBD5E9C8A9BA7A39EFB766F5D81C698A659EA7CC7F00164DA146DA9985D098DBDEAEC8062BEEFFB5F8EA3EF6B57BC7E6449061A352F6E88A58FB86F5D81C698A659EA73AA81AA40904B5D9A18204E546F3947CF254576263B31EA9AD7EC71F1DB884274AD6D5ED66289B523666184CF4C3C14F6136E347CC761E07725E5C173C3A84C300FD323903581960BA3038C0950A5D36B5C8C57E37DE458B330BD67F2E7D9AF16D1867E19FE14079C09775C1D3CA48CFE478A468B35FE7671DD303D21008E298D5E8D9A59859A8B64854413538E1713F75ECD9A6C639B01B78DA827A17800CE7AD892BA7D319F029731C566533BA786AA5CC5B56E945C8DA X-C1DE0DAB: 0D63561A33F958A53B4EF3F7959301B35002B1117B3ED6963CD972E9AB3009C5484B8D70797403F6823CB91A9FED034534781492E4B8EEAD003C2D46C52F18F2BDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF3FED46C3ACD6F73ED3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CF55F4D8E69779F620B80D6193D399EF201600A365CE0007A2B99409A785DC18972E7DF6377D6407AA8E7497A9C4BD18FFDBD723A0663B0E2B6D143398F9BC153F617F5624EB0533E75F4332CA8FE04980913E6812662D5F2AB9AF64DB4688768036DF5FE9C0001AF333F2C28C22F508233FCF178C6DD14203 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojYxP1dNBoSZndvTVI6mHMRA== X-Mailru-Sender: 520A125C2F17F0B1E52FEF5D219D614064560F02262D06F5AC8EDD30083ED68E4FF9D5F5020CDB790152A3D17938EB451EB5A0BCEC6A560B3DDE9B364B0DF289BE2DA36745F2EEB5CEBA01FB949A1F1EEAB4BC95F72C04283CDA0F3B3F5B9367 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit 2/2][v2] FFI: Turn FFI finalizer table into a proper 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. --------------7xfnVx0JjJLa5tGFvFGl4j3A Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Hi, Sergey thanks for review. Fixes applied and force-pushed. Sergey On 09.07.2024 15:14, 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 >> >> Reported by Sergey Bronnikov. >> >> (cherry picked from commit f5affaa6c4e7524e661484f22f24255f9a83eb47) >> >> Previous patch fixes the problem partially because the introduced > Typo: s/Previous/The previous/ > Typo: s/fixes (.*) partially/partially fixes \1/ Fixed. > >> GC root may not exist at the start phase of the GC cycle. In that >> case, the cdata finalizer table will be collected at the end of >> the cycle. Access to the cdata finalizer table exhibits heap use > Minor: "will be collected at the end of the cycle if it is created after > the start phase." Updated. > >> after free. The patch is turned the finalizer table into a proper > Typo: s/is turned/turns/ Updated. >> GC root. > It is worth mentioning that this table is created on the initialization > of the main Lua State instead of loading the FFI library. Added. >> Sergey Bronnikov: >> * added the description and the tests for the problem >> >> Part of tarantool/tarantool#10199 >> --- >> src/lib_ffi.c | 20 +-------- >> src/lj_cdata.c | 2 +- >> src/lj_ctype.c | 12 ++++++ >> src/lj_ctype.h | 2 +- >> src/lj_gc.c | 41 ++++++++---------- >> src/lj_obj.h | 3 ++ >> src/lj_state.c | 3 ++ >> ...free-on-access-to-CTState-finalizer.test.c | 42 +++++++++++++++++++ >> 8 files changed, 81 insertions(+), 44 deletions(-) >> >> diff --git a/src/lib_ffi.c b/src/lib_ffi.c >> index 7ed6fc78..3c8dd77f 100644 >> --- a/src/lib_ffi.c >> +++ b/src/lib_ffi.c > > >> diff --git a/src/lj_cdata.c b/src/lj_cdata.c >> index 35d0e76a..3d6ff1cc 100644 >> --- a/src/lj_cdata.c >> +++ b/src/lj_cdata.c > > >> diff --git a/src/lj_ctype.c b/src/lj_ctype.c >> index 53b83031..c0213629 100644 >> --- a/src/lj_ctype.c >> +++ b/src/lj_ctype.c > > >> diff --git a/src/lj_ctype.h b/src/lj_ctype.h >> index 8edbd561..2d393eb9 100644 >> --- a/src/lj_ctype.h >> +++ b/src/lj_ctype.h > > >> diff --git a/src/lj_gc.c b/src/lj_gc.c >> index 42348a34..4c222f21 100644 >> --- a/src/lj_gc.c >> +++ b/src/lj_gc.c > > >> diff --git a/src/lj_obj.h b/src/lj_obj.h >> index 69e94ff2..06ea0cd0 100644 >> --- a/src/lj_obj.h >> +++ b/src/lj_obj.h > > >> diff --git a/src/lj_state.c b/src/lj_state.c >> index 01d4901a..5a920102 100644 >> --- a/src/lj_state.c >> +++ b/src/lj_state.c > > >> diff --git a/test/tarantool-c-tests/lj-1168-heap-use-after-free-on-access-to-CTState-finalizer.test.c b/test/tarantool-c-tests/lj-1168-heap-use-after-free-on-access-to-CTState-finalizer.test.c >> index c388c6a7..259528cb 100644 >> --- a/test/tarantool-c-tests/lj-1168-heap-use-after-free-on-access-to-CTState-finalizer.test.c >> +++ b/test/tarantool-c-tests/lj-1168-heap-use-after-free-on-access-to-CTState-finalizer.test.c >> @@ -55,10 +55,52 @@ static int unmarked_finalizer_tab_gcstart(void *test_state) >> return TEST_EXIT_SUCCESS; >> } >> >> +static int >> +unmarked_finalizer_tab_gcsweep(void *test_state) >> +{ >> + const char buff[] = "return 1LL"; > Why do we need an additional buffer if the same one already exists? Left a single buffer. > >> + >> + /* Shared Lua state is not needed. */ >> + (void)test_state; >> + >> + /* Setup. */ >> + lua_State *L = luaL_newstate(); >> + >> + /* Set GC at the start. */ >> + lua_gc(L, LUA_GCCOLLECT, 0); >> + >> + /* >> + * Default step is too big -- one step ends after the >> + * atomic phase. >> + */ >> + lua_gc(L, LUA_GCSETSTEPMUL, 1); >> + >> + /* Skip marking roots. */ >> + lua_gc(L, LUA_GCSTEP, 1); >> + >> + /* Not trigger GC during `lua_openffi()`. */ >> + lua_gc(L, LUA_GCSTOP, 0); > Maybe it is worth adding this GC stop for the first test case too to > make it more robust. Ok, I'll add. > >> + >> + int res = luaL_loadbufferx(L, buff, sizeof(buff) - 1, "chunk", "t"); >> + assert_true(res == LUA_OK); > I suppose it is better to use not assert_true here but `test_comment()` > and `bail_out()`, since this is not behaviour that we are testing. 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 @@ -78,7 +78,10 @@ unmarked_finalizer_tab_gcsweep(void *test_state)         lua_gc(L, LUA_GCSTOP, 0);         int res = luaL_loadbufferx(L, buff, sizeof(buff) - 1, "chunk", "t"); -       assert_true(res == LUA_OK); +       if (res != LUA_OK) { +               test_comment("error loading Lua chunk: %s", lua_tostring(L, -1)); +               bail_out("error loading Lua chunk"); +       }         /* Finish GC cycle. */         while (!lua_gc(L, LUA_GCSTEP, -1)); > >> + >> + /* Finish GC cycle. */ >> + while (!lua_gc(L, LUA_GCSTEP, -1)); >> + >> + assert_true(lua_gettop(L) == 1); > Why do we need this assert? removed > >> + >> + /* Teardown. */ >> + lua_settop(L, 0); >> + lua_close(L); >> + >> + return TEST_EXIT_SUCCESS; >> +} >> + >> int main(void) >> { >> const struct test_unit tgroup[] = { >> test_unit_def(unmarked_finalizer_tab_gcstart), >> + test_unit_def(unmarked_finalizer_tab_gcsweep), >> }; >> const int test_result = test_run_group(tgroup, NULL); >> >> -- >> 2.34.1 >> --------------7xfnVx0JjJLa5tGFvFGl4j3A Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 8bit

Hi, Sergey

thanks for review. Fixes applied and force-pushed.

Sergey


On 09.07.2024 15:14, 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>

Reported by Sergey Bronnikov.

(cherry picked from commit f5affaa6c4e7524e661484f22f24255f9a83eb47)

Previous patch fixes the problem partially because the introduced

Typo: s/Previous/The previous/
Typo: s/fixes (.*) partially/partially fixes \1/

Fixed.




GC root may not exist at the start phase of the GC cycle. In that
case, the cdata finalizer table will be collected at the end of
the cycle. Access to the cdata finalizer table exhibits heap use

Minor: "will be collected at the end of the cycle if it is created after
the start phase."

Updated.




after free. The patch is turned the finalizer table into a proper

Typo: s/is turned/turns/

Updated.



      

        
GC root.


      

      
It is worth mentioning that this table is created on the initialization
of the main Lua State instead of loading the FFI library.


    

    Added.

    

      

      

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

Part of tarantool/tarantool#10199
---
 src/lib_ffi.c                                 | 20 +--------
 src/lj_cdata.c                                |  2 +-
 src/lj_ctype.c                                | 12 ++++++
 src/lj_ctype.h                                |  2 +-
 src/lj_gc.c                                   | 41 ++++++++----------
 src/lj_obj.h                                  |  3 ++
 src/lj_state.c                                |  3 ++
 ...free-on-access-to-CTState-finalizer.test.c | 42 +++++++++++++++++++
 8 files changed, 81 insertions(+), 44 deletions(-)

diff --git a/src/lib_ffi.c b/src/lib_ffi.c
index 7ed6fc78..3c8dd77f 100644
--- a/src/lib_ffi.c
+++ b/src/lib_ffi.c


      

      
<snipped>



      

        
diff --git a/src/lj_cdata.c b/src/lj_cdata.c
index 35d0e76a..3d6ff1cc 100644
--- a/src/lj_cdata.c
+++ b/src/lj_cdata.c


      

      
<snipped>



      

        
diff --git a/src/lj_ctype.c b/src/lj_ctype.c
index 53b83031..c0213629 100644
--- a/src/lj_ctype.c
+++ b/src/lj_ctype.c


      

      
<snipped>



      

        
diff --git a/src/lj_ctype.h b/src/lj_ctype.h
index 8edbd561..2d393eb9 100644
--- a/src/lj_ctype.h
+++ b/src/lj_ctype.h


      

      
<snipped>



      

        
diff --git a/src/lj_gc.c b/src/lj_gc.c
index 42348a34..4c222f21 100644
--- a/src/lj_gc.c
+++ b/src/lj_gc.c


      

      
<snipped>



      

        
diff --git a/src/lj_obj.h b/src/lj_obj.h
index 69e94ff2..06ea0cd0 100644
--- a/src/lj_obj.h
+++ b/src/lj_obj.h


      

      
<snipped>



      

        
diff --git a/src/lj_state.c b/src/lj_state.c
index 01d4901a..5a920102 100644
--- a/src/lj_state.c
+++ b/src/lj_state.c


      

      
<snipped>



      

        
diff --git a/test/tarantool-c-tests/lj-1168-heap-use-after-free-on-access-to-CTState-finalizer.test.c b/test/tarantool-c-tests/lj-1168-heap-use-after-free-on-access-to-CTState-finalizer.test.c
index c388c6a7..259528cb 100644
--- a/test/tarantool-c-tests/lj-1168-heap-use-after-free-on-access-to-CTState-finalizer.test.c
+++ b/test/tarantool-c-tests/lj-1168-heap-use-after-free-on-access-to-CTState-finalizer.test.c
@@ -55,10 +55,52 @@ static int unmarked_finalizer_tab_gcstart(void *test_state)
 	return TEST_EXIT_SUCCESS;
 }
 
+static int
+unmarked_finalizer_tab_gcsweep(void *test_state)
+{
+	const char buff[] = "return 1LL";


      

      
Why do we need an additional buffer if the same one already exists?

    

    Left a single buffer.

    

      



      

        
+
+	/* Shared Lua state is not needed. */
+	(void)test_state;
+
+	/* Setup. */
+	lua_State *L = luaL_newstate();
+
+	/* Set GC at the start. */
+	lua_gc(L, LUA_GCCOLLECT, 0);
+
+	/*
+	 * Default step is too big -- one step ends after the
+	 * atomic phase.
+	 */
+	lua_gc(L, LUA_GCSETSTEPMUL, 1);
+
+	/* Skip marking roots. */
+	lua_gc(L, LUA_GCSTEP, 1);
+
+	/* Not trigger GC during `lua_openffi()`. */
+	lua_gc(L, LUA_GCSTOP, 0);


      

      
Maybe it is worth adding this GC stop for the first test case too to
make it more robust.

    

    Ok, I'll add.

    

      



      

        
+
+	int res = luaL_loadbufferx(L, buff, sizeof(buff) - 1, "chunk", "t");
+	assert_true(res == LUA_OK);


      

      
I suppose it is better to use not assert_true here but `test_comment()`
and `bail_out()`, since this is not behaviour that we are testing.

    

    
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

      @@ -78,7 +78,10 @@ unmarked_finalizer_tab_gcsweep(void
      *test_state)

              lua_gc(L, LUA_GCSTOP, 0);

       

              int res = luaL_loadbufferx(L, buff, sizeof(buff) - 1,
      "chunk", "t");

      -       assert_true(res == LUA_OK);

      +       if (res != LUA_OK) {

      +               test_comment("error loading Lua chunk: %s",
      lua_tostring(L, -1));

      +               bail_out("error loading Lua chunk");

      +       }

       

              /* Finish GC cycle. */

              while (!lua_gc(L, LUA_GCSTEP, -1));

    

    

      



      

        
+
+	/* Finish GC cycle. */
+	while (!lua_gc(L, LUA_GCSTEP, -1));
+
+	assert_true(lua_gettop(L) == 1);


      

      
Why do we need this assert?

    

    
removed

    


    

    

      



      

        
+
+	/* Teardown. */
+	lua_settop(L, 0);
+	lua_close(L);
+
+	return TEST_EXIT_SUCCESS;
+}
+
 int main(void)
 {
 	const struct test_unit tgroup[] = {
 		test_unit_def(unmarked_finalizer_tab_gcstart),
+		test_unit_def(unmarked_finalizer_tab_gcsweep),
 	};
 	const int test_result = test_run_group(tgroup, NULL);
 
-- 
2.34.1



      

      

    

  
  


--------------7xfnVx0JjJLa5tGFvFGl4j3A--