Re: [Tarantool-patches] [PATCH luajit] Fix os.date() for wider libc strftime() compatibility.

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

Hi, Maxim!

Thanks for the review!

Fixed your comments, branch is force-pushed.

On 02.02.23, Maxim Kokryashkin wrote:
> 
> Hi, Sergey!
> Thanks for the patch!
> LGTM, except for a few nits below.
> > 
> >>From: Mike Pall <mike>
> >>
> >>Thanks to Jesper Lundgren.
> >>
> >>(cherry picked from commit fc63c938b522e147ea728b75f385728bf4a8fc35)
> >>
> >>When `strftime()` returns empty result (for example, for "%p" on some
> >>locales or when LuaJIT is built with musl's `strftime()` and format
> >>string is invalid), the `os.date()` endless retries to format result
> >Typo: s/endless/endlessly/
> >Typo: s/format result/format the result/

Fixed, thanks!

> >>and reallocates buffer for resulting string. This leads to OOM.
> >Typo: s/buffer for resulting/the buffer for the resulting/

Fixed, thanks!

> >>
> >>This patch limits amount of retries by 4.
> >Typo: s/amount/the amount/

Fixed, thanks!

> >Side note: Why is it exactly 4?

That's what she said:)

> >>
> >>Sergey Kaplun:
> >>* added the description and the test for the problem
> >>
> >>Part of tarantool/tarantool#8069
> >>---
> >>
> >>PR:  https://github.com/tarantool/tarantool/pull/8237
> >>Issues:
> >>*  https://github.com/LuaJIT/LuaJIT/issues/463
> >>*  https://github.com/tarantool/tarantool/issues/8069
> >>Branch:  https://github.com/tarantool/luajit/tree/skaplun/lj-463-os-date-oom-full-ci
> >>
> >> src/lib_os.c | 4 ++--
> >> .../lj-463-os-date-oom.test.lua | 19 +++++++++++++++++++
> >> 2 files changed, 21 insertions(+), 2 deletions(-)
> >> create mode 100644 test/tarantool-tests/lj-463-os-date-oom.test.lua
> >>
> >>diff --git a/src/lib_os.c b/src/lib_os.c
> >>index 9e78d49a..ffbc3fdc 100644
> >>--- a/src/lib_os.c
> >>+++ b/src/lib_os.c
> >>@@ -205,12 +205,12 @@ LJLIB_CF(os_date)
> >>     setboolfield(L, "isdst", stm->tm_isdst);
> >>   } else if (*s) {
> >>     SBuf *sb = &G(L)->tmpbuf;
> >>- MSize sz = 0;
> >>+ MSize sz = 0, retry = 4;
> >>     const char *q;
> >>     for (q = s; *q; q++)
> >>       sz += (*q == '%') ? 30 : 1; /* Overflow doesn't matter. */
> >>     setsbufL(sb, L);
> >>- for (;;) {
> >>+ while (retry--) { /* Limit growth for invalid format or empty result. */
> >>       char *buf = lj_buf_need(sb, sz);
> >>       size_t len = strftime(buf, sbufsz(sb), s, stm);
> >>       if (len) {
> >>diff --git a/test/tarantool-tests/lj-463-os-date-oom.test.lua b/test/tarantool-tests/lj-463-os-date-oom.test.lua
> >>new file mode 100644
> >>index 00000000..cce78b6e
> >>--- /dev/null
> >>+++ b/test/tarantool-tests/lj-463-os-date-oom.test.lua
> >>@@ -0,0 +1,19 @@
> >>+local tap = require('tap')
> >>+
> >>+-- See also  https://github.com/LuaJIT/LuaJIT/issues/463 .
> >>+local test = tap.test('lj-463-os-date-oom')
> >>+test:plan(1)
> >>+
> >>+-- The ru_RU.utf8 locale is chosen as one that will be set on a
> >>+-- developer's PC with high possibility. It may be unavailable at
> >Typo: s/with high possibility/with a high probability/

Fixed.

> >>+-- CI, so don't check the status and result of function calls.
> >>+-- If it's unavailable `os.setlocale()` does nothing.
> >>+-- Before the patch, the call to `os.date('%p')` on non-standard
> >>+-- locale may lead to OOM.
> >>+
> >>+os.setlocale('ru_RU.utf8')
> >>+os.date('%p')
> >>+
> >>+test:ok(true, 'os.date() finished without OOM')
> >>+
> >>+os.exit(test:check() and 0 or 1)
> >>--
> >>2.34.1
> >--
> >Best regards,
> >Maxim Kokryashkin
> > 

-- 
Best regards,
Sergey Kaplun