[go: up one dir, main page]
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.

Sign up for GitHub

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

Jump to bottom

fix memory leak in fs.scandir sync mode #694

Merged
merged 1 commit into from
Feb 23, 2024
Merged

fix memory leak in fs.scandir sync mode #694

merged 1 commit into from
Feb 23, 2024

Conversation

zhaozg
Copy link
Member
@zhaozg zhaozg commented Feb 23, 2024

No description provided.

@zhaozg zhaozg enabled auto-merge (rebase) February 23, 2024 17:07
squeek502
squeek502 approved these changes Feb 23, 2024
View reviewed changes
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 catch, would be nice if we could have caught this in CI somehow.

@zhaozg zhaozg merged commit 0e4a895 into luvit:master Feb 23, 2024
14 checks passed
@zhaozg
Copy link
Member Author
zhaozg commented Feb 24, 2024

I found it in long time luvit/webserver. luv test close will free all mems, so not found by valgrind or Asan.

@lewis6991
Copy link

Just to note that this change caused an issue in Neovim: neovim/neovim#27678 (comment)

Application Specific Information:
abort() called


Thread 0 Crashed::  Dispatch queue: com.apple.main-thread
0   libsystem_kernel.dylib        	       0x18cde20dc __pthread_kill + 8
1   libsystem_pthread.dylib       	       0x18ce19cc0 pthread_kill + 288
2   libsystem_c.dylib             	       0x18cd25a40 abort + 180
3   libsystem_malloc.dylib        	       0x18cc3cb08 malloc_vreport + 908
4   libsystem_malloc.dylib        	       0x18cc403f4 malloc_report + 64
5   libsystem_malloc.dylib        	       0x18cc54ebc find_zone_and_free + 308
6   nvim                          	       0x100eb13b8 uv__fs_scandir_cleanup + 84
7   nvim                          	       0x100eb7e68 uv_fs_req_cleanup + 120
8   nvim                          	       0x100e03634 luv_fs_gc + 132
9   nvim                          	       0x100e42aec lj_BC_FUNCC + 44
10  nvim                          	       0x100e44cc8 gc_call_finalizer + 148 (lj_gc.c:521)
11  nvim                          	       0x100b7fdb4 nlua_push_Object + 320 (converter.c:781)
12  nvim                          	       0x100b7fc48 nlua_push_Dictionary + 332 (converter.c:722)
13  nvim                          	       0x100992228 nlua_api_nvim_get_mode + 160 (lua_api_c_bindings.generated.c:6379)
14  nvim                          	       0x100e42aec lj_BC_FUNCC + 44
15  nvim                          	       0x100e590b4 lua_pcall + 228 (lj_api.c:1150)

@squeek502
Copy link
Member
squeek502 commented Mar 1, 2024 •
edited
Loading

That makes sense, although I can't quite figure out how to reproduce that particular crash yet.

I believe the real problem here is that luv_fs_scandir is returning a uv_req_t that effectively stores a reference to itself in the LUA_REGISTRYINDEX which means that it will never be garbage collected until the program ends.

This PR tries to workaround that by unrefing it, but that means that the underlying uv_req_t memory can be garbage collected while the Lua userdata still exists, and it could therefore get its gc function called which will try to call uv_fs_req_cleanup on already-freed memory.

So the real fix here needs to be making the return of luv_fs_scandir able to be properly garbage collected.

EDIT: Potential fix: #696

@squeek502 squeek502 mentioned this pull request Mar 1, 2024
Merged
squeek502 added a commit to squeek502/luv that referenced this pull request Mar 2, 2024
@squeek502
Add test that would have reproduced a use-after-free
ec6ecf5 
See luvit#694 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@squeek502 squeek502 squeek502 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@zhaozg @lewis6991 @squeek502