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

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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