Re: [Tarantool-patches] [PATCH luajit] FFI: Prevent sanitizer warning in carith_ptr().

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

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

Hello,

Thanks for review! See my comments.

Fixes are applied and the branch was force-pushed.

Sergey

On 7/27/26 10:39, Sergey Kaplun via Tarantool-patches wrote:
> Hi, Sergey!
> Thanks for the patch!
> Please consider my comments below.
>
> On 24.07.26, Sergey Bronnikov wrote:
>> From: Mike Pall <mike>
>>
>> Reported by Sergey Bronnikov.
>>
>> (cherry picked from commit c3b379bf50c8819c61daa3afd9f21d9ec5708bd5)
>>
> Side note: We may mention that this is a follow-up for the commit
> 78f4de4d7d0c410eeaf75cec5bb178fdb7db1452 ("Avoid negation of signed
> integers in C that may hold INT*_MIN.").

Updated, updated commit message is below:

     FFI: Prevent sanitizer warning in carith_ptr().

     Reported by Sergey Bronnikov.

     (cherry picked from commit c3b379bf50c8819c61daa3afd9f21d9ec5708bd5)

     The Undefined Behaviour Sanitizer [1] produce a warning about
     signed integer overflow in the function carith_ptr(), when idx
     equals INT_MIN in the expression idx = -idx. In practice,
     negating -2147483648 wraps back to itself instead of producing the
     correct positive value, leading to incorrect pointer arithmetic in
     carith_ptr() for MM_sub (pointer subtraction). The patch fixes
     that by converting idx to unsigned (uintptr_t), computes two's
     complement negation via bitwise negation + 1 (safe from overflow
     in unsigned arithmetic), and casts back to ptrdiff_t. This avoids
     signed overflow entirely and correctly negates even the INT_MIN
     edge case.

     The patch follows up the commit 
78f4de4d7d0c410eeaf75cec5bb178fdb7db1452
     ("Avoid negation of signed integers in C that may hold INT*_MIN.").

     [1]: https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html

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

     Part of tarantool/tarantool#12480


>
> Feel free to ignore.
>
>> The Undefined Behaviour Sanitizer [1] produce a warning about
> Typo: s/produce/produces/

Fixed, thanks.

>> signed integer overflow in the function carith_ptr(), when idx
>> equals INT_MIN in the expression idx = -idx. In practice,
>> negating -2147483648 wraps back to itself instead of producing the
> It's actually -9223372036854775808 or -0x8000000000000000 for 64-bit
> architecture. Let's use hexademic notation (or just the `INT_MIN`)
> instead of this number.
replaced the number with INT_MIN
>
>> correct positive value, leading to incorrect pointer arithmetic in
>> carith_ptr() for MM_sub (pointer subtraction). The patch fixes
>> that by converting idx to unsigned (uintptr_t), computes two's
>> complement negation via bitwise negation + 1 (safe from overflow
>> in unsigned arithmetic), and casts back to ptrdiff_t. This avoids
>> signed overflow entirely and correctly negates even the INT_MIN
>> edge case.
>>
>> [1]:https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html
>>
>> Sergey Bronnikov:
>> * added the description and the test for the problem
>>
>> Part of tarantool/tarantool#12480
> Typo: s/12480/12880/
Fixed, thanks!
>
>> ---
>>
>> Branch:https://github.com/tarantool/luajit/tree/ligurio/lj-1459-ub-carith_ptr
>> Related issues:
>> -https://github.com/LuaJIT/LuaJIT/issues/1459
>> -https://github.com/tarantool/tarantool/issues/12880
>>
>>   src/lj_carith.c                                  |  2 +-
>>   .../lj-1459-subtraction-carith.test.lua          | 16 ++++++++++++++++
>>   2 files changed, 17 insertions(+), 1 deletion(-)
>>   create mode 100644 test/tarantool-tests/lj-1459-subtraction-carith.test.lua
>>
>> diff --git a/src/lj_carith.c b/src/lj_carith.c
>> index eb56d552..e971bfc2 100644
>> --- a/src/lj_carith.c
>> +++ b/src/lj_carith.c
> <snipped>
>
>> diff --git a/test/tarantool-tests/lj-1459-subtraction-carith.test.lua b/test/tarantool-tests/lj-1459-subtraction-carith.test.lua
>> new file mode 100644
>> index 00000000..df401a56
>> --- /dev/null
>> +++ b/test/tarantool-tests/lj-1459-subtraction-carith.test.lua
>> @@ -0,0 +1,16 @@
>> +local tap = require('tap')
>> +
>> +-- The test file to demonstrate UBSan warning in carith_ptr().
> Typo: s/UBSan/the UBSan/
> Minor: s/carith_ptr()/`carith_ptr()`/

Updated:

--- a/test/tarantool-tests/lj-1459-subtraction-carith.test.lua
+++ b/test/tarantool-tests/lj-1459-subtraction-carith.test.lua
@@ -1,6 +1,7 @@
  local tap = require('tap')

--- The test file to demonstrate UBSan warning in carith_ptr().
+-- The test file to demonstrate the UBSan warning in
+-- `carith_ptr()`.
  -- See also: https://github.com/LuaJIT/LuaJIT/issues/1459.
  local test = tap.test('lj-1459-subtraction-carith')


>
>> +-- See also:https://github.com/LuaJIT/LuaJIT/issues/1459.
>> +local test = tap.test('lj-1459-subtraction-carith')
>> +
>> +test:plan(2)
>> +
>> +local func = load('_ = nil - 0LL%0')
> Minor: Let's use loadstring instead (I know that they are literally the
> same function in LuaJIT, but it's semantically better to use it for
> loading strings, while `load()` is used for generators).
>
> For Lua 5.1 they are not the same:
>
> | lua5.1 -e 'load"print(1)"()'
> | lua5.1: (command line):1: bad argument #1 to 'load' (function expected, got string)
>
> The `loadstring()` has been deprecated in Lua5.2.
>
>
> This `0LL%0` looks weird. Let's just use the resulting
> `-0x8000000000000000LL` instead.
>
>
> Also, please add the comment that `nil` as the first operand of
> subtraction is required, since it is required to trigger metamethod
> invocation. It successfully passes the argument check since it may be
> considered as NULL ptr for other metamethods.

Updated:

--- a/test/tarantool-tests/lj-1459-subtraction-carith.test.lua
+++ b/test/tarantool-tests/lj-1459-subtraction-carith.test.lua
@@ -7,7 +7,11 @@ local test = tap.test('lj-1459-subtraction-carith')

test:plan(2)

-local func = loadstring('_ = nil - 0LL%0')
+-- The `nil` as the first operand of subtraction is required,
+-- since it is required to trigger metamethod invocation.
+-- It successfully passes the argument check since it may be
+-- considered as NULL ptr for other metamethods.
+local func = loadstring('_ = nil - 0x8000000000000000LL')
  local res, err = pcall(func)

test:is(res, false, 'correct result')

>
>> +local res, err = pcall(func)
>> +
>> +test:is(res, false, 'correct result')
>> +local error_msg = "attempt to perform arithmetic on 'nil' and 'int64_t'"
>> +test:ok(err:match(error_msg), 'error on subtraction')
>> +
>> +test:done(true)
>> -- 
>> 2.43.0
>>

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