From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 973F0580F6B; Wed, 16 Aug 2023 17:57:36 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 973F0580F6B DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1692197856; bh=UM0HWlngjQ/nFdm2F2vuTDmq/isdhblByzklNELkeRc=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=F63Pc/gw8Tb8ZRwH6nPDk4YFS+ZW2bg3S2EWU1N6rPd6bIDvbRqkmQnzuZo7+UV6u krOwrfZDwaQHVTmliDCo/Ak7p28aWJdj7P/0sPJ8vdkCnJ/IgXXWMHFxOajid3FJLZ rN9tGrtKpua4j2uyLTYSnVKh7tA0JJUiLQE9GjYM= Received: from smtpng3.i.mail.ru (smtpng3.i.mail.ru [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 0F3BE532601 for ; Wed, 16 Aug 2023 17:57:35 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 0F3BE532601 Received: by smtpng3.m.smailru.net with esmtpa (envelope-from ) id 1qWHxl-0002nv-UO; Wed, 16 Aug 2023 17:57:34 +0300 Date: Wed, 16 Aug 2023 17:52:46 +0300 To: Maxim Kokryashkin Message-ID: References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD9700E0DCE2907754DD1BE1B13E5F9E0E702D280D9A56F9A40182A05F538085040015C9C0C64F326747B40D229BA56BEF3A3EEEF3231147054D430718C096838CA X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7922D113DFDC6D5A3EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006374F638C8F0F4AA0FB8638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8192D9945F18412DBB55D8C4C2558C941117882F4460429724CE54428C33FAD305F5C1EE8F4F765FC0F49EF363AAD6E82A471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F446042972877693876707352033AC447995A7AD186FD1C55BDD38FC3FD2E47CDBA5A96583BA9C0B312567BB231DD303D21008E29813377AFFFEAFD269A417C69337E82CC2E827F84554CEF50127C277FBC8AE2E8BAE9A1BBD95851C5BAAAE862A0553A39223F8577A6DFFEA7CC1948A84299AD5C643847C11F186F3C59DAA53EE0834AAEE X-C1DE0DAB: 0D63561A33F958A504D695A03FF972B19A17DED0F9A4B2695F58144C5C36ACBDF87CCE6106E1FC07E67D4AC08A07B9B04B3849D6E5CCBAFDBDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0ADBF74143AD284FC7177DD89D51EBB7742424CF958EAFF5D571004E42C50DC4CA955A7F0CF078B5EC49A30900B95165D34324A9840C798D5006DAF2B3269374088B6A9DD8A9D5CBCE082506926D8A92A8B9DD37CCB91B48CC21D7E09C32AA3244C0E0ABAC553A47A90AD23A2D205EF287864EE5813BBCA3A9DBAD658CF5C8AB4025DA084F8E80FEBD3202CD0F03380D9577A83BD0C44CE203720ABEDE4BBDD9CDD X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojHVl7ekwB6hjsVlBjZSDrCw== X-DA7885C5: 5E0621F6290E746D201DC5335C488F055EA112EC5DB2FAECD41E5ABC5602EA27262E2D401490A4A0DB037EFA58388B346E8BC1A9835FDE71 X-Mailru-Sender: 689FA8AB762F73930F533AC2B33E986B783670E40512FB91D5C5ECB9EA75EDE70FBE9A32752B8C9C2AA642CC12EC09F1FB559BB5D741EB962F61BD320559CF1EFD657A8799238ED55FEEDEB644C299C0ED14614B50AE0675 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit 16/19] Prevent integer overflow while parsing long strings. X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Sergey Kaplun via Tarantool-patches Reply-To: Sergey Kaplun Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 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 > > > > (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 > > 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 . > > +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 . @@ -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