Re: [Tarantool-patches] [PATCH luajit 2/2] Different fix for partial snapshot restore due to stack overflow.

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

[-- Attachment #1: Type: text/plain, Size: 2963 bytes --]

Hi, Sergey,

LGTM

On 6/10/25 19:22, Sergey Kaplun wrote:
> Hi, Sergey!
> Thanks for the review!
> Please, consider my answer below.
>
> On 10.06.25, Sergey Bronnikov wrote:
>> Hello, Sergey!
>>
>> Thanks for the patch! See a comment below.
>>
>> Sergey
>>
>> On 6/10/25 13:28, Sergey Kaplun wrote:
>>> From: Mike Pall <mike>
>>>
>>> Reported by Junlong Li. Fixed by Peter Cawley.
>>>
>>> (cherry picked from commit 86e7123bb1782a5f200ba5e83b8c4f3fbad4f7bc)
>>>
>>> This patch is a follow-up to the previous commit, which leads to a dirty
>>> read of the pseudo-valid PC set for the cframe on snapshot restoration.
>>> To avoid these dirty reads, this patch sets the PC to the outer frame
>>> as well before possible error throwing.
>>>
>>> Sergey Kaplun:
>>> * added the description and the test for the problem
>>>
>>> Part of tarantool/tarantool#11278
>>> ---
>>>    src/lj_snap.c                                 |  3 +-
>>>    src/lj_trace.c                                |  4 +-
>>>    ...-1196-stack-overflow-snap-restore.test.lua | 65 +++++++++++++++++++
>>>    3 files changed, 68 insertions(+), 4 deletions(-)
>>>    create mode 100644 test/tarantool-tests/lj-1196-stack-overflow-snap-restore.test.lua
>>>
>>> diff --git a/src/lj_snap.c b/src/lj_snap.c
>>> index 8d7bd868..4cfae579 100644
>>> --- a/src/lj_snap.c
>>> +++ b/src/lj_snap.c
> <snipped>
>
>>> diff --git a/test/tarantool-tests/lj-1196-stack-overflow-snap-restore.test.lua b/test/tarantool-tests/lj-1196-stack-overflow-snap-restore.test.lua
>>> new file mode 100644
>>> index 00000000..942d1f82
>>> --- /dev/null
>>> +++ b/test/tarantool-tests/lj-1196-stack-overflow-snap-restore.test.lua
>>> @@ -0,0 +1,65 @@
>>> +local tap = require('tap')
>>> +
>>> +-- Test file to demonstrate LuaJIT dirty reads after stack
>>> +-- overflow during restoration from the snapshot.
>>> +-- The test fails before the patch under Valgrind.
>> Please specify valgrind option that is required for reproducing the bug.
> This just mean the default valgrind settings (without any flags).

With this command-line the bug is reproduced:

cmake -S . -B build -DLUAJIT_USE_VALGRIND=ON -DLUAJIT_USE_ASAN=OFF 
-DLUAJIT_USE_SYSMALLOC=ON -DLUAJIT_ENABLE_GC64=ON  -DLUA_USE_APICHECK=ON 
-DLUA_USE_ASSERT=ON -DCMAKE_BUILD_TYPE=Debug

cd build && ctest -V -R 
test/tarantool-tests/lj-1196-stack-overflow-snap-restore.test.lua -V


>
>> Cannot reproduce with command line below with reverted patch:
>>
>> VALGRIND_OPTS="--leak-check=no --malloc-fill=0x00 --free-fill=0x00"
>> ctest -V -R test/tarantool-tests/lj-1196-partial-snap-restore.test.lua -V
> This doesn't fail due to --leadk-check=no flag.
> Also, I suppose this should fail under ASan.
>
>>> +--
>>> +-- luacheck: push no max_comment_line_length
>>> +--
>>> +-- Seealso:https://github.com/LuaJIT/LuaJIT/issues/1196,
>>> +--https://www.freelists.org/post/luajit/Invalid-read-found-by-valgrind.
>>> +--
>>> +-- luacheck: pop
>>> +
> <snipped>
>
>>> +test:done(true)

[-- Attachment #2: Type: text/html, Size: 4624 bytes --]