-
Notifications
You must be signed in to change notification settings - Fork 187
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
Conversation
3d71269
to
2af42c3
Compare
f8295ec
to
0b97f64
Compare
So hard, long time, but a few code change. We use ref avoid gc call before close_cb, but Now use |
loop_gc
loop_gc
There was a problem hiding this 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.
Lua `os.exit` prioritize `userdata` gc, cause `reference` failed.
Fix #599
Fix part of #437