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

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

[Sergey Bronnikov via Tarantool-patches]

From: Mike Pall <mike>

Reported by Sergey Bronnikov.

(cherry picked from commit f322ecb51e3cea06683cc201e8ce224ec42fdab8)

The UndefinedBehaviour sanitizer produce a runtime warning when
INT_MAX is passed to `unpack()` as index `j`. This happens because
`i` in lj_cf_unpack() was incremented before the checking loop
invariant and this could to a signed integer overflo. The patch
fixes the issue by moving a loop invariant to a loop body.

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
@@ -241,7 +241,9 @@ LJLIB_CF(unpack)
     } else {
       setnilV(L->top++);
     }
-  } while (i++ < e);
+    if (i >= e) break;
+    i++;
+  } while (1);
   return n;
 }
 
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
+-- were added just to be sure we don't change the behaviour.
+-- The second test additionally check a correct behaviour for
+-- a <maximum - 1> value.
+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')
+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

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

[Sergey Kaplun via Tarantool-patches]

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

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

[Sergey Bronnikov via Tarantool-patches]

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@

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

[Sergey Kaplun via Tarantool-patches]

Sergey,
Thanks for the fixes!
LGTM!

-- 
Best regards,
Sergey Kaplun