From: Sergey Bronnikov via Tarantool-patches <tarantool-patches@dev.tarantool.org> To: Sergey Kaplun <skaplun@tarantool.org>, Maxim Kokryashkin <m.kokryashkin@tarantool.org> Cc: tarantool-patches@dev.tarantool.org Subject: Re: [Tarantool-patches] [PATCH luajit] FFI: Fix various issues in recff_cdata_arith. Date: Mon, 9 Sep 2024 18:37:36 +0300 [thread overview] Message-ID: <5f51a9ad-3302-4e0c-8d91-c5a26c147fe2@tarantool.org> (raw) In-Reply-To: <20240821165250.11087-1-skaplun@tarantool.org> [-- Attachment #1: Type: text/plain, Size: 10062 bytes --] Hi, Sergey, thanks for the patch! see my comments below. On 21.08.2024 19:52, Sergey Kaplun wrote: > From: Mike Pall <mike> > > Thanks to Sergey Kaplun. > > (cherry picked from commit 7a608e4425ce0777f5c980dad9f4fdc1bcce0b8c) > > The aforementioned function doesn't handle gentle recording of the cdata > addition to `nil` or some string, presuming that the interpreter will throw > an error. This may lead to an assertion due to an uninitialized ctype > state or an attempt to use in the fold engine the non-cdata summand (casted > to `IR_KPTR`) as the (invalid) GC pointer. > > This patch handles such cases by: > * Initializing the ctype state where it is needed. > * Raising an error when the argument has a suspicious type. Since the > interpreter will throw the error anyway, these traces will abort > anyway. > > Sergey Kaplun: > * added the description and the test for the problem > > Part of tarantool/tarantool#10199 > --- > > Branch:https://github.com/tarantool/luajit/tree/skaplun/lj-1224-fix-jit-cdata-arith > Related issues: > *https://github.com/tarantool/tarantool/issues/10199 > *https://github.com/LuaJIT/LuaJIT/issues/1224 > > src/lj_crecord.c | 10 ++-- > .../lj-1224-fix-cdata-arith-ptr.test.lua | 48 +++++++++++++++++++ This test does not fail without fix (but repro from the issue does): [0] ~/sources/MRG/tarantool/third_party/luajit $ ./build/gc64/src/luajit -Ohotloop=1 -e " repeat r = 1LL + nil until true " LuaJIT ASSERT /home/sergeyb/sources/MRG/tarantool/third_party/luajit/src/lj_ctype.c:185: lj_ctype_intern: uninitialized cts->L Aborted [0] ~/sources/MRG/tarantool/third_party/luajit $ ./build/gc64/src/luajit test/tarantool-tests/lj-1224-fix-cdata-arith-ptr.test.lua TAP version 13 1..2 # cdata arithmetic with nil 1..2 ok - correct recording error with bad cdata arithmetic ok - correct error message # cdata arithmetic with nil: end ok - cdata arithmetic with nil # cdata arithmetic with string 1..2 ok - correct recording error with bad cdata arithmetic ok - correct error message # cdata arithmetic with string: end ok - cdata arithmetic with string [0] ~/sources/MRG/tarantool/third_party/luajit $ > .../lj-1224-fix-cdata-arith-side.test.lua | 20 ++++++++ > .../lj-1224-fix-cdata-arith-side/script.lua | 16 +++++++ > .../lj-1224-fix-cdata-arith.test.lua | 20 ++++++++ > .../lj-1224-fix-cdata-arith/script.lua | 18 +++++++ > 6 files changed, 128 insertions(+), 4 deletions(-) > create mode 100644 test/tarantool-tests/lj-1224-fix-cdata-arith-ptr.test.lua > create mode 100644 test/tarantool-tests/lj-1224-fix-cdata-arith-side.test.lua > create mode 100644 test/tarantool-tests/lj-1224-fix-cdata-arith-side/script.lua > create mode 100644 test/tarantool-tests/lj-1224-fix-cdata-arith.test.lua > create mode 100644 test/tarantool-tests/lj-1224-fix-cdata-arith/script.lua > > diff --git a/src/lj_crecord.c b/src/lj_crecord.c > index 255bfa45..81ae6dfa 100644 > --- a/src/lj_crecord.c > +++ b/src/lj_crecord.c > @@ -1473,7 +1473,8 @@ static TRef crec_arith_meta(jit_State *J, TRef *sp, CType **s, CTState *cts, > > void LJ_FASTCALL recff_cdata_arith(jit_State *J, RecordFFData *rd) > { > - CTState *cts = ctype_ctsG(J2G(J)); > + CTState *cts = ctype_cts(J->L); > + MMS mm = (MMS)rd->data; > TRef sp[2]; > CType *s[2]; > MSize i; > @@ -1523,6 +1524,8 @@ void LJ_FASTCALL recff_cdata_arith(jit_State *J, RecordFFData *rd) > } > } > } else if (tref_isnil(tr)) { > + if (!(mm == MM_len || mm == MM_eq || mm == MM_lt || mm == MM_le)) > + lj_trace_err(J, LJ_TRERR_BADTYPE); > tr = lj_ir_kptr(J, NULL); > ct = ctype_get(cts, CTID_P_VOID); > } else if (tref_isinteger(tr)) { > @@ -1541,12 +1544,12 @@ void LJ_FASTCALL recff_cdata_arith(jit_State *J, RecordFFData *rd) > ct = ctype_child(cts, cct); > tr = lj_ir_kint(J, (int32_t)ofs); > } else { /* Interpreter will throw or return false. */ > - ct = ctype_get(cts, CTID_P_VOID); > + lj_trace_err(J, LJ_TRERR_BADTYPE); > } > } else if (ctype_isptr(ct->info)) { > tr = emitir(IRT(IR_ADD, IRT_PTR), tr, lj_ir_kintp(J, sizeof(GCstr))); > } else { > - ct = ctype_get(cts, CTID_P_VOID); > + lj_trace_err(J, LJ_TRERR_BADTYPE); > } > } else if (!tref_isnum(tr)) { > tr = 0; > @@ -1558,7 +1561,6 @@ void LJ_FASTCALL recff_cdata_arith(jit_State *J, RecordFFData *rd) > } > { > TRef tr; > - MMS mm = (MMS)rd->data; > if ((mm == MM_len || mm == MM_concat || > (!(tr = crec_arith_int64(J, sp, s, mm)) && > !(tr = crec_arith_ptr(J, sp, s, mm)))) && > diff --git a/test/tarantool-tests/lj-1224-fix-cdata-arith-ptr.test.lua b/test/tarantool-tests/lj-1224-fix-cdata-arith-ptr.test.lua > new file mode 100644 > index 00000000..9de9586d > --- /dev/null > +++ b/test/tarantool-tests/lj-1224-fix-cdata-arith-ptr.test.lua > @@ -0,0 +1,48 @@ > +local tap = require('tap') > + > +-- Test file to demonstrate LuaJIT's incorrect recording of cdata > +-- arithmetic looks like pointer arithmetic, which raises the > +-- error. > +-- See also:https://github.com/LuaJIT/LuaJIT/issues/1224. > + > +local test = tap.test('lj-1224-fix-cdata-arith-ptr'):skipcond({ > + ['Test requires JIT enabled'] = not jit.status(), > +}) > + > +test:plan(2) > + > +local function arith_nil_protected() > + local i = 1 > + while i < 3 do > + -- Before the patch, `nil` was "cast" to `IR_KPTR(NULL)` > + -- during the recording of the cdata arith metamethod, and the > + -- fold engine tried to add this value to the `-1LL`. This > + -- obviously leads to the value being out-of-range for the GC > + -- pointer. > + local _ = -1LL + nil > + i = i + 1 > + end > +end > + > +-- The similar test but for the string value instead of nil. > +local function arith_str_protected() > + local i = 1 > + while i < 3 do > + local _ = 1LL + '' > + i = i + 1 > + end > +end > + > +local function check_error(subtest, test_f) > + subtest:plan(2) > + -- Reset counters. > + jit.opt.start('hotloop=1') > + local result, errmsg = pcall(test_f) > + subtest:ok(not result, 'correct recording error with bad cdata arithmetic') > + subtest:like(errmsg, 'attempt to perform arithmetic', 'correct error message') > +end > + > +test:test('cdata arithmetic with nil', check_error, arith_nil_protected) > +test:test('cdata arithmetic with string', check_error, arith_str_protected) > + > +test:done(true) > diff --git a/test/tarantool-tests/lj-1224-fix-cdata-arith-side.test.lua b/test/tarantool-tests/lj-1224-fix-cdata-arith-side.test.lua > new file mode 100644 > index 00000000..3f3e6011 > --- /dev/null > +++ b/test/tarantool-tests/lj-1224-fix-cdata-arith-side.test.lua > @@ -0,0 +1,20 @@ > +local tap = require('tap') > + > +-- Test file to demonstrate LuaJIT's incorrect recording for the > +-- side trace with cdata arithmetic, which raises the error. > +-- See also:https://github.com/LuaJIT/LuaJIT/issues/1224. > + > +local test = tap.test('lj-1224-fix-cdata-arith-side'):skipcond({ > + ['Test requires JIT enabled'] = not jit.status(), > +}) > + > +-- The loading of the 'tap' module initializes `cts->L` during > +-- parsing. Run standalone script for testing. > +local script = require('utils').exec.makecmd(arg) > + > +test:plan(1) > + > +local output = script() > +test:is(output, 'OK', 'correct recording with uninitialized cts->L') > + > +test:done(true) > diff --git a/test/tarantool-tests/lj-1224-fix-cdata-arith-side/script.lua b/test/tarantool-tests/lj-1224-fix-cdata-arith-side/script.lua > new file mode 100644 > index 00000000..e96877ff > --- /dev/null > +++ b/test/tarantool-tests/lj-1224-fix-cdata-arith-side/script.lua > @@ -0,0 +1,16 @@ > +local function test_record_protected() > + -- Start the root trace here. > + for _ = 1, 3 do end > + -- An exit from the root trace triggered the side trace > + -- recording. > + local _ = 1LL + '' > +end > + > +jit.opt.start('hotloop=1', 'hotexit=1') > + > +local status, errmsg = pcall(test_record_protected) > + > +assert(not status, 'recoding correctly handling the error') > +assert(errmsg:match('attempt to perform arithmetic'), 'correct error message') > + > +print('OK') > diff --git a/test/tarantool-tests/lj-1224-fix-cdata-arith.test.lua b/test/tarantool-tests/lj-1224-fix-cdata-arith.test.lua > new file mode 100644 > index 00000000..ba89b472 > --- /dev/null > +++ b/test/tarantool-tests/lj-1224-fix-cdata-arith.test.lua > @@ -0,0 +1,20 @@ > +local tap = require('tap') > + > +-- Test file to demonstrate LuaJIT's incorrect recording of cdata > +-- arithmetic, which raises the error. > +-- See also:https://github.com/LuaJIT/LuaJIT/issues/1224. > + > +local test = tap.test('lj-1224-fix-cdata-arith'):skipcond({ > + ['Test requires JIT enabled'] = not jit.status(), > +}) > + > +-- The loading of the 'tap' module initializes `cts->L` during > +-- parsing. Run standalone script for testing. > +local script = require('utils').exec.makecmd(arg) > + > +test:plan(1) > + > +local output = script() > +test:is(output, 'OK', 'correct recording with uninitialized cts->L') > + > +test:done(true) > diff --git a/test/tarantool-tests/lj-1224-fix-cdata-arith/script.lua b/test/tarantool-tests/lj-1224-fix-cdata-arith/script.lua > new file mode 100644 > index 00000000..df79e430 > --- /dev/null > +++ b/test/tarantool-tests/lj-1224-fix-cdata-arith/script.lua > @@ -0,0 +1,18 @@ > +local function test_record_protected() > + local i = 1 > + while i < 3 do > + -- Use `while` to start compilation of the trace at the first > + -- iteration, before `cts->L` is uninitialized. > + local _ = 1LL + nil > + i = i + 1 > + end > +end > + > +jit.opt.start('hotloop=1') > + > +local status, errmsg = pcall(test_record_protected) > + > +assert(not status, 'recoding correctly handling the error') > +assert(errmsg:match('attempt to perform arithmetic'), 'correct error message') > + > +print('OK') [-- Attachment #2: Type: text/html, Size: 12523 bytes --]
next prev parent reply other threads:[~2024-09-09 15:37 UTC|newest] Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top 2024-08-21 16:52 Sergey Kaplun via Tarantool-patches 2024-09-09 15:37 ` Sergey Bronnikov via Tarantool-patches [this message] 2024-09-10 10:04 ` Sergey Bronnikov via Tarantool-patches 2024-09-10 14:01 ` Sergey Kaplun via Tarantool-patches 2024-09-12 10:16 ` Sergey Kaplun via Tarantool-patches 2024-09-12 11:45 ` Sergey Bronnikov via Tarantool-patches 2024-09-23 7:21 ` Maxim Kokryashkin via Tarantool-patches
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=5f51a9ad-3302-4e0c-8d91-c5a26c147fe2@tarantool.org \ --to=tarantool-patches@dev.tarantool.org \ --cc=m.kokryashkin@tarantool.org \ --cc=sergeyb@tarantool.org \ --cc=skaplun@tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH luajit] FFI: Fix various issues in recff_cdata_arith.' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox