Re: [Tarantool-patches] [PATCH luajit] FFI: Always fall back to metamethods for cdata length/concat.

Re: [Tarantool-patches] [PATCH luajit] FFI: Always fall back to metamethods for cdata length/concat.

[Sergey Kaplun via Tarantool-patches]

Hi, Maxim!

Thanks for the review!

On 08.09.22, Maxim Kokryashkin wrote:
> 
> Hi, Sergey!
> Thanks for the patch!
> Please consider the single comment below:
>  
> <snipped>
> >
> > src/lj_carith.c | 3 +--
> > src/lj_crecord.c | 6 ++++--
> > test/tarantool-tests/fix-cdata-concat.test.lua | 15 +++++++++++++++
> > 3 files changed, 20 insertions(+), 4 deletions(-)
> > create mode 100644 test/tarantool-tests/fix-cdata-concat.test.lua
> Besides everything that Sergos has already said, I wonder why you didn't include any changes in "lj_carith.h" and "lib_ffi.c" from the original patch?

They were annihilated during the cherry-pick. See the following changes
in the merge commit:
https://github.com/LuaJIT/LuaJIT/commit/d5e12d5174720fbd3c5fad4e02da5850b8433057

>  
> <snipped>
> --
> Best regards,
> Maxim Kokryashkin

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit] FFI: Always fall back to metamethods for cdata length/concat.

[Maxim Kokryashkin via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 1099 bytes --]


Hi, Sergey!
Thanks for the clarification!
LGTM
 
--
Best regards,
Maxim Kokryashkin
 
  
>Понедельник, 12 сентября 2022, 11:10 +03:00 от Sergey Kaplun <skaplun@tarantool.org>:
> 
>Hi, Maxim!
>
>Thanks for the review!
>
>On 08.09.22, Maxim Kokryashkin wrote:
>>
>> Hi, Sergey!
>> Thanks for the patch!
>> Please consider the single comment below:
>>  
>> <snipped>
>> >
>> > src/lj_carith.c | 3 +--
>> > src/lj_crecord.c | 6 ++++--
>> > test/tarantool-tests/fix-cdata-concat.test.lua | 15 +++++++++++++++
>> > 3 files changed, 20 insertions(+), 4 deletions(-)
>> > create mode 100644 test/tarantool-tests/fix-cdata-concat.test.lua
>> Besides everything that Sergos has already said, I wonder why you didn't include any changes in "lj_carith.h" and "lib_ffi.c" from the original patch?
>
>They were annihilated during the cherry-pick. See the following changes
>in the merge commit:
>https://github.com/LuaJIT/LuaJIT/commit/d5e12d5174720fbd3c5fad4e02da5850b8433057
>
>>  
>> <snipped>
>> --
>> Best regards,
>> Maxim Kokryashkin
>
>--
>Best regards,
>Sergey Kaplun
 

[-- Attachment #2: Type: text/html, Size: 1856 bytes --]

[Tarantool-patches] [PATCH luajit] FFI: Always fall back to metamethods for cdata length/concat.

[Sergey Kaplun via Tarantool-patches]

From: Mike Pall <mike>

Thanks to Egor Skriptunoff.

(cherry picked from commit cc4bbec483d3f3250b519ccb7cc22f1a8e6fe6f0)

When user tries to concatenate 2 cdata objects without declared
metamethod, the assertion is raised in `carith_int64()`, due to
concatenation operation is not specified and default (assert) branch is
taken.

This patch forcifies usage of metamethod for concatenation on cdata
objects. Also, as far as the behaviour for length operation is the same,
the `lj_carith_len()` routine is removed, its call is replaced with
`ffi_arith()`.

Sergey Kaplun:
* added the description and the test for the problem

Part of tarantool/tarantool#7230
---

Issue: https://github.com/tarantool/tarantool/issues/7230
Branch: https://github.com/tarantool/luajit/tree/skaplun/gh-noticket-cdata-ll-concat-full-ci
PR: https://github.com/tarantool/tarantool/pull/7598
ML: https://www.freelists.org/post/luajit/cdata-concatenation

 src/lj_carith.c                                |  3 +--
 src/lj_crecord.c                               |  6 ++++--
 test/tarantool-tests/fix-cdata-concat.test.lua | 15 +++++++++++++++
 3 files changed, 20 insertions(+), 4 deletions(-)
 create mode 100644 test/tarantool-tests/fix-cdata-concat.test.lua

diff --git a/src/lj_carith.c b/src/lj_carith.c
index 218abd26..04c18054 100644
--- a/src/lj_carith.c
+++ b/src/lj_carith.c
@@ -265,7 +265,7 @@ int lj_carith_op(lua_State *L, MMS mm)
 {
   CTState *cts = ctype_cts(L);
   CDArith ca;
-  if (carith_checkarg(L, cts, &ca)) {
+  if (carith_checkarg(L, cts, &ca) && mm != MM_len && mm != MM_concat) {
     if (carith_int64(L, cts, &ca, mm) || carith_ptr(L, cts, &ca, mm)) {
       copyTV(L, &G(L)->tmptv2, L->top-1);  /* Remember for trace recorder. */
       return 1;
@@ -347,7 +347,6 @@ uint64_t lj_carith_check64(lua_State *L, int narg, CTypeID *id)
   }
 }
 
-
 /* -- 64 bit integer arithmetic helpers ----------------------------------- */
 
 #if LJ_32 && LJ_HASJIT
diff --git a/src/lj_crecord.c b/src/lj_crecord.c
index 0d7b71f0..3d562d9a 100644
--- a/src/lj_crecord.c
+++ b/src/lj_crecord.c
@@ -1546,8 +1546,10 @@ void LJ_FASTCALL recff_cdata_arith(jit_State *J, RecordFFData *rd)
   }
   {
     TRef tr;
-    if (!(tr = crec_arith_int64(J, sp, s, (MMS)rd->data)) &&
-	!(tr = crec_arith_ptr(J, sp, s, (MMS)rd->data)) &&
+    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)))) &&
 	!(tr = crec_arith_meta(J, sp, s, cts, rd)))
       return;
     J->base[0] = tr;
diff --git a/test/tarantool-tests/fix-cdata-concat.test.lua b/test/tarantool-tests/fix-cdata-concat.test.lua
new file mode 100644
index 00000000..aaeb36fa
--- /dev/null
+++ b/test/tarantool-tests/fix-cdata-concat.test.lua
@@ -0,0 +1,15 @@
+local tap = require('tap')
+
+-- Test file to demonstrate incorrect behaviour of cdata
+-- concatenation in LuaJIT.
+-- See also
+-- https://www.freelists.org/post/luajit/cdata-concatenation.
+local test = tap.test('cdata-concat')
+test:plan(1)
+
+local r, e = pcall(function()
+  return 1LL .. 2LL
+end)
+test:ok(not r and e:match('attempt to concatenate'), 'cdata concatenation')
+
+os.exit(test:check() and 0 or 1)
-- 
2.34.1

Re: [Tarantool-patches] [PATCH luajit] FFI: Always fall back to metamethods for cdata length/concat.

[sergos via Tarantool-patches]

Hi!
Thanks for the patch!

Just a minor message update and a test extension - otherwise LGTM.

Sergos

> On 23 Aug 2022, at 17:27, Sergey Kaplun <skaplun@tarantool.org> wrote:
> 
> From: Mike Pall <mike>
> 
> Thanks to Egor Skriptunoff.
> 
> (cherry picked from commit cc4bbec483d3f3250b519ccb7cc22f1a8e6fe6f0)
> 
> When user tries to concatenate 2 cdata objects without declared
> metamethod, the assertion is raised in `carith_int64()`, due to
> concatenation operation is not specified and default (assert) branch is
> taken.

The above doesn’t explain the behavior - the default branch leads to what?
Neither it explains the expected behavior.

> 
> This patch forcifies usage of metamethod for concatenation on cdata
> objects. Also, as far as the behaviour for length operation is the same,
> the `lj_carith_len()` routine is removed, its call is replaced with
> `ffi_arith()`.
> 
> Sergey Kaplun:
> * added the description and the test for the problem
> 
> Part of tarantool/tarantool#7230
> ---
> 
> Issue: https://github.com/tarantool/tarantool/issues/7230
> Branch: https://github.com/tarantool/luajit/tree/skaplun/gh-noticket-cdata-ll-concat-full-ci
> PR: https://github.com/tarantool/tarantool/pull/7598
> ML: https://www.freelists.org/post/luajit/cdata-concatenation
> 
> src/lj_carith.c                                |  3 +--
> src/lj_crecord.c                               |  6 ++++--
> test/tarantool-tests/fix-cdata-concat.test.lua | 15 +++++++++++++++
> 3 files changed, 20 insertions(+), 4 deletions(-)
> create mode 100644 test/tarantool-tests/fix-cdata-concat.test.lua
> 
> diff --git a/src/lj_carith.c b/src/lj_carith.c
> index 218abd26..04c18054 100644
> --- a/src/lj_carith.c
> +++ b/src/lj_carith.c
> @@ -265,7 +265,7 @@ int lj_carith_op(lua_State *L, MMS mm)
> {
>   CTState *cts = ctype_cts(L);
>   CDArith ca;
> -  if (carith_checkarg(L, cts, &ca)) {
> +  if (carith_checkarg(L, cts, &ca) && mm != MM_len && mm != MM_concat) {
>     if (carith_int64(L, cts, &ca, mm) || carith_ptr(L, cts, &ca, mm)) {
>       copyTV(L, &G(L)->tmptv2, L->top-1);  /* Remember for trace recorder. */
>       return 1;
> @@ -347,7 +347,6 @@ uint64_t lj_carith_check64(lua_State *L, int narg, CTypeID *id)
>   }
> }
> 
> -
> /* -- 64 bit integer arithmetic helpers ----------------------------------- */
> 
> #if LJ_32 && LJ_HASJIT
> diff --git a/src/lj_crecord.c b/src/lj_crecord.c
> index 0d7b71f0..3d562d9a 100644
> --- a/src/lj_crecord.c
> +++ b/src/lj_crecord.c
> @@ -1546,8 +1546,10 @@ void LJ_FASTCALL recff_cdata_arith(jit_State *J, RecordFFData *rd)
>   }
>   {
>     TRef tr;
> -    if (!(tr = crec_arith_int64(J, sp, s, (MMS)rd->data)) &&
> -	!(tr = crec_arith_ptr(J, sp, s, (MMS)rd->data)) &&
> +    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)))) &&
> 	!(tr = crec_arith_meta(J, sp, s, cts, rd)))
>       return;
>     J->base[0] = tr;
> diff --git a/test/tarantool-tests/fix-cdata-concat.test.lua b/test/tarantool-tests/fix-cdata-concat.test.lua
> new file mode 100644
> index 00000000..aaeb36fa
> --- /dev/null
> +++ b/test/tarantool-tests/fix-cdata-concat.test.lua
> @@ -0,0 +1,15 @@
> +local tap = require('tap')
> +
> +-- Test file to demonstrate incorrect behaviour of cdata
> +-- concatenation in LuaJIT.
> +-- See also
> +-- https://www.freelists.org/post/luajit/cdata-concatenation.
> +local test = tap.test('cdata-concat')
> +test:plan(1)
> +
> +local r, e = pcall(function()
> +  return 1LL .. 2LL
> +end)
> +test:ok(not r and e:match('attempt to concatenate'), 'cdata concatenation')

As with programmer’s joke about full and empty glass - let’s have a second case
for the existent metamethod? 

> +
> +os.exit(test:check() and 0 or 1)
> -- 
> 2.34.1
> 

Re: [Tarantool-patches] [PATCH luajit] FFI: Always fall back to metamethods for cdata length/concat.

[Sergey Kaplun via Tarantool-patches]

Hi, Sergos!

Thanks for the review!

On 31.08.22, sergos wrote:
> Hi!
> Thanks for the patch!
> 
> Just a minor message update and a test extension - otherwise LGTM.
> 
> Sergos
> 
> > On 23 Aug 2022, at 17:27, Sergey Kaplun <skaplun@tarantool.org> wrote:
> > 
> > From: Mike Pall <mike>
> > 
> > Thanks to Egor Skriptunoff.
> > 
> > (cherry picked from commit cc4bbec483d3f3250b519ccb7cc22f1a8e6fe6f0)
> > 
> > When user tries to concatenate 2 cdata objects without declared
> > metamethod, the assertion is raised in `carith_int64()`, due to
> > concatenation operation is not specified and default (assert) branch is
> > taken.
> 
> The above doesn’t explain the behavior - the default branch leads to what?
> Neither it explains the expected behavior.

Fixed. The new commit message is the following:

| FFI: Always fall back to metamethods for cdata length/concat.
|
| Thanks to Egor Skriptunoff.
|
| (cherry picked from commit cc4bbec483d3f3250b519ccb7cc22f1a8e6fe6f0)
|
| When user tries to concatenate 2 cdata objects without declared
| metamethod, the assertion is raised in `carith_int64()`, due to
| concatenation operation is not specified and default (assert) branch is
| taken. In non debug mode this leads to returning of new cdata with
| unfilled `cdataptr` content (i.e. random value from memory).
|
| It is not possible to predict, what behavior the user expects in case of
| concatenation of 2 cdata objects. So, the error should be rased, when
| user tries to concatenate these cdata objects without metamethod
| declared.
|
| This patch forcifies usage of metamethod for concatenation on cdata
| objects. Also, as far as the behaviour for length operation is the same,
| the `lj_carith_len()` routine is removed, its call is replaced with
| `ffi_arith()`.
|
| Sergey Kaplun:
| * added the description and the test for the problem
|
| Part of tarantool/tarantool#7230

> 
> > 
> > This patch forcifies usage of metamethod for concatenation on cdata
> > objects. Also, as far as the behaviour for length operation is the same,
> > the `lj_carith_len()` routine is removed, its call is replaced with
> > `ffi_arith()`.
> > 
> > Sergey Kaplun:
> > * added the description and the test for the problem
> > 
> > Part of tarantool/tarantool#7230
> > ---
> > 
> > Issue: https://github.com/tarantool/tarantool/issues/7230
> > Branch: https://github.com/tarantool/luajit/tree/skaplun/gh-noticket-cdata-ll-concat-full-ci
> > PR: https://github.com/tarantool/tarantool/pull/7598
> > ML: https://www.freelists.org/post/luajit/cdata-concatenation
> > 
> > src/lj_carith.c                                |  3 +--
> > src/lj_crecord.c                               |  6 ++++--
> > test/tarantool-tests/fix-cdata-concat.test.lua | 15 +++++++++++++++
> > 3 files changed, 20 insertions(+), 4 deletions(-)
> > create mode 100644 test/tarantool-tests/fix-cdata-concat.test.lua
> > 
> > diff --git a/src/lj_carith.c b/src/lj_carith.c
> > index 218abd26..04c18054 100644
> > --- a/src/lj_carith.c
> > +++ b/src/lj_carith.c

<snipped>

> > diff --git a/src/lj_crecord.c b/src/lj_crecord.c
> > index 0d7b71f0..3d562d9a 100644
> > --- a/src/lj_crecord.c
> > +++ b/src/lj_crecord.c

<snipped>

> > diff --git a/test/tarantool-tests/fix-cdata-concat.test.lua b/test/tarantool-tests/fix-cdata-concat.test.lua
> > new file mode 100644
> > index 00000000..aaeb36fa
> > --- /dev/null
> > +++ b/test/tarantool-tests/fix-cdata-concat.test.lua
> > @@ -0,0 +1,15 @@
> > +local tap = require('tap')
> > +
> > +-- Test file to demonstrate incorrect behaviour of cdata
> > +-- concatenation in LuaJIT.
> > +-- See also
> > +-- https://www.freelists.org/post/luajit/cdata-concatenation.
> > +local test = tap.test('cdata-concat')
> > +test:plan(1)
> > +
> > +local r, e = pcall(function()
> > +  return 1LL .. 2LL
> > +end)
> > +test:ok(not r and e:match('attempt to concatenate'), 'cdata concatenation')
> 
> As with programmer’s joke about full and empty glass - let’s have a second case
> for the existent metamethod? 

Added with the following patch:

===================================================================
diff --git a/test/tarantool-tests/fix-cdata-concat.test.lua b/test/tarantool-tests/fix-cdata-concat.test.lua
index aaeb36fa..df069e58 100644
--- a/test/tarantool-tests/fix-cdata-concat.test.lua
+++ b/test/tarantool-tests/fix-cdata-concat.test.lua
@@ -5,11 +5,17 @@ local tap = require('tap')
 -- See also
 -- https://www.freelists.org/post/luajit/cdata-concatenation.
 local test = tap.test('cdata-concat')
-test:plan(1)
+test:plan(2)

 local r, e = pcall(function()
   return 1LL .. 2LL
 end)
 test:ok(not r and e:match('attempt to concatenate'), 'cdata concatenation')

+-- Check, that concatenation work, when metamethod is defined.
+debug.getmetatable(1LL).__concat = function(a, b)
+  return tostring(a) .. tostring(b)
+end
+test:ok(1LL .. 2LL == '1LL2LL', 'cdata concatenation with defined metamethod')
+
 os.exit(test:check() and 0 or 1)
===================================================================

Branch is force-pushed.

> 
> > +
> > +os.exit(test:check() and 0 or 1)
> > -- 
> > 2.34.1
> > 
> 

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit] FFI: Always fall back to metamethods for cdata length/concat.

[Igor Munkin via Tarantool-patches]

Sergey,

I've checked the patch into all long-term branches in tarantool/luajit
and bumped a new version in master, 2.10 and 1.10.

-- 
Best regards,
IM