Re: [Tarantool-patches] [PATCH luajit 16/19] Prevent integer overflow while parsing long strings.

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

Hi, Sergey!
Thanks for the patch!
LGTM, except for a few comments below.

On Wed, Aug 09, 2023 at 06:36:05PM +0300, Sergey Kaplun via Tarantool-patches wrote:
> From: Mike Pall <mike>
> 
> (cherry-picked from commit 16e5605eec2e3882d709c6b123a644f6a8023945)
> 
> This commit fixes possible integer overflow of the separator's length
Typo: s/possible/a possible/
> counter during parsing long strings. It may lead to the fact, that
> parser considers a string with unbalanced long brackets to be correct.
Typo: s/parser/the parser/
> Since this is pointless to parse too long string separators in the hope,
Typo: s/this is/it is/
> that the string is correct, just use hardcoded limit (2 ^ 25 is enough).
Typo: s/use hardcoded/use the hardcoded/
> 
> Be aware that this limit is different for Lua 5.1.
> 
> We can't check the string overflow itself without a really large file,
> because the ERR_MEM error will be raised, due to the string buffer
> reallocations during parsing. Keep such huge file in the repo is
Typo: s/Keep such/Keeping such a/
> pointless, so just check that we don't parse long string after
Typo: s/long string/long strings/
> aforementioned separator length.
Typo: s/aforementioned/the aforementioned/
> 
> Sergey Kaplun:
> * added the description and the test for the problem
> 
> Part of tarantool/tarantool#8825
> ---
>  src/lj_lex.c                                  |  2 +-
>  .../lj-812-too-long-string-separator.test.lua | 31 +++++++++++++++++++
>  2 files changed, 32 insertions(+), 1 deletion(-)
>  create mode 100644 test/tarantool-tests/lj-812-too-long-string-separator.test.lua
> 
> diff --git a/src/lj_lex.c b/src/lj_lex.c
> index 52856912..c66660d7 100644
> --- a/src/lj_lex.c
> +++ b/src/lj_lex.c
> @@ -138,7 +138,7 @@ static int lex_skipeq(LexState *ls)
>    int count = 0;
>    LexChar s = ls->c;
>    lua_assert(s == '[' || s == ']');
> -  while (lex_savenext(ls) == '=')
> +  while (lex_savenext(ls) == '=' && count < 0x20000000)
>      count++;
>    return (ls->c == s) ? count : (-count) - 1;
>  }
> diff --git a/test/tarantool-tests/lj-812-too-long-string-separator.test.lua b/test/tarantool-tests/lj-812-too-long-string-separator.test.lua
> new file mode 100644
> index 00000000..fda69d17
> --- /dev/null
> +++ b/test/tarantool-tests/lj-812-too-long-string-separator.test.lua
> @@ -0,0 +1,31 @@
> +local tap = require('tap')
> +
> +-- Test to check that we avoid parsing of too long separator
Typo: s/parsing of/parsing/
Typo: s/separator/separators/
> +-- for long strings.
> +-- See also the discussion in the
> +-- https://github.com/LuaJIT/LuaJIT/issues/812.
> +
> +local test = tap.test('lj-812-too-long-string-separator'):skipcond({
> +  ['Test requires GC64 mode enabled'] = not require('ffi').abi('gc64'),
Please write a more detailed description of how it can be tested for non-GC64 build
and why it is disabled now, as we have discussed offline.

> +})
> +test:plan(2)
> +
> +-- We can't check the string overflow itself without a really
> +-- large file, because the ERR_MEM error will be raised, due to
> +-- the string buffer reallocations during parsing.
> +-- Keep such huge file in the repo is pointless, so just check
> +-- that we don't parse long string after some separator length.
> +-- Be aware that this limit is different for Lua 5.1.
Please fix the same typos as in the commit message here.
> +
> +-- Use the hardcoded limit. The same as in the <src/lj_lex.c>.
> +local separator = string.rep('=', 0x20000000 + 1)
> +local test_str = ('return [%s[]%s]'):format(separator, separator)
> +
> +local f, err = loadstring(test_str, 'empty_str_f')
> +test:ok(not f, 'correct status when parsing string with too long separator')
> +
> +-- Check error message.
> +test:ok(tostring(err):match('invalid long string delimiter'),
> +        'correct error when parsing string with too long separator')
> +
> +test:done(true)
> -- 
> 2.41.0
> 
Best regards,
Maxim Kokryashkin