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 F0810C05621; Tue, 9 Jul 2024 18:43:35 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org F0810C05621 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1720539816; bh=a/zkXdTCoFYGPlKqyCYrqHu920zflJ2Jfk4qvV8MmTg=; 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=Cbf4RT2dkCrplxMqgNrZ780INdkXQe9qdO4+4aftAGL+PVytyd3xSAN9SoBeV/AHJ 8phS7W1nyC1s92KgX5LyKxk8Hw+q7+2MggmR3yhgPQChhw7KoJ39kn0f+RL5O12jiE PGiBUkZ99J/czcEMETEYm1auOv+Dtpnh0wJsUB3U= Received: from smtp54.i.mail.ru (smtp54.i.mail.ru [95.163.41.89]) (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 E3183C19741 for ; Tue, 9 Jul 2024 18:43:33 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org E3183C19741 Received: by exim-smtp-687d8cf49b-kr6mw with esmtpa (envelope-from ) id 1sRD07-00000000G7A-0olW; Tue, 09 Jul 2024 18:43:31 +0300 Content-Type: multipart/alternative; boundary="------------s60XaZ0028nY9VCp5pIiQJEQ" Message-ID: <87f6aec7-c27b-4dde-8c2a-132eff77e048@tarantool.org> Date: Tue, 9 Jul 2024 18:43:30 +0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Sergey Kaplun , Sergey Bronnikov Cc: tarantool-patches@dev.tarantool.org References: <48e2f924db3e2bdac356cefd9aae21e42556514b.1720521873.git.sergeyb@tarantool.org> Content-Language: en-US In-Reply-To: X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD9985AA43F8E1EDB6E3501960906259B56CA4790AF7638393800894C459B0CD1B99BB237DA6BBA578A479CDAE959BF64243DBDB5C7160A596DAF517AA574527CF15F9067195EC86057 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7D6964C9E324ABA58EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637D08E1E5B2BD3D3B78638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D83B88A975C6880CD57A3B017B869DE61432C5D738C9CEB9A0CC7F00164DA146DAFE8445B8C89999728AA50765F7900637D0FEED2715E18529389733CBF5DBD5E9C8A9BA7A39EFB766F5D81C698A659EA7CC7F00164DA146DA9985D098DBDEAEC8D2DCF9CF1F528DBCF6B57BC7E6449061A352F6E88A58FB86F5D81C698A659EA73AA81AA40904B5D9A18204E546F3947C13BDA61BF53F5E1D9735652A29929C6C4AD6D5ED66289B523666184CF4C3C14F6136E347CC761E07725E5C173C3A84C355C31AA2AB4E8F64BA3038C0950A5D36B5C8C57E37DE458B330BD67F2E7D9AF16D1867E19FE14079C09775C1D3CA48CF17B107DEF921CE791DD303D21008E298D5E8D9A59859A8B6D082881546D9349175ECD9A6C639B01B78DA827A17800CE7AD122C37AACC1F02731C566533BA786AA5CC5B56E945C8DA X-C1DE0DAB: 0D63561A33F958A54A8413B50B1749B45002B1117B3ED696800AEF5782C633071A1B8FE1FED62FE8823CB91A9FED034534781492E4B8EEAD69BF13FED57427F1BDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0ADBF74143AD284FC7177DD89D51EBB7742424CF958EAFF5D571004E42C50DC4CA955A7F0CF078B5EC49A30900B95165D347324AA9FA07FF01E5AE6222E6AC675C37C27449E9C0F57EEE7AE3D8DCA30D78FB025A071F5DA9B8D1D7E09C32AA3244CAF9B562D3AF87C9377DD89D51EBB7742A761F264DAE84560EA455F16B58544A2557BDE0DD54B3590A5AE236DF995FB59978A700BF655EAEEED6A17656DB59BCAD427812AF56FC65B X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojfJERNbCcGXMXwTq60CXenQ== X-Mailru-Sender: C4F68CFF4024C8867DFDF7C7F25884588E4DB3CD54CE702E601A8E04A89F3FFC85D54C89DB9FEF195C5AA4FCFAFB13EE645D15D82EE4B272BD6E4642A116CA93524AA66B5ACBE6721EF430B9A63E2A504198E0F3ECE9B5443453F38A29522196 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. --------------s60XaZ0028nY9VCp5pIiQJEQ 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 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. > >> 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. > >> 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 > The filenames are too long for my taste. I suggest the following names: > > Renamed. > >> diff --git a/src/lj_gc.c b/src/lj_gc.c >> index 591862b3..42348a34 100644 >> --- a/src/lj_gc.c >> +++ b/src/lj_gc.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 >> new file mode 100644 >> index 00000000..c388c6a7 >> --- /dev/null >> +++ b/test/tarantool-c-tests/lj-1168-heap-use-after-free-on-access-to-CTState-finalizer.test.c >> @@ -0,0 +1,66 @@ >> +#include > This include is excess. Removed. > >> + >> +#include "lua.h" >> +#include "lauxlib.h" >> + >> +#include "test.h" >> + >> +/* >> + * This test demonstrates LuaJIT's incorrect behaviour on >> + * loading Lua chunk. > Minor: chunk with cdata numbers. Updated. > >> + * Seehttps://github.com/LuaJIT/LuaJIT/issues/1168 for details. >> + * >> + * The GC is driving forward during parsing of the Lua chunk (`chunk`). > Comment line is more than 66 symbols. Here and below. Updated. > >> + * The chunk contains plenty of global objects, and the parsing >> + * of this creates a lot of strings to be used as keys in `_G`. >> + * Also, it contains several imaginary numbers (1i, 4i, 8i). > This chunk doesn't contains such objects, only cdata number 1LL. Right, updated. > >> + * That leads to the opening of the FFI library on-demand during the >> + * parsing of these numbers. After the FFI library is open, `ffi.gc` > (this number). Fixed. > >> + * 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 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. > It will be nice to add this comment to the commit message. Added. > >> + */ >> + >> +const char buff[] = "return 1LL"; >> + >> +/* >> + * lua_close is a part of testcase, so testcase creates >> + * it's own Lua state and closes it at the end. > Typo: s/it's/its/ Fixed. > >> + */ >> +static int unmarked_finalizer_tab_gcstart(void *test_state) > > Minor: Maybe rename this variable like the following: > test_state -> unused; Renamed. >> +{ >> + /* Shared Lua state is not needed. */ >> + (void)test_state; > Maybe it is better to introduce the UNUSED macro in the for > it. We already defined such macro for the following tests: > > * fix-yield-c-hook.test.c > * lj-549-lua-load.test.c > * misclib-sysprof-capi.test.c > * unit-tap.test.c Added a macro UNUSED and renamed unused to test_data back, because `UNUSED(unused)` doesn't look very good (because масло масляное). > >> + >> + /* Setup. */ >> + lua_State *L = luaL_newstate(); >> + >> + /* Set GC at the start. */ >> + lua_gc(L, LUA_GCCOLLECT, 0); >> + >> + if (luaL_loadbufferx(L, buff, sizeof(buff) - 1, "chunk", "t") != LUA_OK) > Why do we need to omit the ending zero byte? > > Maybe name "test_chunk" is better? Feel free to ignore. Renamed. > >> + printf("Err: %s\n", lua_tostring(L, -1)); > Please use bail_out instead of `printf()`. Fixed. > > | test_comment("error running payload: %s", lua_tostring(L, -1)); > | bail_out("error running " __func__ " test payload"); > > Maybe it is worth introducing some helpers in the for loading > and running Lua code inside our C tests. I don't think this code is complicated enough to introduce a helper for it. Ignored. > >> + >> + /* Finish GC cycle. */ > Minor: s/./ to collect the finalizer table./ Updated. > >> + while (!lua_gc(L, LUA_GCSTEP, -1)); >> + >> + /* 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), >> + }; >> + const int test_result = test_run_group(tgroup, NULL); >> + >> + return test_result; >> +} >> 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 @@ >> +local tap = require('tap') >> + >> +-- This test demonstrates LuaJIT's heap-use-after-free >> +-- on collecting garbage. Test simulates "unloading" of the library, > I'd rather say that "on cleaning of resources during shoutdown", since > it happens during `lua_close()`. Updated. > > Comment width is more than 66 lines. > >> +-- 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. > >> +test:plan(1) >> + >> +local ffi = require('ffi') >> + >> +ffi.gc = nil >> +collectgarbage() >> + >> +test:ok(true, 'no heap use after free') >> + >> +test:done(true) >> -- >> 2.34.1 >> --------------s60XaZ0028nY9VCp5pIiQJEQ 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 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.




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.


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

The filenames are too long for my taste. I suggest the following names:
<lj-1168-unmarked-finalizer-tab.test.c>
<lj-1168-unmarked-finalizer-tab.test.lua>
Renamed.


diff --git a/src/lj_gc.c b/src/lj_gc.c
index 591862b3..42348a34 100644
--- a/src/lj_gc.c
+++ b/src/lj_gc.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
new file mode 100644
index 00000000..c388c6a7
--- /dev/null
+++ b/test/tarantool-c-tests/lj-1168-heap-use-after-free-on-access-to-CTState-finalizer.test.c
@@ -0,0 +1,66 @@
+#include <string.h>

This include is excess.

Removed.




+
+#include "lua.h"
+#include "lauxlib.h"
+
+#include "test.h"
+
+/*
+ * This test demonstrates LuaJIT's incorrect behaviour on
+ * loading Lua chunk.

Minor: chunk with cdata numbers.

Updated.




+ * See https://github.com/LuaJIT/LuaJIT/issues/1168 for details.
+ *
+ * The GC is driving forward during parsing of the Lua chunk (`chunk`).

Comment line is more than 66 symbols. Here and below.

Updated.




+ * The chunk contains plenty of global objects, and the parsing
+ * of this creates a lot of strings to be used as keys in `_G`.
+ * Also, it contains several imaginary numbers (1i, 4i, 8i).

This chunk doesn't contains such objects, only cdata number 1LL.
Right, updated.


+ * That leads to the opening of the FFI library on-demand during the
+ * parsing of these numbers. After the FFI library is open, `ffi.gc`

(this number).

Fixed.




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

It will be nice to add this comment to the commit message.
Added.


+ */
+
+const char buff[] = "return 1LL";
+
+/*
+ * lua_close is a part of testcase, so testcase creates
+ * it's own Lua state and closes it at the end.

Typo: s/it's/its/

Fixed.




+ */
+static int unmarked_finalizer_tab_gcstart(void *test_state)


Minor: Maybe rename this variable like the following:
test_state -> unused;

Renamed.



      

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


      

      
Maybe it is better to introduce the UNUSED macro in the <utils.h> for
it. We already defined such macro for the following tests:

* fix-yield-c-hook.test.c
* lj-549-lua-load.test.c
* misclib-sysprof-capi.test.c
* unit-tap.test.c

    

    
Added a macro UNUSED and renamed unused to test_data back,

    
because `UNUSED(unused)` doesn't look very good (because масло
      масляное).

    

    

      



      

        
