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

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

Hi, Maxim!
Thanks for the review!
See my answers below.

On 15.08.23, Maxim Kokryashkin wrote:
> 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/

Fixed.

> > 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/

Fixed.

> > Since this is pointless to parse too long string separators in the hope,
> Typo: s/this is/it is/

Fixed.

> > that the string is correct, just use hardcoded limit (2 ^ 25 is enough).
> Typo: s/use hardcoded/use the hardcoded/

Fixed.

> > 
> > 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/

Fixed.

> > pointless, so just check that we don't parse long string after
> Typo: s/long string/long strings/

Fixed.

> > aforementioned separator length.
> Typo: s/aforementioned/the aforementioned/

Fixed.

> > 
> > 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

<snipped>

> > 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/

Fixed.

> > +-- 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.

Added, see the diff below.

> 
> > +})
> > +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.

Fixed.

> > +
> > +-- 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')

Also, changed this part to the `test:like()`, since it is more readable
and has the same behaviour.

See the iterative patch below:

===================================================================
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
index fda69d17..380e26f0 100644
--- a/test/tarantool-tests/lj-812-too-long-string-separator.test.lua
+++ b/test/tarantool-tests/lj-812-too-long-string-separator.test.lua
@@ -1,11 +1,17 @@
 local tap = require('tap')
 
--- Test to check that we avoid parsing of too long separator
--- for long strings.
+-- Test to check that we avoid parsing too long 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({
+  -- In non-GC64 mode, we get the OOM error since we need memory
+  -- for the string to load and the same amount of memory for the
+  -- string buffer. So, the only option is to create a big file
+  -- in the repo and keep it, or generate it and remove each time.
+  -- These options are kinda pointless, so let's check the
+  -- behaviour only for GC64 mode.
   ['Test requires GC64 mode enabled'] = not require('ffi').abi('gc64'),
 })
 test:plan(2)
@@ -13,8 +19,9 @@ 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.
+-- Keeping such a huge file in the repo is pointless, so just
+-- check that we don't parse long strings after some separator
+-- length.
 -- Be aware that this limit is different for Lua 5.1.
 
 -- Use the hardcoded limit. The same as in the <src/lj_lex.c>.
@@ -25,7 +32,7 @@ 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:like(err, 'invalid long string delimiter',
+          'correct error when parsing string with too long separator')
 
 test:done(true)
===================================================================

> > +
> > +test:done(true)
> > -- 
> > 2.41.0
> > 
> Best regards,
> Maxim Kokryashkin

-- 
Best regards,
Sergey Kaplun