* [Tarantool-patches] [PATCH luajit] Restore state when recording __concat metamethod throws an error.
@ 2024-08-21 8:23 Sergey Kaplun via Tarantool-patches
2024-09-13 13:07 ` Sergey Bronnikov via Tarantool-patches
2024-10-18 15:14 ` Sergey Kaplun via Tarantool-patches
0 siblings, 2 replies; 7+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2024-08-21 8:23 UTC (permalink / raw)
To: Maxim Kokryashkin, Sergey Bronnikov; +Cc: tarantool-patches
From: Mike Pall <mike>
Thanks to Sergey Kaplun.
(cherry picked from commit 7421a1b33c7ea46f12bba9700c15b5c90253fee0)
Since neither `rec_cat()` nor `lj_record_ret()` restore the Lua stack,
if the error is raised, it leads either to a crash in `BC_RET` or to the
"unbalanced stack" assertion failure.
This patch protects the `rec_mm_arith()`, which can raise an error. Its
caller returns the negated error code to be rethrown in case of the
caught error.
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-1234-err-in-record-concat
Related issues:
* https://github.com/tarantool/tarantool/issues/10199
* https://github.com/LuaJIT/LuaJIT/issues/1234
src/lj_record.c | 26 ++++++++++-
.../lj-1234-err-in-record-concat.test.lua | 43 +++++++++++++++++++
2 files changed, 67 insertions(+), 2 deletions(-)
create mode 100644 test/tarantool-tests/lj-1234-err-in-record-concat.test.lua
diff --git a/src/lj_record.c b/src/lj_record.c
index 96fe26d8..64da3256 100644
--- a/src/lj_record.c
+++ b/src/lj_record.c
@@ -940,7 +940,9 @@ void lj_record_ret(jit_State *J, BCReg rbase, ptrdiff_t gotresults)
J->L->base = b + baseadj;
copyTV(J->L, b-(2<<LJ_FR2), &save);
}
- if (tr) { /* Store final result. */
+ if (tr >= 0xffffff00) {
+ lj_err_throw(J->L, -(int32_t)tr); /* Propagate errors. */
+ } else if (tr) { /* Store final result. */
BCReg dst = bc_a(*(frame_contpc(frame)-1));
J->base[dst] = tr;
if (dst >= J->maxslot) {
@@ -1936,12 +1938,27 @@ static TRef rec_tnew(jit_State *J, uint32_t ah)
/* -- Concatenation ------------------------------------------------------- */
+typedef struct RecCatDataCP {
+ jit_State *J;
+ RecordIndex *ix;
+} RecCatDataCP;
+
+static TValue *rec_mm_concat_cp(lua_State *L, lua_CFunction dummy, void *ud)
+{
+ RecCatDataCP *rcd = (RecCatDataCP *)ud;
+ UNUSED(L); UNUSED(dummy);
+ rec_mm_arith(rcd->J, rcd->ix, MM_concat); /* Call __concat metamethod. */
+ return NULL;
+}
+
static TRef rec_cat(jit_State *J, BCReg baseslot, BCReg topslot)
{
TRef *top = &J->base[topslot];
TValue savetv[5+LJ_FR2];
BCReg s;
RecordIndex ix;
+ RecCatDataCP rcd;
+ int errcode;
lj_assertJ(baseslot < topslot, "bad CAT arg");
for (s = baseslot; s <= topslot; s++)
(void)getslot(J, s); /* Ensure all arguments have a reference. */
@@ -1977,8 +1994,11 @@ static TRef rec_cat(jit_State *J, BCReg baseslot, BCReg topslot)
ix.tab = top[-1];
ix.key = top[0];
memcpy(savetv, &J->L->base[topslot-1], sizeof(savetv)); /* Save slots. */
- rec_mm_arith(J, &ix, MM_concat); /* Call __concat metamethod. */
+ rcd.J = J;
+ rcd.ix = &ix;
+ errcode = lj_vm_cpcall(J->L, NULL, &rcd, rec_mm_concat_cp);
memcpy(&J->L->base[topslot-1], savetv, sizeof(savetv)); /* Restore slots. */
+ if (errcode) return (TRef)(-errcode);
return 0; /* No result yet. */
}
@@ -2300,6 +2320,8 @@ void lj_record_ins(jit_State *J)
case BC_CAT:
rc = rec_cat(J, rb, rc);
+ if (rc >= 0xffffff00)
+ lj_err_throw(J->L, -(int32_t)rc); /* Propagate errors. */
break;
/* -- Constant and move ops --------------------------------------------- */
diff --git a/test/tarantool-tests/lj-1234-err-in-record-concat.test.lua b/test/tarantool-tests/lj-1234-err-in-record-concat.test.lua
new file mode 100644
index 00000000..9abaeba5
--- /dev/null
+++ b/test/tarantool-tests/lj-1234-err-in-record-concat.test.lua
@@ -0,0 +1,43 @@
+local tap = require('tap')
+
+-- Test file to demonstrate the crash during the concat recording
+-- if it throws an error.
+-- See also: https://github.com/LuaJIT/LuaJIT/issues/1234.
+
+local test = tap.test('lj-1234-err-in-record-concat'):skipcond({
+ ['Test requires JIT enabled'] = not jit.status(),
+})
+
+test:plan(2)
+
+jit.opt.start('hotloop=1')
+
+local __concat = function(v1, v2)
+ return tostring(v1) .. tostring(v2)
+end
+
+-- Need to use metamethod call in the concat recording.
+debug.setmetatable(nil, {
+ __concat = __concat,
+})
+
+local function test_concat_p()
+ local counter = 0
+ while counter < 1 do
+ counter = counter + 1
+ -- The first result is placed on the Lua stack before the
+ -- error is raised. When the error is raised, it is handled by
+ -- the trace recorder, but since neither `rec_cat()` nor
+ -- `lj_record_ret()` restore the Lua stack (before the patch),
+ -- it becomes unbalanced after the instruction recording
+ -- attempt.
+ local _ = {} .. (nil .. nil)
+ end
+end
+
+local result, errmsg = pcall(test_concat_p)
+
+test:ok(not result, 'the error is raised')
+test:like(errmsg, 'attempt to concatenate a table value', 'correct error')
+
+test:done(true)
--
2.45.2
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit] Restore state when recording __concat metamethod throws an error.
2024-08-21 8:23 [Tarantool-patches] [PATCH luajit] Restore state when recording __concat metamethod throws an error Sergey Kaplun via Tarantool-patches
@ 2024-09-13 13:07 ` Sergey Bronnikov via Tarantool-patches
2024-09-13 15:27 ` Sergey Kaplun via Tarantool-patches
2024-10-18 15:14 ` Sergey Kaplun via Tarantool-patches
1 sibling, 1 reply; 7+ messages in thread
From: Sergey Bronnikov via Tarantool-patches @ 2024-09-13 13:07 UTC (permalink / raw)
To: Sergey Kaplun, Maxim Kokryashkin; +Cc: tarantool-patches
[-- Attachment #1: Type: text/plain, Size: 1796 bytes --]
Hi, Sergey,
thanks for the patch! LGTM with a minor comment. See below.
On 21.08.2024 11:23, Sergey Kaplun wrote:
<snipped>
> diff --git a/test/tarantool-tests/lj-1234-err-in-record-concat.test.lua b/test/tarantool-tests/lj-1234-err-in-record-concat.test.lua
> new file mode 100644
> index 00000000..9abaeba5
> --- /dev/null
> +++ b/test/tarantool-tests/lj-1234-err-in-record-concat.test.lua
> @@ -0,0 +1,43 @@
> +local tap = require('tap')
> +
> +-- Test file to demonstrate the crash during the concat recording
> +-- if it throws an error.
> +-- See also:https://github.com/LuaJIT/LuaJIT/issues/1234.
> +
> +local test = tap.test('lj-1234-err-in-record-concat'):skipcond({
> + ['Test requires JIT enabled'] = not jit.status(),
> +})
> +
> +test:plan(2)
> +
> +jit.opt.start('hotloop=1')
> +
> +local __concat = function(v1, v2)
> + return tostring(v1) .. tostring(v2)
> +end
> +
> +-- Need to use metamethod call in the concat recording.
> +debug.setmetatable(nil, {
I propose to add a comment that explain why `nil` is used here.
> + __concat = __concat,
> +})
> +
> +local function test_concat_p()
> + local counter = 0
> + while counter < 1 do
> + counter = counter + 1
> + -- The first result is placed on the Lua stack before the
> + -- error is raised. When the error is raised, it is handled by
> + -- the trace recorder, but since neither `rec_cat()` nor
> + -- `lj_record_ret()` restore the Lua stack (before the patch),
> + -- it becomes unbalanced after the instruction recording
> + -- attempt.
> + local _ = {} .. (nil .. nil)
> + end
> +end
> +
> +local result, errmsg = pcall(test_concat_p)
> +
> +test:ok(not result, 'the error is raised')
> +test:like(errmsg, 'attempt to concatenate a table value', 'correct error')
> +
> +test:done(true)
[-- Attachment #2: Type: text/html, Size: 2467 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit] Restore state when recording __concat metamethod throws an error.
2024-09-13 13:07 ` Sergey Bronnikov via Tarantool-patches
@ 2024-09-13 15:27 ` Sergey Kaplun via Tarantool-patches
2024-09-13 15:48 ` Sergey Kaplun via Tarantool-patches
0 siblings, 1 reply; 7+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2024-09-13 15:27 UTC (permalink / raw)
To: Sergey Bronnikov; +Cc: tarantool-patches
Hi, Sergey!
Thanks for the review!
On 13.09.24, Sergey Bronnikov wrote:
> Hi, Sergey,
>
> thanks for the patch! LGTM with a minor comment. See below.
>
> On 21.08.2024 11:23, Sergey Kaplun wrote:
>
>
> <snipped>
>
> > +-- Need to use metamethod call in the concat recording.
> > +debug.setmetatable(nil, {
> I propose to add a comment that explain why `nil` is used here.
I've removed the nil usage to avoid confusion.
Branch is rebased on the current master and force-pushed.
===================================================================
diff --git a/test/tarantool-tests/lj-1234-err-in-record-concat.test.lua b/test/tarantool-tests/lj-1234-err-in-record-concat.test.lua
index 9abaeba5..4fe02708 100644
--- a/test/tarantool-tests/lj-1234-err-in-record-concat.test.lua
+++ b/test/tarantool-tests/lj-1234-err-in-record-concat.test.lua
@@ -12,12 +12,13 @@ test:plan(2)
jit.opt.start('hotloop=1')
-local __concat = function(v1, v2)
- return tostring(v1) .. tostring(v2)
+local __concat = function()
+ return ''
end
-- Need to use metamethod call in the concat recording.
-debug.setmetatable(nil, {
+-- Let's use a table with the metamethod.
+local concatable_t = setmetatable({}, {
__concat = __concat,
})
@@ -31,7 +32,7 @@ local function test_concat_p()
-- `lj_record_ret()` restore the Lua stack (before the patch),
-- it becomes unbalanced after the instruction recording
-- attempt.
- local _ = {} .. (nil .. nil)
+ local _ = {} .. (concatable_t .. concatable_t)
end
end
===================================================================
> > + __concat = __concat,
> > +})
> > +
<snipped>
--
Best regards,
Sergey Kaplun
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit] Restore state when recording __concat metamethod throws an error.
2024-09-13 15:27 ` Sergey Kaplun via Tarantool-patches
@ 2024-09-13 15:48 ` Sergey Kaplun via Tarantool-patches
2024-09-17 6:33 ` Sergey Bronnikov via Tarantool-patches
2024-09-23 6:30 ` Maxim Kokryashkin via Tarantool-patches
0 siblings, 2 replies; 7+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2024-09-13 15:48 UTC (permalink / raw)
To: Sergey Bronnikov, tarantool-patches
One more minor update below.
On 13.09.24, Sergey Kaplun via Tarantool-patches wrote:
> Hi, Sergey!
> Thanks for the review!
>
> On 13.09.24, Sergey Bronnikov wrote:
> > Hi, Sergey,
> >
> > thanks for the patch! LGTM with a minor comment. See below.
> >
> > On 21.08.2024 11:23, Sergey Kaplun wrote:
> >
> >
> > <snipped>
> >
> > > +-- Need to use metamethod call in the concat recording.
> > > +debug.setmetatable(nil, {
> > I propose to add a comment that explain why `nil` is used here.
>
> I've removed the nil usage to avoid confusion.
> Branch is rebased on the current master and force-pushed.
> ===================================================================
> diff --git a/test/tarantool-tests/lj-1234-err-in-record-concat.test.lua b/test/tarantool-tests/lj-1234-err-in-record-concat.test.lua
> index 9abaeba5..4fe02708 100644
> --- a/test/tarantool-tests/lj-1234-err-in-record-concat.test.lua
> +++ b/test/tarantool-tests/lj-1234-err-in-record-concat.test.lua
> @@ -12,12 +12,13 @@ test:plan(2)
>
> jit.opt.start('hotloop=1')
>
> -local __concat = function(v1, v2)
> - return tostring(v1) .. tostring(v2)
> +local __concat = function()
> + return ''
> end
>
> -- Need to use metamethod call in the concat recording.
> -debug.setmetatable(nil, {
> +-- Let's use a table with the metamethod.
> +local concatable_t = setmetatable({}, {
> __concat = __concat,
> })
>
> @@ -31,7 +32,7 @@ local function test_concat_p()
> -- `lj_record_ret()` restore the Lua stack (before the patch),
> -- it becomes unbalanced after the instruction recording
> -- attempt.
> - local _ = {} .. (nil .. nil)
> + local _ = {} .. (concatable_t .. concatable_t)
> end
> end
>
> ===================================================================
Added more verbose description:
===================================================================
diff --git a/test/tarantool-tests/lj-1234-err-in-record-concat.test.lua b/test/tarantool-tests/lj-1234-err-in-record-concat.test.lua
index 4fe02708..75736e74 100644
--- a/test/tarantool-tests/lj-1234-err-in-record-concat.test.lua
+++ b/test/tarantool-tests/lj-1234-err-in-record-concat.test.lua
@@ -17,7 +17,8 @@ local __concat = function()
end
-- Need to use metamethod call in the concat recording.
--- Let's use a table with the metamethod.
+-- We may use any object with a metamethod, but let's use a table
+-- as the most common one.
local concatable_t = setmetatable({}, {
__concat = __concat,
})
===================================================================
>
> > > + __concat = __concat,
> > > +})
> > > +
>
> <snipped>
>
> --
> Best regards,
> Sergey Kaplun
--
Best regards,
Sergey Kaplun
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit] Restore state when recording __concat metamethod throws an error.
2024-09-13 15:48 ` Sergey Kaplun via Tarantool-patches
@ 2024-09-17 6:33 ` Sergey Bronnikov via Tarantool-patches
2024-09-23 6:30 ` Maxim Kokryashkin via Tarantool-patches
1 sibling, 0 replies; 7+ messages in thread
From: Sergey Bronnikov via Tarantool-patches @ 2024-09-17 6:33 UTC (permalink / raw)
To: Sergey Kaplun, tarantool-patches
[-- Attachment #1: Type: text/plain, Size: 2838 bytes --]
Hi, Sergey,
thanks for fixes! LGTM
On 13.09.2024 18:48, Sergey Kaplun wrote:
> One more minor update below.
>
> On 13.09.24, Sergey Kaplun via Tarantool-patches wrote:
>> Hi, Sergey!
>> Thanks for the review!
>>
>> On 13.09.24, Sergey Bronnikov wrote:
>>> Hi, Sergey,
>>>
>>> thanks for the patch! LGTM with a minor comment. See below.
>>>
>>> On 21.08.2024 11:23, Sergey Kaplun wrote:
>>>
>>>
>>> <snipped>
>>>
>>>> +-- Need to use metamethod call in the concat recording.
>>>> +debug.setmetatable(nil, {
>>> I propose to add a comment that explain why `nil` is used here.
>> I've removed the nil usage to avoid confusion.
>> Branch is rebased on the current master and force-pushed.
>> ===================================================================
>> diff --git a/test/tarantool-tests/lj-1234-err-in-record-concat.test.lua b/test/tarantool-tests/lj-1234-err-in-record-concat.test.lua
>> index 9abaeba5..4fe02708 100644
>> --- a/test/tarantool-tests/lj-1234-err-in-record-concat.test.lua
>> +++ b/test/tarantool-tests/lj-1234-err-in-record-concat.test.lua
>> @@ -12,12 +12,13 @@ test:plan(2)
>>
>> jit.opt.start('hotloop=1')
>>
>> -local __concat = function(v1, v2)
>> - return tostring(v1) .. tostring(v2)
>> +local __concat = function()
>> + return ''
>> end
>>
>> -- Need to use metamethod call in the concat recording.
>> -debug.setmetatable(nil, {
>> +-- Let's use a table with the metamethod.
>> +local concatable_t = setmetatable({}, {
>> __concat = __concat,
>> })
>>
>> @@ -31,7 +32,7 @@ local function test_concat_p()
>> -- `lj_record_ret()` restore the Lua stack (before the patch),
>> -- it becomes unbalanced after the instruction recording
>> -- attempt.
>> - local _ = {} .. (nil .. nil)
>> + local _ = {} .. (concatable_t .. concatable_t)
>> end
>> end
>>
>> ===================================================================
> Added more verbose description:
>
> ===================================================================
> diff --git a/test/tarantool-tests/lj-1234-err-in-record-concat.test.lua b/test/tarantool-tests/lj-1234-err-in-record-concat.test.lua
> index 4fe02708..75736e74 100644
> --- a/test/tarantool-tests/lj-1234-err-in-record-concat.test.lua
> +++ b/test/tarantool-tests/lj-1234-err-in-record-concat.test.lua
> @@ -17,7 +17,8 @@ local __concat = function()
> end
>
> -- Need to use metamethod call in the concat recording.
> --- Let's use a table with the metamethod.
> +-- We may use any object with a metamethod, but let's use a table
> +-- as the most common one.
> local concatable_t = setmetatable({}, {
> __concat = __concat,
> })
> ===================================================================
>
>>>> + __concat = __concat,
>>>> +})
>>>> +
>> <snipped>
>>
>> --
>> Best regards,
>> Sergey Kaplun
[-- Attachment #2: Type: text/html, Size: 3883 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit] Restore state when recording __concat metamethod throws an error.
2024-09-13 15:48 ` Sergey Kaplun via Tarantool-patches
2024-09-17 6:33 ` Sergey Bronnikov via Tarantool-patches
@ 2024-09-23 6:30 ` Maxim Kokryashkin via Tarantool-patches
1 sibling, 0 replies; 7+ messages in thread
From: Maxim Kokryashkin via Tarantool-patches @ 2024-09-23 6:30 UTC (permalink / raw)
To: Sergey Kaplun via Tarantool-patches
Hi, Sergey!
Thanks for the patch!
LGTM
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit] Restore state when recording __concat metamethod throws an error.
2024-08-21 8:23 [Tarantool-patches] [PATCH luajit] Restore state when recording __concat metamethod throws an error Sergey Kaplun via Tarantool-patches
2024-09-13 13:07 ` Sergey Bronnikov via Tarantool-patches
@ 2024-10-18 15:14 ` Sergey Kaplun via Tarantool-patches
1 sibling, 0 replies; 7+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2024-10-18 15:14 UTC (permalink / raw)
To: Maxim Kokryashkin, Sergey Bronnikov; +Cc: tarantool-patches
I've applied the patch into all long-term branches in tarantool/luajit
and bumped a new version in master [1], release/3.2 [2] and
release/2.11 [3].
[1]: https://github.com/tarantool/tarantool/pull/10712
[2]: https://github.com/tarantool/tarantool/pull/10713
[3]: https://github.com/tarantool/tarantool/pull/10714
--
Best regards,
Sergey Kaplun
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2024-10-18 15:14 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-08-21 8:23 [Tarantool-patches] [PATCH luajit] Restore state when recording __concat metamethod throws an error Sergey Kaplun via Tarantool-patches
2024-09-13 13:07 ` Sergey Bronnikov via Tarantool-patches
2024-09-13 15:27 ` Sergey Kaplun via Tarantool-patches
2024-09-13 15:48 ` Sergey Kaplun via Tarantool-patches
2024-09-17 6:33 ` Sergey Bronnikov via Tarantool-patches
2024-09-23 6:30 ` Maxim Kokryashkin via Tarantool-patches
2024-10-18 15:14 ` Sergey Kaplun via Tarantool-patches
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox