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

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

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

Hi, Sergey,

thanks for the review! See my comments below.

Sergey

On 6/6/26 10:14, Sergey Kaplun via Tarantool-patches wrote:
> Hi, Sergey!
> Thanks for the patch!
> LGTM, with minor ignorable comments below.
>
> On 05.06.26, Sergey Bronnikov wrote:
>> From: Mike Pall <mike>
>>
>> Reported by Sergey Bronnikov.
>>
>> (cherry picked from commit 86d414f5cae062b06998ec66b0696a47d4f6a0f0)
>>
>> The function `lj_cf_os_time()` calculates `tm_mon` and `tm_year`
>> values using `get_field()` and when the helper function returns
>> a negative value the resulted values may be negative as well. This
>> is an undefined behaviour (signed integer overflow). The patch fixes
>> that by adding a cast for the resulted value to returned by
> Typo: s/to returned/to be returned/
Fixed.
>
>> `get_field()`.
> I am suggesting rephrasing this paragraph a bit to make it more clear:
>
> | The function `lj_cf_os_time()` calculates the `tm_mon` and `tm_year`
> | values using `get_field()`. If this helper function returns a negative
> | result, subtraction may lead to undefined behavior due to signed integer
> | overflow. The patch fixes the issue by adding a cast to the value
> | returned by `get_field()`.
>
> Feel free to ignore.
Updated.
>
>> Sergey Bronnikov:
>> * added the description and the test for the problem
>>
>> Part of tarantool/tarantool#12480
>> ---
>>
>> Branch:https://github.com/tarantool/luajit/tree/ligurio/lj-1454-ub-os-time
>>
>> Related issues:
>>
>> *https://github.com/tarantool/tarantool/issues/12480
>> *https://github.com/LuaJIT/LuaJIT/issues/1454
>>
>>   src/lib_os.c                                  |  4 ++--
>>   test/tarantool-tests/lj-1454-os-time.test.lua | 19 +++++++++++++++++++
>>   2 files changed, 21 insertions(+), 2 deletions(-)
>>   create mode 100644 test/tarantool-tests/lj-1454-os-time.test.lua
>>
>> diff --git a/src/lib_os.c b/src/lib_os.c
>> index ffbc3fdc..0feb0d47 100644
>> --- a/src/lib_os.c
>> +++ b/src/lib_os.c
> <snipped>
>
>> diff --git a/test/tarantool-tests/lj-1454-os-time.test.lua b/test/tarantool-tests/lj-1454-os-time.test.lua
>> new file mode 100644
>> index 00000000..2a48750c
>> --- /dev/null
>> +++ b/test/tarantool-tests/lj-1454-os-time.test.lua
> I prefer the <lj-1454-ub-os-time.test.lua> name (as you named the
> branch). Feel free to ignore.

Updated:

$ git show --name-only

<snipped>
src/lib_os.c
test/tarantool-tests/lj-1454-ub-os-time.test.lua

>> @@ -0,0 +1,19 @@
>> +local tap = require('tap')
>> +
>> +-- The test file to demonstrate UBSan warning for `os.time()` with
>> +-- a huge indices value for month and/or year.
> s/a huge indices value/huge negative index values/

Updated:

--- a/test/tarantool-tests/lj-1454-ub-os-time.test.lua
+++ b/test/tarantool-tests/lj-1454-ub-os-time.test.lua
@@ -1,7 +1,7 @@
  local tap = require('tap')

  -- The test file to demonstrate UBSan warning for `os.time()` with
--- a huge indices value for month and/or year.
+-- huge negative index values for month and/or year.
  -- See also: https://github.com/LuaJIT/LuaJIT/issues/1454.
  local test = tap.test('lj-1454-os-time')


>
>> +-- See also:https://github.com/LuaJIT/LuaJIT/issues/1454.
>> +local test = tap.test('lj-1454-os-time')
> I prefer the lj-1454-ub-os-time name (as you named the branch). Feel
> free to ignore.

Updated:

  -- See also: https://github.com/LuaJIT/LuaJIT/issues/1454.
-local test = tap.test('lj-1454-os-time')
+local test = tap.test('lj-1454-ub-os-time')

test:plan(1)

>
>> +
>> +test:plan(1)
>> +
>> +local INT_MIN = -2 ^ 31
>> +
>> +local cur_time = os.time({
>> +  day = 1,
>> +  month = INT_MIN,
>> +  year = INT_MIN,
>> +})
>> +test:is(cur_time, nil, 'os.time() with INT_MIN')
>> +
>> +test:done(true)
>> -- 
>> 2.43.0
>>

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