osx: Fix a memory leak in uv_fs_readdir() #1048
Conversation
|
Thank you for contributing this pull request! Here are a few pointers to make sure your submission will be considered for inclusion. Commit alexcrichton/libuv@74e759e has the following error(s):
You can fix all these things without opening another issue. Please see CONTRIBUTING.md for more information |
| @@ -205,7 +205,7 @@ static int uv__fs_readdir_filter(const struct dirent* dent) { | |||
|
|
|||
| /* This should have been called uv__fs_scandir(). */ | |||
| static ssize_t uv__fs_readdir(uv_fs_t* req) { | |||
| struct dirent **dents; | |||
| struct dirent **dents = NULL; | |||
There was a problem hiding this comment.
Please put the assignment on a different line.
|
If n is 0, then we won't free any extra stuff in the out label because the for loop won't be entered, right? What is freed exactly? I seem to be missing something :-) |
|
Yes, the Also updated with comments! |
|
I think I found it :-) I downloaded the source for Libc from opensource.apple.com, here is the scandir implementation: https://gist.github.com/saghul/8013376 The dirent is allocated here: https://gist.github.com/saghul/8013376#file-scandir-c-L89 And it's returned even if the directory is empty, since this while loop won't run: https://gist.github.com/saghul/8013376#file-scandir-c-L93 So, you seem to be right :-) @indutny, does it also LGTY? |
|
FWIW, I had a look at glibc and though the implementation is different, if the returned dirent is not NULL it needs to be freed as well. Maybe we should also reword the commit log and remove that comment about OSX. FreeBSD probably has the same implementation. We could say that "Some scandir implementations allocate the dirent struct even if the directory is empty, ...". |
Some scandir implementations allocate the dirent struct even if the directory is empty, so if `scandir` returns 0 there may still be memory that needs to get deallocated. I have altered uv__fs_readdir to go to the "deallocation exit area" when 0 files are found in the directory and continue to return early on a return value of -1. I went to add a test for this functionality, but it appears that one already exists (reading an empty directory), so I imagine that the valgrind builds must only be happening on linux instead of OSX as well? I have confirmed manually that a program without this fix will infinitely leak memory, and with this fix the memory usage stays constant.
|
Amended the message to be a be more general. |
|
Ok, I'll wait for @indutny's thumbs up, just in case my reasoning is totally broken (let's hope not ;-) ) but LGTM. |
|
Thanks for looking into this and the other libc implementations! |
|
LGTM! |
|
Landed in v0.10 in 7c6bddb |
|
Thank you! |
|
Thanks @alexcrichton! :-) |
I haven't landed this fix upstream just yet, but it's opened as joyent/libuv#1048. For now, I've locally merged it into my fork, and I've upgraded our repo to point to the new revision. Closes rust-lang#11027
I haven't landed this fix upstream just yet, but it's opened as joyent/libuv#1048. For now, I've locally merged it into my fork, and I've upgraded our repo to point to the new revision. Closes #11027
On OSX, apparently directories with 0 files in them still allocate memory on
the invocation of
scandir, so if the function returns early it ends upleaking memory allocated in
scandir. I have altered uv__fs_readdir toinstead go to the "deallocation exit area" when 0 files are found in the
directory, and continue to return early on a return value of -1.
I went to add a test for this functionality, but it appears that one already
exists (reading an empty directory), so I imagine that the valgrind builds
must only be happening on linux instead of OSX as well? I have confirmed
manually that a program without this fix will infinitely leak memory, and with
this fix the memory usage stays constant.