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

[Evgeniy Temirgaleev via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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

Hi, Sergey! Thanks for the patch!

LGTM

--
Best regards,
Evgeniy Temirgaleev

> 
> From: Sergey Bronnikov via Tarantool-patches <tarantool-patches@dev.tarantool.org
> >
> To: Sergey Kaplun <skaplun@tarantool.org>, Sergey Bronnikov <estetus@gmail.com
> >
> Cc: tarantool-patches@dev.tarantool.org
> Date: Tuesday, July 28, 2026 1:06 PM +03:00
> 
> 
> 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: 10548 bytes --]