<HTML><BODY><div>Hi!</div><div>Thanks for the fixes!</div><div>LGTM.</div><div data-signature-widget="container"><div data-signature-widget="content"><div>--<br>Best regards,</div><div>Maxim Kokryashkin</div></div></div><div> </div><div> </div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div> <blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_16753663400202190697_BODY">Hi, Maxim!<br><br>Thanks for the review!<br><br>Fixed your comments, branch is force-pushed.<br><br>On 02.02.23, Maxim Kokryashkin wrote:<br>><br>> Hi, Sergey!<br>> Thanks for the patch!<br>> LGTM, except for a few nits below.<br>> > <br>> >>From: Mike Pall <mike><br>> >><br>> >>Thanks to Jesper Lundgren.<br>> >><br>> >>(cherry picked from commit fc63c938b522e147ea728b75f385728bf4a8fc35)<br>> >><br>> >>When `strftime()` returns empty result (for example, for "%p" on some<br>> >>locales or when LuaJIT is built with musl's `strftime()` and format<br>> >>string is invalid), the `os.date()` endless retries to format result<br>> >Typo: s/endless/endlessly/<br>> >Typo: s/format result/format the result/<br><br>Fixed, thanks!<br><br>> >>and reallocates buffer for resulting string. This leads to OOM.<br>> >Typo: s/buffer for resulting/the buffer for the resulting/<br><br>Fixed, thanks!<br><br>> >><br>> >>This patch limits amount of retries by 4.<br>> >Typo: s/amount/the amount/<br><br>Fixed, thanks!<br><br>> >Side note: Why is it exactly 4?<br><br>That's what she said:)<br><br>> >><br>> >>Sergey Kaplun:<br>> >>* added the description and the test for the problem<br>> >><br>> >>Part of tarantool/tarantool#8069<br>> >>---<br>> >><br>> >>PR: <a href="https://github.com/tarantool/tarantool/pull/8237" target="_blank">https://github.com/tarantool/tarantool/pull/8237</a><br>> >>Issues:<br>> >>* <a href="https://github.com/LuaJIT/LuaJIT/issues/463" target="_blank">https://github.com/LuaJIT/LuaJIT/issues/463</a><br>> >>* <a href="https://github.com/tarantool/tarantool/issues/8069" target="_blank">https://github.com/tarantool/tarantool/issues/8069</a><br>> >>Branch: <a href="https://github.com/tarantool/luajit/tree/skaplun/lj-463-os-date-oom-full-ci" target="_blank">https://github.com/tarantool/luajit/tree/skaplun/lj-463-os-date-oom-full-ci</a><br>> >><br>> >> src/lib_os.c | 4 ++--<br>> >> .../lj-463-os-date-oom.test.lua | 19 +++++++++++++++++++<br>> >> 2 files changed, 21 insertions(+), 2 deletions(-)<br>> >> create mode 100644 test/tarantool-tests/lj-463-os-date-oom.test.lua<br>> >><br>> >>diff --git a/src/lib_os.c b/src/lib_os.c<br>> >>index 9e78d49a..ffbc3fdc 100644<br>> >>--- a/src/lib_os.c<br>> >>+++ b/src/lib_os.c<br>> >>@@ -205,12 +205,12 @@ LJLIB_CF(os_date)<br>> >> setboolfield(L, "isdst", stm->tm_isdst);<br>> >> } else if (*s) {<br>> >> SBuf *sb = &G(L)->tmpbuf;<br>> >>- MSize sz = 0;<br>> >>+ MSize sz = 0, retry = 4;<br>> >> const char *q;<br>> >> for (q = s; *q; q++)<br>> >> sz += (*q == '%') ? 30 : 1; /* Overflow doesn't matter. */<br>> >> setsbufL(sb, L);<br>> >>- for (;;) {<br>> >>+ while (retry--) { /* Limit growth for invalid format or empty result. */<br>> >> char *buf = lj_buf_need(sb, sz);<br>> >> size_t len = strftime(buf, sbufsz(sb), s, stm);<br>> >> if (len) {<br>> >>diff --git a/test/tarantool-tests/lj-463-os-date-oom.test.lua b/test/tarantool-tests/lj-463-os-date-oom.test.lua<br>> >>new file mode 100644<br>> >>index 00000000..cce78b6e<br>> >>--- /dev/null<br>> >>+++ b/test/tarantool-tests/lj-463-os-date-oom.test.lua<br>> >>@@ -0,0 +1,19 @@<br>> >>+local tap = require('tap')<br>> >>+<br>> >>+-- See also <a href="https://github.com/LuaJIT/LuaJIT/issues/463" target="_blank">https://github.com/LuaJIT/LuaJIT/issues/463</a> .<br>> >>+local test = tap.test('lj-463-os-date-oom')<br>> >>+test:plan(1)<br>> >>+<br>> >>+-- The ru_RU.utf8 locale is chosen as one that will be set on a<br>> >>+-- developer's PC with high possibility. It may be unavailable at<br>> >Typo: s/with high possibility/with a high probability/<br><br>Fixed.<br><br>> >>+-- CI, so don't check the status and result of function calls.<br>> >>+-- If it's unavailable `os.setlocale()` does nothing.<br>> >>+-- Before the patch, the call to `os.date('%p')` on non-standard<br>> >>+-- locale may lead to OOM.<br>> >>+<br>> >>+os.setlocale('ru_RU.utf8')<br>> >>+os.date('%p')<br>> >>+<br>> >>+test:ok(true, 'os.date() finished without OOM')<br>> >>+<br>> >>+os.exit(test:check() and 0 or 1)<br>> >>--<br>> >>2.34.1<br>> >--<br>> >Best regards,<br>> >Maxim Kokryashkin<br>> > <br><br>--<br>Best regards,<br>Sergey Kaplun</div></div></div></div></blockquote><div> </div></div></blockquote></BODY></HTML>