Hi, Sergey
thanks for review! See my comments.
New changes were force-pushed.
Hi, Sergey! Thanks for the patch! Please consider my comments below. On 29.08.23, Sergey Bronnikov wrote:From: sergeyb@tarantool.org Reported by Sergey Bronnikov. #1054I suggest to remove the ticket number, to avoid trouble trouble until trouble troubles you. :)
Agree, removed to avoid troubles.
(cherry picked from commit 309fb42b871b6414f53e0e0e708bce0b0d62daff) The following Lua snippet triggers an out of boundary access to a stack:Typo: s/an out of boundary/out-of-boundary/
Fixed.
```lua a, b, c = 1, 2, 3 local d for _ in nil do end ``` With execution snippet by LuaJIT instrumented by ASAN it leads to a heap-buffer-overflow.I suppose that it leads ever without ASAN, but the issue is observable only with ASAN, isn't it?
Right. Rephrased it:
+-- The test demonstrates a problem with out-of-boundary access
+-- to a stack. The problem can be easily observed on execution
+-- the sample by LuaJIT by ASAN, sanitizer reports a
heap-buffer-overflow.
In a function `predict_next` variable `exprpc` looks forward and expectsMinor: I suggest using of `()` for distinguishing function and variable names. Feel free to ignore.
Fixed. However "function" was before "predict_next".
extra bytecodes on the stack. However, `KPRI` is merged to the `KNIL`Typo: s/the `KNIL`/`KNIL`
Fixed.
and there is no new bytecode to add, so `exprpc == fs->bclim` and itTypo: /fs->bclim`/fs->bclim`,/
Fixed.
leads to out of boundary access.Typo: s/out of boundary/out-of-boundary/
Fixed.
Minor: I suppose that we can mention that the patch fixes the issue via early return.
Added.
Sergey Bronnikov: * added the description and the test for the problem Part of tarantool/tarantool#8825 --- PR: https://github.com/tarantool/tarantool/pull/9054 Branch: https://github.com/tarantool/luajit/tree/ligurio/lj-1054-incorrect-pc-value-predict_next Related issue: * https://github.com/LuaJIT/LuaJIT/issues/1054 src/lj_parse.c | 4 +++- ...incorrect-pc-value-in-predict_next.test.lua | 18 ++++++++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) create mode 100644 test/tarantool-tests/lj-1054-incorrect-pc-value-in-predict_next.test.lua diff --git a/src/lj_parse.c b/src/lj_parse.c index 343fa797..f1015960 100644 --- a/src/lj_parse.c +++ b/src/lj_parse.c<snipped>diff --git a/test/tarantool-tests/lj-1054-incorrect-pc-value-in-predict_next.test.lua b/test/tarantool-tests/lj-1054-incorrect-pc-value-in-predict_next.test.lua new file mode 100644 index 00000000..17f1b994 --- /dev/null +++ b/test/tarantool-tests/lj-1054-incorrect-pc-value-in-predict_next.test.lua @@ -0,0 +1,18 @@ +local tap = require('tap') +local test = tap.test('lj-1054-incorrect-pc-value-in-predict_next') +test:plan(1) + +Excess empty line.
Fixed.
+-- The test demonstrates a problem with out of boundary access to a stack.Typo: s/out of boundary/out-of-boundary/ Comment line width is more than 66 symbols.
Fixed.
+-- Sample executed in LuaJIT instrumented by ASAN leads to +-- a heap-buffer-overflow.Minor: IDK why, but suggested varian here is "heap buffer overflow".
ASAN reports error with hyphens, like this:
==90673==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x6020000000fb at pc 0x000108868a95 bp 0x7fff573979a0 sp 0x7fff57397998
READ of size 1 at 0x6020000000fb thread T0
If you don't like variant "heap-buffer-overflow" then we can use
variant used in CWE list: "heap-based buffer overflow", see [1].
What variant should
1. https://cwe.mitre.org/data/definitions/122.html
Added.+-- See also https://github.com/LuaJIT/LuaJIT/issues/528I suggest to add an empty line here.
+local lua_code = [[ +a, b, c = 1, 2, 3 +local d +for _ in nil do end +]] + +test:ok(loadstring(lua_code), 'parsing is correct')I suggest also to test that the behaviour of the executed chunk is the same as in the PUC RIO Lua 5.1 (like it is done for the lj-1033).
Updated:
TAP version 13
1..3
ok - chunk loaded successfully
ok - loaded function is failed (expected)
ok - correct error message
+ +test:done(true) -- 2.34.1