Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: fix crash in loop_gc #654

Merged
merged 2 commits into from
Aug 14, 2023
Merged

chore: fix crash in loop_gc #654

merged 2 commits into from
Aug 14, 2023

Conversation

zhaozg
Copy link
Member

@zhaozg zhaozg commented May 26, 2023

Fix #599
Fix part of #437

@zhaozg zhaozg force-pushed the fix/exit branch 3 times, most recently from 3d71269 to 2af42c3 Compare May 26, 2023 15:49
@zhaozg zhaozg marked this pull request as draft May 26, 2023 16:36
@zhaozg zhaozg force-pushed the fix/exit branch 4 times, most recently from f8295ec to 0b97f64 Compare May 27, 2023 04:13
@zhaozg
Copy link
Member Author

zhaozg commented May 27, 2023

So hard, long time, but a few code change.

We use ref avoid gc call before close_cb, but lua_close will prioritize userdata gc , cause reference failed.
So crash/ signal 11 occures.

Now use lhandle->ref as guard, to do right memory free. Please review, check and verify.

@zhaozg zhaozg marked this pull request as ready for review May 27, 2023 07:19
@zhaozg zhaozg changed the title chore: check handle to close in current loop when loop_gc chore: fix crash in loop_gc May 28, 2023
Copy link
Member

@squeek502 squeek502 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work on this, sorry for taking so long to review it.

Note that this only fixes part of #437, though. It's still possible to get callback functions called during lua_close/GC, which can still lead to problems. For example:

local uv = require('luv')

uv.fs_opendir('.', function()
  print("opendir cb called")
end)

-- note: calling uv.run() would fix this example

will result in (with a printf added to loop_gc):

loop_gc
opendir cb called

During that callback, it could attempt to use something that has been garbage collected (and potentially freed) so there's a high chance of errors/crashes/UB/etc.

For example, this will segfault/UAF for me on Linux because it will free the pipe and then try to read its memory in the opendir callback:

local uv = require('luv')

local pipe = uv.new_pipe(false)

uv.fs_opendir('.', function()
  print("opendir cb called")
  print(pipe)
end)

-- note: calling uv.run() would fix this example

with some more debug prints added:

handle_gc: 0x4bae640
loop_gc
handle_free: 0x4bae640
opendir cb called
==12330== Invalid read of size 8
==12330==    at 0x4F994F0: luv_check_handle.constprop.0 (handle.c:56)
==12330==    at 0x4F9973C: luv_handle_tostring (handle.c:65)

But this is a separate problem to the one solved in this PR, so it doesn't need to block this being merged.

Note: I've edited the OP to make sure this doesn't close #437.

tests/manual-test-exit.lua Outdated Show resolved Hide resolved
Lua `os.exit` prioritize `userdata` gc, cause `reference` failed.
@zhaozg zhaozg merged commit ff5e902 into luvit:master Aug 14, 2023
13 checks passed
@zhaozg zhaozg deleted the fix/exit branch August 14, 2023 10:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crash during calling os.exit()
2 participants