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 E1ED02552B; Fri, 11 Nov 2022 12:06:10 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org E1ED02552B DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1668157571; bh=nw+2PSnyz3TRC0U/TW5hDv9G+BVJXyvbl25beSCT8YU=; 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=vgeeQJZB8CxMCZZg0QjtPD89P3otbc074e1145Kir+S24E3w8Vc/8+szzLM5XXTCt Gnrj4vc8mMZXzm5YPol+g0ILAoO9XnObREopHrqJDrxnQfwn92EWBVu0mdPA7T7rNI Lk+urpkFmCNE437WTJD1ActxgH1DD1uafUL4+RiU= Received: from smtpng1.i.mail.ru (smtpng1.i.mail.ru [94.100.181.251]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id D965070370 for ; Fri, 11 Nov 2022 12:06:08 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org D965070370 Received: by smtpng1.m.smailru.net with esmtpa (envelope-from ) id 1otPzD-00079k-RL; Fri, 11 Nov 2022 12:06:08 +0300 Date: Fri, 11 Nov 2022 11:53:27 +0300 To: Sergey Kaplun Message-ID: References: <20221109174948.10952-1-skaplun@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20221109174948.10952-1-skaplun@tarantool.org> X-Clacks-Overhead: GNU Terry Pratchett X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD9D880D530162779F13EAC6FBDF6B7DB4C8B89852CAAF032A0182A05F53808504023D29482BF68304F7858A3BD9F5A2DA8975189EE0936E86E12F06C2DE8FFC60E X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE796AA83661EF29BCEEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006370218B86C916BF3528638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D89282BEC38EFDEDCDC417E4ACFF4588B0117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCECADA55FE5B58BB7A471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F4460429728776938767073520C65AC60A1F0286FE2CC0D3CB04F14752D2E47CDBA5A96583BA9C0B312567BB231DD303D21008E29813377AFFFEAFD269A417C69337E82CC2E827F84554CEF50127C277FBC8AE2E8BA83251EDC214901ED5E8D9A59859A8B613653AF4ED260ED2089D37D7C0E48F6C5571747095F342E88FB05168BE4CE3AF X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34FF70CCB59A6ED8700A7FFDB72D9FF732FD700B06987E2541CC775EC96D21F81E30BBD4B2A336CBFB1D7E09C32AA3244C13A7F3918E9128A2C8E43DC72C27D65524AF4FAF06DA24FD927AC6DF5659F194 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojSqkc8Nj21UMUMoweyoo/Lg== X-DA7885C5: D987706EF9FC303AB0A2DD148E512202F7602E76FA85723FC0EAD8027EBEED3D262E2D401490A4A0DB037EFA58388B346E8BC1A9835FDE71 X-Mailru-Sender: 689FA8AB762F7393CC2E0F076E87284E7D03003B7731D4BA0E79CBDBB7B92BA2A7C8D0F45F857DBFE9F1EFEE2F478337FB559BB5D741EB964C8C2C849690F8E70A04DAD6CC59E3365FEEDEB644C299C0ED14614B50AE0675 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit] Ensure correct stack top for OOM error message. 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: Igor Munkin via Tarantool-patches Reply-To: Igor Munkin Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Sergey, Thanks for the patch! On 09.11.22, Sergey Kaplun wrote: > From: Mike Pall > > Reported by Sergey Kaplun. > > (cherry picked from commit ca8d3257bb44e42100c7910c47dcdcf01f494187) > > `lj_err_mem()` doesn't set up `L->top` for Lua frames, but uses it for I believe it "doesn't set `L->top` for the current Lua frame, but...", doesn't it? > pushing error message on the stack. So, when we call some routine that Typo: s/some routine/a routine/ or s/some routine/arbitrary routine/. > does some allocations, it can raise the OOM error (like `lj_tab_dup()` Strictly saying it's LUA_ERRMEM, not OOM. > in `BC_TDUP`) and this error may corrupt stack for unwind in situations > when `L->top` < `L->base`. > > This patch restores `L->top` for Lua frames when raise the error via Typo: s/raise the error/the error is raised/. > `lj_err_mem()`. > > Sergey Kaplun: > * added the description and the test for the problem > > Resolves tarantool/tarantool#3840 > Part of tarantool/tarantool#7230 > --- > > Issues: > * https://github.com/LuaJIT/LuaJIT/issues/906 > * https://github.com/tarantool/tarantool/issues/7230 > * https://github.com/tarantool/tarantool/issues/3840 > PR: https://github.com/tarantool/tarantool/pull/7915 > Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-906-fix-err-mem > > Red LuaJIT CI for MacOS Release builds is a known issue with self-hosted > runners, as Igor has said before. Everything is fixed at the moment, please try to rebase. > > src/lj_err.c | 1 + > .../lj-906-fix-err-mem.test.lua | 90 +++++++++++++++++++ > 2 files changed, 91 insertions(+) > create mode 100644 test/tarantool-tests/lj-906-fix-err-mem.test.lua > > diff --git a/test/tarantool-tests/lj-906-fix-err-mem.test.lua b/test/tarantool-tests/lj-906-fix-err-mem.test.lua > new file mode 100644 > index 00000000..a139e1c9 > --- /dev/null > +++ b/test/tarantool-tests/lj-906-fix-err-mem.test.lua > @@ -0,0 +1,90 @@ > +local tap = require('tap') > +local ffi = require('ffi') > +local table_new = require('table.new') > + > +-- Avoid test to be killed. > +require('utils').skipcond(ffi.abi('gc64'), 'test is not GC64 only') > + > +local test = tap.test('lj-906-fix-err-mem') > +test:plan(1) > + > +local KB = 1024 > +local MB = 1024 * KB > + > +-- The maximum available table size, taking into account created > +-- constants for one function. > +local TNEW_SIZE = 511 > + > +local gc_anchor = {} > + > +-- This function works until raises the error. > +local function eat_chunks(size) > + -- Need raise the OOM error inside TDUP, not TNEW, so reserve > + -- memory for it. > + -- luacheck: no unused > + local tnew_anchor = table_new(TNEW_SIZE, 0) > + while true do > + table.insert(gc_anchor, ffi.new('char [?]', size)) > + end > +end > + > +-- Function to format inner tab leading to TDUP emitting. > +local function format_inner_tab() Minor: Maybe instead of ? Just asking, feel free to ignore. > + local inner_tab = '' > + local inner_depth = 128 Why 128? Please, drop a few words. > + -- Repeate table template for TDUP. > + for _ = 1, inner_depth do > + inner_tab = inner_tab .. '{a =' > + end > + inner_tab = inner_tab .. '{}' > + for _ = 1, inner_depth do > + inner_tab = inner_tab .. '},' > + end > + return inner_tab > +end > + Minor: Same (also think about moving these helpers to utils.*). Maybe instead of ? And mention TDUP specifics in the comment. Just asking too, feel free to ignore. > +local function format_TDUP_chunk() > + local big_tab = 'local _ = {\n' > + local inner_tab = format_inner_tab() > + for _ = 1, TNEW_SIZE do > + big_tab = big_tab .. inner_tab .. '\n' > + end > + big_tab = big_tab .. '}' > + return big_tab > +end > + > +local TDUP, err = loadstring(format_TDUP_chunk()) > +assert(TDUP, err) > + > +local function frame_before_TDUP() > + -- Stack slots are needed for coredump in case of misbehaviour. Why are they needed? Please drop few more words regarding this. > + -- luacheck: no unused > + local frame_slot1, frame_slot2 > + TDUP() > + return frame_slot1, frame_slot2 > +end > + Minor: I believe you can pack these two lines into something one can call : | local function janitor() | collectgarbage('collect') | collectgarbage('stop') | end Anyway, you can leave this as is. > +collectgarbage() > +collectgarbage('stop') > + > +-- Avoid OOM on traces. > +jit.off() > + > +-- Stack slots are needed for coredump in case of misbehaviour. > +-- luacheck: no unused > +local r, e = pcall(eat_chunks, 8 * MB) > +collectgarbage() > +pcall(eat_chunks, 8 * KB) > +collectgarbage() > +pcall(eat_chunks, 8) > +collectgarbage() > + > +pcall(frame_before_TDUP) Do we need to check the status of the above? > + > +-- Release memory for `tap` functions. > +gc_anchor = nil > +collectgarbage() Is this step required? I doubt. > + > +test:ok(true, 'correctly throw memory error') > + > +os.exit(test:check() and 0 or 1) > -- > 2.34.1 > -- Best regards, IM