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 B7D61EB44FF; Tue, 9 Jul 2024 14:52:12 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org B7D61EB44FF DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1720525932; bh=ehLP54lHn/qsUb4SZTh+7frkSnHH7o6qgERLWGfMuts=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=eJaXGqJS3G4JYgHTdLNXFEt6C+yuX6WJZOsrdy9Mx3zN8Z8tpLp4aMyiQsdU6es9u V11Amkvob/+W9qqADWTCG/UfEWU2MfhyW9V+HyxXD2l2l0INwsFD+lBhbMQ9xYY0zf ogQzjJyQ+aD3tpqXRaMK+NtF/2/HT1JGvfKrsMsM= 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 BAE45EB44F2 for ; Tue, 9 Jul 2024 14:52:10 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org BAE45EB44F2 Received: by smtp49.i.mail.ru with esmtpa (envelope-from ) id 1sR9OC-0000000HCz7-2cH3; Tue, 09 Jul 2024 14:52:09 +0300 Date: Tue, 9 Jul 2024 14:52:00 +0300 To: Sergey Bronnikov Message-ID: References: <48e2f924db3e2bdac356cefd9aae21e42556514b.1720521873.git.sergeyb@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <48e2f924db3e2bdac356cefd9aae21e42556514b.1720521873.git.sergeyb@tarantool.org> X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: 78E4E2B564C1792B X-77F55803: 4F1203BC0FB41BD91AFB032D31068B0B306917858D4BA930F887C5E84208B0B2CD62213F67905E7ABF69F3E34D041A26EF02C6BBF7CC3ADA0C7394DC60BE01BA91E1AA9215045D1C24DAF05A372A3159 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7AD2F2D6F6013FF7FC2099A533E45F2D0395957E7521B51C2CFCAF695D4D8E9FCEA1F7E6F0F101C6778DA827A17800CE7589DF9800DB1E191EA1F7E6F0F101C6723150C8DA25C47586E58E00D9D99D84E1BDDB23E98D2D38B043BF0FB74779F36E55511DF064010F5BB63AC0E6D152D8B080EFC0037FE762BA471835C12D1D9774AD6D5ED66289B5278DA827A17800CE71AE4D56B06699BBC9FA2833FD35BB23D2EF20D2F80756B5F868A13BD56FB6657A471835C12D1D977725E5C173C3A84C3E478A468B35FE767117882F4460429728AD0CFFFB425014E868A13BD56FB6657E2021AF6380DFAD1A18204E546F3947C1D471462564A2E192E808ACE2090B5E1725E5C173C3A84C3ED8438A78DFE0A9E089D37D7C0E48F6C8AA50765F79006377870F476E0DB9443EFF80C71ABB335746BA297DBC24807EABDAD6C7F3747799A X-C1DE0DAB: 0D63561A33F958A53167DD90A483FC965002B1117B3ED69680DD53E386E4F3E303803A57F48E4E5A823CB91A9FED034534781492E4B8EEAD220496FFA5CD4785BDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0ADBF74143AD284FC7177DD89D51EBB7742424CF958EAFF5D571004E42C50DC4CA955A7F0CF078B5EC49A30900B95165D34A541DB4061E9383236583F860BC2A5C9B8D3061B7D16F006A92D8882A9D269F9CEBB8918CC7842361D7E09C32AA3244CDDC2321FB00AF0E913EA5626864FADBF05BEFBFB9F145B8AEA455F16B58544A2557BDE0DD54B3590A5AE236DF995FB59829709634694AABAED6A17656DB59BCAD427812AF56FC65B X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojfJERNbCcGXNVS84IZpXLiQ== X-DA7885C5: CF9F4AE49314AC89F255D290C0D534F9F09F30F2BB88FAD6A4871814B975A7F18D12859541E9749F5B1A4C17EAA7BC4BEF2421ABFA55128DAF83EF9164C44C7E X-Mailru-Sender: 689FA8AB762F7393C6D0B12EA33CAA9B7CD80A6E366E805C316087A2BDAE3045E92F361B86F5CEA8E49D44BB4BD9522A059A1ED8796F048DB274557F927329BE89D5A3BC2B10C37545BD1C3CC395C826B4A721A3011E896F 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 Kaplun via Tarantool-patches Reply-To: Sergey Kaplun Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 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... > 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()` > 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: > > 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. > + > +#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. > + * 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. > + * 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. > + * 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). > + * 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. > + */ > + > +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/ > + */ > +static int unmarked_finalizer_tab_gcstart(void *test_state) Minor: Maybe rename this variable like the following: test_state -> unused; > +{ > + /* 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 > + > + /* 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. > + printf("Err: %s\n", lua_tostring(L, -1)); Please use bail_out instead of `printf()`. | 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. > + > + /* Finish GC cycle. */ Minor: s/./ to collect the finalizer table./ > + 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()`. 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. > +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 > -- Best regards, Sergey Kaplun