Tarantool development patches archive
 help / color / mirror / Atom feed
From: sergos via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Sergey Kaplun <skaplun@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH luajit] Fix os.date() for wider libc strftime() compatibility.
Date: Fri, 3 Feb 2023 16:28:59 +0300	[thread overview]
Message-ID: <11D74B84-C7B2-442B-BFE8-8FB39D32FD57@tarantool.org> (raw)
In-Reply-To: <Y9zGloxMUiCaVkYw@root>

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

Thanks for update!

LGTM.

Sergos

> On 3 Feb 2023, at 11:32, Sergey Kaplun <skaplun@tarantool.org> wrote:
> 
> Hi, Sergos!
> 
> Thanks for the review!
> 
> Changed the test as you propose:
> 
> ===================================================================
> diff --git a/test/tarantool-tests/lj-463-os-date-oom.test.lua b/test/tarantool-tests/lj-463-os-date-oom.test.lua
> index a39ab6d1..0861ce14 100644
> --- a/test/tarantool-tests/lj-463-os-date-oom.test.lua
> +++ b/test/tarantool-tests/lj-463-os-date-oom.test.lua
> @@ -4,15 +4,13 @@ local tap = require('tap')
> 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 a high possibility. It may be unavailable
> --- at CI, so don't check the status and result of function calls.
> --- If it's unavailable `os.setlocale()` does nothing.
> +-- Try all available locales to check the behaviour.
> -- 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')
> +-- locale (ru_RU.utf8, sv_SE.utf8, etc.) may lead to OOM.
> +for locale in io.popen('locale -a'):read('*a'):gmatch('([^\n]*)\n?') do
> +    os.setlocale(locale)
> +    os.date('%p')
> +end
> 
> test:ok(true, 'os.date() finished without OOM')
> ===================================================================
> 
> Branch is force-pushed.
> 
> On 03.02.23, sergos wrote:
>> Hi!
>> 
>> Thanks for the patch!
>> 
>> I can propose to update test with something like
>> 
>>  for i in io.popen('locale -a', 'r'):read('*a'):gmatch("([^\n]*)\n?") do
>>      os.setlocale(i)
>>      os.date(%p)
>>  end
>> 
>> to run the test across all available options. The reason is I got:
>> 
>>  tarantool> os.setlocale('ru_RU.utf8')
>>  ---
>>  - null
>> 
>> on my system, so I don’t by the ‘high probability’. 
> 
> Lucky you:).
> 
>> 
>> Regards,
>> Sergos
>> 
>> 
>>> On 1 Feb 2023, at 10:46, Sergey Kaplun <skaplun@tarantool.org> wrote:
>>> 
>>> 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
>>> and reallocates buffer for resulting string. This leads to OOM.
>>> 
>>> This patch limits amount of retries by 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
> 
> <snipped>
> 
>>> 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
>>> +-- 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,
> Sergey Kaplun


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

  reply	other threads:[~2023-02-03 13:29 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-02-01  7:46 Sergey Kaplun via Tarantool-patches
2023-02-01 23:41 ` Maxim Kokryashkin via Tarantool-patches
2023-02-02 19:28   ` Sergey Kaplun via Tarantool-patches
2023-02-02 21:08     ` Maxim Kokryashkin via Tarantool-patches
2023-02-02 22:28 ` sergos via Tarantool-patches
2023-02-03  8:32   ` Sergey Kaplun via Tarantool-patches
2023-02-03 13:28     ` sergos via Tarantool-patches [this message]
2023-02-20  9:57 ` Igor Munkin via Tarantool-patches

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=11D74B84-C7B2-442B-BFE8-8FB39D32FD57@tarantool.org \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=sergos@tarantool.org \
    --cc=skaplun@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH luajit] Fix os.date() for wider libc strftime() compatibility.' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox