* Re: [Tarantool-patches] [PATCH luajit] FFI: Fix pragma push stack limit check and throw on overflow.
2023-11-08 8:40 [Tarantool-patches] [PATCH luajit] FFI: Fix pragma push stack limit check and throw on overflow Sergey Kaplun via Tarantool-patches
@ 2023-11-08 18:39 ` Igor Munkin via Tarantool-patches
2023-11-08 18:59 ` Maxim Kokryashkin via Tarantool-patches
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Igor Munkin via Tarantool-patches @ 2023-11-08 18:39 UTC (permalink / raw)
To: Sergey Kaplun; +Cc: tarantool-patches
Sergey,
Thanks for the patch! LGTM, with a single nit below.
On 08.11.23, Sergey Kaplun wrote:
> From: Mike Pall <mike>
>
> Reported by Sergey Kaplun.
>
> (cherry-picked from commit 433d7e8d8d182f44e88b5cfdc4b2d3026469dfb7)
>
> `cp->packstack` is the array of size `CPARSE_MAX_PACKSTACK` (7). Before
> the patch, `cp->curpack` is checked to be less than
> `CPARSE_MAX_PACKSTACK`, but then `cp->packstack` is accessed at
> `cp->curpack + 1`, which is out of bounds, so `cp->curpack` value is
> overwritten.
>
> This patch fixes a condition and also adds the error throw when counter
> is overflow (instead of rewriting a top `cp->packstack` value).
>
> Sergey Kaplun:
> * added the description and the test for the problem
>
> Resolves tarantool/tarantool#9339
> Part of tarantool/tarantool#9145
> ---
>
> Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-1114-ffi-pragma-pack
> Tarantool PR: https://github.com/tarantool/tarantool/pull/9342
> Relate issues:
> * https://github.com/LuaJIT/LuaJIT/issues/1114
> * https://github.com/tarantool/tarantool/issues/9339
> * https://github.com/tarantool/tarantool/issues/9145
>
> src/lj_cparse.c | 4 +-
> .../lj-1114-ffi-pragma-pack.test.lua | 44 +++++++++++++++++++
> 2 files changed, 47 insertions(+), 1 deletion(-)
> create mode 100644 test/tarantool-tests/lj-1114-ffi-pragma-pack.test.lua
>
<snipped>
> diff --git a/test/tarantool-tests/lj-1114-ffi-pragma-pack.test.lua b/test/tarantool-tests/lj-1114-ffi-pragma-pack.test.lua
> new file mode 100644
> index 00000000..e5642828
> --- /dev/null
> +++ b/test/tarantool-tests/lj-1114-ffi-pragma-pack.test.lua
> @@ -0,0 +1,44 @@
> +local tap = require('tap')
> +
> +-- Test file to demonstrate LuaJIT incorrect parsing of `#pragma`
> +-- directive via FFI.
> +-- See also: https://github.com/LuaJIT/LuaJIT/issues/1114.
> +
> +local test = tap.test('lj-1114-ffi-pragma-pack')
> +local ffi = require 'ffi'
Please use parantheses here too.
> +
> +test:plan(2)
> +
> +-- `cp->packstack` is the array of size `CPARSE_MAX_PACKSTACK`
> +-- (7). Before the patch, `cp->curpack` is checked to be less than
> +-- `CPARSE_MAX_PACKSTACK`, but then `cp->packstack` is accessed at
> +-- `cp->curpack + 1`, which is out of bounds, so `cp->curpack`
> +-- value is overwritten.
> +-- As a result, the incorrect pack value (1) is chosen after pop.
> +-- After the patch, the error is thrown in the case of overflow
> +-- (instead of rewriting the top pack slot value), so we use the
> +-- wrapper to catch the error.
> +local function ffi_cdef_wp()
> + ffi.cdef[[
> + #pragma pack(push, 1)
> + #pragma pack(push, 1)
> + #pragma pack(push, 1)
> + #pragma pack(push, 1)
> + #pragma pack(push, 8)
> + #pragma pack(push, 8)
> + #pragma pack(push, 8)
> + #pragma pack(pop)
> + struct aligned_struct {uint64_t a; uint8_t b;};
> + ]]
> +
> + -- Got 9 in case of buffer overflow.
> + return ffi.sizeof(ffi.new('struct aligned_struct'))
> +end
> +
> +local err, msg = pcall(ffi_cdef_wp)
> +
> +test:ok(not err, 'the error is thrown when couner overflows')
> +test:like(msg, 'chunk has too many syntax levels',
> + 'the error message is correct')
> +
> +test:done(true)
> --
> 2.42.0
>
--
Best regards,
IM
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [Tarantool-patches] [PATCH luajit] FFI: Fix pragma push stack limit check and throw on overflow.
2023-11-08 8:40 [Tarantool-patches] [PATCH luajit] FFI: Fix pragma push stack limit check and throw on overflow Sergey Kaplun via Tarantool-patches
2023-11-08 18:39 ` Igor Munkin via Tarantool-patches
@ 2023-11-08 18:59 ` Maxim Kokryashkin via Tarantool-patches
2023-11-09 5:57 ` Sergey Kaplun via Tarantool-patches
2023-11-23 6:31 ` Igor Munkin via Tarantool-patches
3 siblings, 0 replies; 5+ messages in thread
From: Maxim Kokryashkin via Tarantool-patches @ 2023-11-08 18:59 UTC (permalink / raw)
To: Sergey Kaplun; +Cc: tarantool-patches
Hi, Sergey!
Thanks for the patch!
LGTM after fixing the comment left by Igor, and
one additional comment from me (see it below).
On Wed, Nov 08, 2023 at 11:40:44AM +0300, Sergey Kaplun wrote:
> From: Mike Pall <mike>
>
> Reported by Sergey Kaplun.
>
> (cherry-picked from commit 433d7e8d8d182f44e88b5cfdc4b2d3026469dfb7)
>
> `cp->packstack` is the array of size `CPARSE_MAX_PACKSTACK` (7). Before
> the patch, `cp->curpack` is checked to be less than
> `CPARSE_MAX_PACKSTACK`, but then `cp->packstack` is accessed at
> `cp->curpack + 1`, which is out of bounds, so `cp->curpack` value is
> overwritten.
>
> This patch fixes a condition and also adds the error throw when counter
> is overflow (instead of rewriting a top `cp->packstack` value).
>
> Sergey Kaplun:
> * added the description and the test for the problem
>
> Resolves tarantool/tarantool#9339
> Part of tarantool/tarantool#9145
> ---
>
> Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-1114-ffi-pragma-pack
> Tarantool PR: https://github.com/tarantool/tarantool/pull/9342
> Relate issues:
> * https://github.com/LuaJIT/LuaJIT/issues/1114
> * https://github.com/tarantool/tarantool/issues/9339
> * https://github.com/tarantool/tarantool/issues/9145
>
> src/lj_cparse.c | 4 +-
> .../lj-1114-ffi-pragma-pack.test.lua | 44 +++++++++++++++++++
> 2 files changed, 47 insertions(+), 1 deletion(-)
> create mode 100644 test/tarantool-tests/lj-1114-ffi-pragma-pack.test.lua
>
> diff --git a/src/lj_cparse.c b/src/lj_cparse.c
> index 6d9490ca..01deb3bf 100644
> --- a/src/lj_cparse.c
> +++ b/src/lj_cparse.c
<snipped>
> diff --git a/test/tarantool-tests/lj-1114-ffi-pragma-pack.test.lua b/test/tarantool-tests/lj-1114-ffi-pragma-pack.test.lua
> new file mode 100644
> index 00000000..e5642828
> --- /dev/null
> +++ b/test/tarantool-tests/lj-1114-ffi-pragma-pack.test.lua
> @@ -0,0 +1,44 @@
<snipped>
> +test:ok(not err, 'the error is thrown when couner overflows')
Typo: s/couner/counter/
> +test:like(msg, 'chunk has too many syntax levels',
> + 'the error message is correct')
> +
> +test:done(true)
> --
> 2.42.0
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit] FFI: Fix pragma push stack limit check and throw on overflow.
2023-11-08 8:40 [Tarantool-patches] [PATCH luajit] FFI: Fix pragma push stack limit check and throw on overflow Sergey Kaplun via Tarantool-patches
2023-11-08 18:39 ` Igor Munkin via Tarantool-patches
2023-11-08 18:59 ` Maxim Kokryashkin via Tarantool-patches
@ 2023-11-09 5:57 ` Sergey Kaplun via Tarantool-patches
2023-11-23 6:31 ` Igor Munkin via Tarantool-patches
3 siblings, 0 replies; 5+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2023-11-09 5:57 UTC (permalink / raw)
To: Maxim Kokryashkin, Igor Munkin, Sergey Bronnikov; +Cc: tarantool-patches
Hi, folks!
Thanks for the review!
Your comments are fixed, branch is rebased on master and force-pushed.
See iterative diff below.
===================================================================
diff --git a/test/tarantool-tests/lj-1114-ffi-pragma-pack.test.lua b/test/tarantool-tests/lj-1114-ffi-pragma-pack.test.lua
index e5642828..99b98900 100644
--- a/test/tarantool-tests/lj-1114-ffi-pragma-pack.test.lua
+++ b/test/tarantool-tests/lj-1114-ffi-pragma-pack.test.lua
@@ -5,7 +5,7 @@ local tap = require('tap')
-- See also: https://github.com/LuaJIT/LuaJIT/issues/1114.
local test = tap.test('lj-1114-ffi-pragma-pack')
-local ffi = require 'ffi'
+local ffi = require('ffi')
test:plan(2)
@@ -37,7 +37,7 @@ end
local err, msg = pcall(ffi_cdef_wp)
-test:ok(not err, 'the error is thrown when couner overflows')
+test:ok(not err, 'the error is thrown when counter overflows')
test:like(msg, 'chunk has too many syntax levels',
'the error message is correct')
===================================================================
--
Best regards,
Sergey Kaplun
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [Tarantool-patches] [PATCH luajit] FFI: Fix pragma push stack limit check and throw on overflow.
2023-11-08 8:40 [Tarantool-patches] [PATCH luajit] FFI: Fix pragma push stack limit check and throw on overflow Sergey Kaplun via Tarantool-patches
` (2 preceding siblings ...)
2023-11-09 5:57 ` Sergey Kaplun via Tarantool-patches
@ 2023-11-23 6:31 ` Igor Munkin via Tarantool-patches
3 siblings, 0 replies; 5+ messages in thread
From: Igor Munkin via Tarantool-patches @ 2023-11-23 6:31 UTC (permalink / raw)
To: Sergey Kaplun; +Cc: tarantool-patches
Sergey,
I've checked the patchset into all long-term branches in
tarantool/luajit and bumped a new version in master, release/2.11 and
release/2.10.
On 08.11.23, Sergey Kaplun wrote:
> From: Mike Pall <mike>
>
> Reported by Sergey Kaplun.
>
> (cherry-picked from commit 433d7e8d8d182f44e88b5cfdc4b2d3026469dfb7)
>
> `cp->packstack` is the array of size `CPARSE_MAX_PACKSTACK` (7). Before
> the patch, `cp->curpack` is checked to be less than
> `CPARSE_MAX_PACKSTACK`, but then `cp->packstack` is accessed at
> `cp->curpack + 1`, which is out of bounds, so `cp->curpack` value is
> overwritten.
>
> This patch fixes a condition and also adds the error throw when counter
> is overflow (instead of rewriting a top `cp->packstack` value).
>
> Sergey Kaplun:
> * added the description and the test for the problem
>
> Resolves tarantool/tarantool#9339
> Part of tarantool/tarantool#9145
> ---
>
> Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-1114-ffi-pragma-pack
> Tarantool PR: https://github.com/tarantool/tarantool/pull/9342
> Relate issues:
> * https://github.com/LuaJIT/LuaJIT/issues/1114
> * https://github.com/tarantool/tarantool/issues/9339
> * https://github.com/tarantool/tarantool/issues/9145
>
> src/lj_cparse.c | 4 +-
> .../lj-1114-ffi-pragma-pack.test.lua | 44 +++++++++++++++++++
> 2 files changed, 47 insertions(+), 1 deletion(-)
> create mode 100644 test/tarantool-tests/lj-1114-ffi-pragma-pack.test.lua
>
> diff --git a/src/lj_cparse.c b/src/lj_cparse.c
> index 6d9490ca..01deb3bf 100644
> --- a/src/lj_cparse.c
> +++ b/src/lj_cparse.c
> @@ -1777,9 +1777,11 @@ static void cp_pragma(CPState *cp, BCLine pragmaline)
> cp_check(cp, '(');
> if (cp->tok == CTOK_IDENT) {
> if (cp_str_is(cp->str, "push")) {
> - if (cp->curpack < CPARSE_MAX_PACKSTACK) {
> + if (cp->curpack < CPARSE_MAX_PACKSTACK-1) {
> cp->packstack[cp->curpack+1] = cp->packstack[cp->curpack];
> cp->curpack++;
> + } else {
> + cp_errmsg(cp, cp->tok, LJ_ERR_XLEVELS);
> }
> } else if (cp_str_is(cp->str, "pop")) {
> if (cp->curpack > 0) cp->curpack--;
> diff --git a/test/tarantool-tests/lj-1114-ffi-pragma-pack.test.lua b/test/tarantool-tests/lj-1114-ffi-pragma-pack.test.lua
> new file mode 100644
> index 00000000..e5642828
> --- /dev/null
> +++ b/test/tarantool-tests/lj-1114-ffi-pragma-pack.test.lua
> @@ -0,0 +1,44 @@
> +local tap = require('tap')
> +
> +-- Test file to demonstrate LuaJIT incorrect parsing of `#pragma`
> +-- directive via FFI.
> +-- See also: https://github.com/LuaJIT/LuaJIT/issues/1114.
> +
> +local test = tap.test('lj-1114-ffi-pragma-pack')
> +local ffi = require 'ffi'
> +
> +test:plan(2)
> +
> +-- `cp->packstack` is the array of size `CPARSE_MAX_PACKSTACK`
> +-- (7). Before the patch, `cp->curpack` is checked to be less than
> +-- `CPARSE_MAX_PACKSTACK`, but then `cp->packstack` is accessed at
> +-- `cp->curpack + 1`, which is out of bounds, so `cp->curpack`
> +-- value is overwritten.
> +-- As a result, the incorrect pack value (1) is chosen after pop.
> +-- After the patch, the error is thrown in the case of overflow
> +-- (instead of rewriting the top pack slot value), so we use the
> +-- wrapper to catch the error.
> +local function ffi_cdef_wp()
> + ffi.cdef[[
> + #pragma pack(push, 1)
> + #pragma pack(push, 1)
> + #pragma pack(push, 1)
> + #pragma pack(push, 1)
> + #pragma pack(push, 8)
> + #pragma pack(push, 8)
> + #pragma pack(push, 8)
> + #pragma pack(pop)
> + struct aligned_struct {uint64_t a; uint8_t b;};
> + ]]
> +
> + -- Got 9 in case of buffer overflow.
> + return ffi.sizeof(ffi.new('struct aligned_struct'))
> +end
> +
> +local err, msg = pcall(ffi_cdef_wp)
> +
> +test:ok(not err, 'the error is thrown when couner overflows')
> +test:like(msg, 'chunk has too many syntax levels',
> + 'the error message is correct')
> +
> +test:done(true)
> --
> 2.42.0
>
--
Best regards,
IM
^ permalink raw reply [flat|nested] 5+ messages in thread