+
+	/* Setup. */
+	lua_State *L = luaL_newstate();
+
+	/* Set GC at the start. */
+	lua_gc(L, LUA_GCCOLLECT, 0);
+
+	if (luaL_loadbufferx(L, buff, sizeof(buff) - 1, "chunk", "t") != LUA_OK)


      

      
Why do we need to omit the ending zero byte?

Maybe name "test_chunk" is better? Feel free to ignore.

    

    Renamed.

    

      



      

        
+		printf("Err: %s\n", lua_tostring(L, -1));


      

      
Please use bail_out instead of `printf()`.

    

    
Fixed.

    


    

    

      

| test_comment("error running payload: %s", lua_tostring(L, -1));
| bail_out("error running " __func__ " test payload");

Maybe it is worth introducing some helpers in the <utils.h> for loading
and running Lua code inside our C tests.

    

    
I don't think this code is complicated enough to introduce a
      helper for it.

    
Ignored.

    

    

      



      

        
+
+	/* Finish GC cycle. */


      

      
Minor: s/./ to collect the finalizer table./

    

    
Updated.

    


    

    

      



      

        
+	while (!lua_gc(L, LUA_GCSTEP, -1));
+
+	/* 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),
+	};
+	const int test_result = test_run_group(tgroup, NULL);
+
+	return test_result;
+}
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 @@
+local tap = require('tap')
+
+-- This test demonstrates LuaJIT's heap-use-after-free
+-- on collecting garbage. Test simulates "unloading" of the library,


      

      
I'd rather say that "on cleaning of resources during shoutdown", since
it happens during `lua_close()`.

    

    Updated.

    

      

Comment width is more than 66 lines.



      

        
+-- or removing some of the functionality of it and then calls
+-- `collectgarbage`.
+-- See https://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.

    

      



      

        
+test:plan(1)
+
+local ffi = require('ffi')
+
+ffi.gc = nil
+collectgarbage()
+
+test:ok(true, 'no heap use after free')
+
+test:done(true)
-- 
2.34.1



      

      

    

  
  


--------------s60XaZ0028nY9VCp5pIiQJEQ--