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

[Maxim Kokryashkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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


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/
>>and reallocates buffer for resulting string. This leads to OOM.
>Typo: s/buffer for resulting/the buffer for the resulting/
>>
>>This patch limits amount of retries by 4.
>Typo: s/amount/the amount/
>Side note: Why is it exactly 4?
>>
>>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/
>>+-- 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
> 

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