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

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

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

Feel free to ignore.

> The Undefined Behaviour Sanitizer [1] produce a warning about

Typo: s/produce/produces/

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

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

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

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

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

-- 
Best regards,
Sergey Kaplun