Re: [Tarantool-patches] [PATCH luajit] Prevent false positive sanitizer warning in unpack().

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

Hi, Sergey!

thanks for the review! Fixes were applied and branch was force-pushed.

Sergey

On 15:30 Mon 30 Mar , Sergey Kaplun via Tarantool-patches wrote:
>Hi, Sergey!
>Thanks for the patch!
>LGTM, after fixing my minor comments below.
>
>On 30.03.26, Sergey Bronnikov wrote:
>> From: Mike Pall <mike>
>>
>> Reported by Sergey Bronnikov.
>>
>> (cherry picked from commit f322ecb51e3cea06683cc201e8ce224ec42fdab8)
>>
>> The UndefinedBehaviour sanitizer produce a runtime warning when
>
>Typo: s/produce/produces/

Fixed, thanks!

>
>> INT_MAX is passed to `unpack()` as index `j`. This happens because
>
>Typo: s/j/i/

Right, fixed!
>
>> `i` in lj_cf_unpack() was incremented before the checking loop
>> invariant and this could to a signed integer overflo. The patch
>
>Typo: s/could to/leads to/
>Typo: s/overflo/overflow/

Fixed.
>
>> fixes the issue by moving a loop invariant to a loop body.
>
>Minor: It is rather "by moving index incrementation in the separate
>statement from the loop invariant check".
>
Updated.

>>
>> Sergey Bronnikov:
>> * added the description and the test for the problem
>>
>> Part of tarantool/tarantool#12134
>> ---
>>
>> Branch: https://github.com/tarantool/luajit/tree/ligurio/lj-1450-unpack-ub
>> Related issues:
>> * https://github.com/LuaJIT/LuaJIT/issues/1450
>> * https://github.com/tarantool/tarantool/issues/12134
>>
>>  src/lib_base.c                                |  4 ++-
>>  .../lj-1450-unpack-huge-indices.test.lua      | 28 +++++++++++++++++++
>>  2 files changed, 31 insertions(+), 1 deletion(-)
>>  create mode 100644 test/tarantool-tests/lj-1450-unpack-huge-indices.test.lua
>>
>> diff --git a/src/lib_base.c b/src/lib_base.c
>> index a5b907de..714dc5bf 100644
>> --- a/src/lib_base.c
>> +++ b/src/lib_base.c
>
><snipped>
>
>> diff --git a/test/tarantool-tests/lj-1450-unpack-huge-indices.test.lua b/test/tarantool-tests/lj-1450-unpack-huge-indices.test.lua
>> new file mode 100644
>> index 00000000..1a09e273
>> --- /dev/null
>> +++ b/test/tarantool-tests/lj-1450-unpack-huge-indices.test.lua
>> @@ -0,0 +1,28 @@
>> +local tap = require('tap')
>> +
>> +-- The test file to demonstrate UBSan warning for `unpack()` with
>> +-- a huge indices value.
>> +-- See also: https://github.com/LuaJIT/LuaJIT/issues/1450.
>> +local test = tap.test('lj-1450-unpack-huge-indices')
>> +
>> +test:plan(4)
>> +
>> +local INT_MAX = 2 ^ 31 - 1
>> +
>> +-- The first test check the UBSan runtime error. The assertions
>
>Typo: s/check/checks/

Fixed.
>
>> +-- were added just to be sure we don't change the behaviour.
>> +-- The second test additionally check a correct behaviour for
>
>Typo: s/check/checks/

Fixed.

  local INT_MAX = 2 ^ 31 - 1
  
--- The first test check the UBSan runtime error. The assertions
+-- The first test checks the UBSan runtime error. The assertions
  -- were added just to be sure we don't change the behaviour.
--- The second test additionally check a correct behaviour for
+-- The second test additionally checks a correct behaviour for
  -- a <maximum - 1> value.
  local tbl = {
    [INT_MAX] = INT_MAX,
    [INT_MAX - 1] = INT_MAX - 1,
  }


>
>> +-- a <maximum - 1> value.
>> +local tbl = {
>> +  [INT_MAX] = INT_MAX,
>> +  [INT_MAX - 1] = INT_MAX - 1,
>
>I suggest creating for this value the `INT_MAX_M1` variable.
>Feel free to ignore.

IMHO this variable name is less clear than using INT_MAX - 1.
Left without changes.
>
>> +}
>> +local status, res = pcall(unpack, tbl, INT_MAX, INT_MAX)
>
>`pcall()`-s look excessive here since we always expect only 1 element to
>be unpacked, so we obviously have enough space for it. Let's drop them.
>

Updated:

  local tbl = {
    [INT_MAX] = INT_MAX,
    [INT_MAX - 1] = INT_MAX - 1,
  }
-local status, res = pcall(unpack, tbl, INT_MAX, INT_MAX)
-test:ok(status, 'unpack with INT_MAX: correct status')
+local res = unpack(tbl, INT_MAX, INT_MAX)
  test:is(res, INT_MAX, 'unpack with INT_MAX: correct result')
  
-status, res = pcall(unpack, tbl, INT_MAX - 1, INT_MAX - 1)
-test:ok(status, 'unpack with INT_MAX - 1: correct status')
+res = unpack(tbl, INT_MAX - 1, INT_MAX - 1)
  test:is(res, INT_MAX - 1, 'unpack with INT_MAX - 1: correct result')
  
  test:done(true)


>> +test:ok(status, 'unpack with INT_MAX: correct status')
>> +test:is(res, INT_MAX, 'unpack with INT_MAX: correct result')
>> +
>> +status, res = pcall(unpack, tbl, INT_MAX - 1, INT_MAX - 1)
>> +test:ok(status, 'unpack with INT_MAX - 1: correct status')
>> +test:is(res, INT_MAX - 1, 'unpack with INT_MAX - 1: correct result')
>> +
>> +test:done(true)
>> --
>> 2.43.0
>>
>
>-- 
>Best regards,
>Sergey Kaplun
>

-- 
sergeyb@