<!DOCTYPE html>
<html data-lt-installed="true">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body style="padding-bottom: 1px;">
<p>Hi, Sergey,</p>
<p>thanks for the review! See my comments below.</p>
<p>Sergey</p>
<div class="moz-cite-prefix">On 6/6/26 10:14, Sergey Kaplun via
Tarantool-patches wrote:<br>
</div>
<blockquote type="cite" cite="mid:aiPI3GBhizBPSpLk@root">
<pre wrap="" class="moz-quote-pre">Hi, Sergey!
Thanks for the patch!
LGTM, with minor ignorable comments below.
On 05.06.26, Sergey Bronnikov wrote:
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">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
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
Typo: s/to returned/to be returned/</pre>
</blockquote>
Fixed.
<blockquote type="cite" cite="mid:aiPI3GBhizBPSpLk@root">
<pre wrap="" class="moz-quote-pre">
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">`get_field()`.
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
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.</pre>
</blockquote>
Updated.
<blockquote type="cite" cite="mid:aiPI3GBhizBPSpLk@root">
<pre wrap="" class="moz-quote-pre">
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">
Sergey Bronnikov:
* added the description and the test for the problem
Part of tarantool/tarantool#12480
---
Branch: <a class="moz-txt-link-freetext" href="https://github.com/tarantool/luajit/tree/ligurio/lj-1454-ub-os-time">https://github.com/tarantool/luajit/tree/ligurio/lj-1454-ub-os-time</a>
Related issues:
* <a class="moz-txt-link-freetext" href="https://github.com/tarantool/tarantool/issues/12480">https://github.com/tarantool/tarantool/issues/12480</a>
* <a class="moz-txt-link-freetext" href="https://github.com/LuaJIT/LuaJIT/issues/1454">https://github.com/LuaJIT/LuaJIT/issues/1454</a>
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
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
<snipped>
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">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
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
I prefer the <lj-1454-ub-os-time.test.lua> name (as you named the
branch). Feel free to ignore.
</pre>
</blockquote>
<p>Updated:</p>
<p>$ git show --name-only <br>
</p>
<p><snipped><br>
src/lib_os.c<br>
test/tarantool-tests/lj-1454-ub-os-time.test.lua<br>
<br>
</p>
<blockquote type="cite" cite="mid:aiPI3GBhizBPSpLk@root">
<pre wrap="" class="moz-quote-pre">
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">@@ -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.
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
s/a huge indices value/huge negative index values/</pre>
</blockquote>
<p>Updated:</p>
<p>--- a/test/tarantool-tests/lj-1454-ub-os-time.test.lua<br>
+++ b/test/tarantool-tests/lj-1454-ub-os-time.test.lua<br>
@@ -1,7 +1,7 @@<br>
local tap = require('tap')<br>
<br>
-- The test file to demonstrate UBSan warning for `os.time()`
with<br>
--- a huge indices value for month and/or year.<br>
+-- huge negative index values for month and/or year.<br>
-- See also: <a class="moz-txt-link-freetext" href="https://github.com/LuaJIT/LuaJIT/issues/1454">https://github.com/LuaJIT/LuaJIT/issues/1454</a>.<br>
local test = tap.test('lj-1454-os-time')<br>
<br>
<br>
</p>
<blockquote type="cite" cite="mid:aiPI3GBhizBPSpLk@root">
<pre wrap="" class="moz-quote-pre">
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">+-- See also: <a class="moz-txt-link-freetext" href="https://github.com/LuaJIT/LuaJIT/issues/1454">https://github.com/LuaJIT/LuaJIT/issues/1454</a>.
+local test = tap.test('lj-1454-os-time')
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
I prefer the lj-1454-ub-os-time name (as you named the branch). Feel
free to ignore.</pre>
</blockquote>
<p>Updated:</p>
<p> -- See also: <a class="moz-txt-link-freetext" href="https://github.com/LuaJIT/LuaJIT/issues/1454">https://github.com/LuaJIT/LuaJIT/issues/1454</a>.<br>
-local test = tap.test('lj-1454-os-time')<br>
+local test = tap.test('lj-1454-ub-os-time')<br>
<br>
<a class="moz-txt-link-freetext" href="test:plan(1)">test:plan(1)</a><br>
<br>
</p>
<blockquote type="cite" cite="mid:aiPI3GBhizBPSpLk@root">
<pre wrap="" class="moz-quote-pre">
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">+
+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
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
</pre>
</blockquote>
</body>
<lt-container></lt-container>
</html>