[Tarantool-patches] [PATCH luajit] Restore state when recording __concat metamethod throws an error.

[Tarantool-patches] [PATCH luajit] Restore state when recording __concat metamethod throws an error.

[Sergey Kaplun via 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

Re: [Tarantool-patches] [PATCH luajit] Restore state when recording __concat metamethod throws an error.

[Sergey Bronnikov via 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 --]

Re: [Tarantool-patches] [PATCH luajit] Restore state when recording __concat metamethod throws an error.

[Sergey Kaplun via 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

Re: [Tarantool-patches] [PATCH luajit] Restore state when recording __concat metamethod throws an error.

[Sergey Kaplun via 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

Re: [Tarantool-patches] [PATCH luajit] Restore state when recording __concat metamethod throws an error.

[Sergey Bronnikov via 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 --]

Re: [Tarantool-patches] [PATCH luajit] Restore state when recording __concat metamethod throws an error.

[Maxim Kokryashkin via Tarantool-patches]

Hi, Sergey!
Thanks for the patch!
LGTM

Re: [Tarantool-patches] [PATCH luajit] Restore state when recording __concat metamethod throws an error.

[Sergey Kaplun via 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