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

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

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/

> INT_MAX is passed to `unpack()` as index `j`. This happens because

Typo: s/j/i/

> `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/

> 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".

> 
> 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/

> +-- 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/

> +-- 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.

> +}
> +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.

> +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