fix: remove orphan ADDREF which causes resource leaks (#118)#119
Open
sj-i wants to merge 1 commit into
Open
Conversation
sj-i
force-pushed
the
fix-fd-leak
branch
2 times, most recently
from
b2b4f48 to
aaee94c
Compare
First use of PHP_UV_ZVAL_TO_FD already adds the refcount, and there are no matching DECREF for those explicit ADDREFs. So those extra additions cause resource leaks. Note: This leak is hiding another bug. In other words, fixing only the leak exposes a double-close race. Call to PHP_UV_FD_TO_ZVAL() in uv_fs_open() wraps the fd in a php_stream via php_stream_fopen_from_fd(). The stream owns that fd: when the resource hits refcount 0, the stream destructor calls close(fd). But uv_fs_close() also closes the same fd via libuv. The libuv close happens promptly (async, completes before the cb returns to the script). The stream destructor's close happens later, when the PHP-side resource finally drops to refcount 0. The extra refcount removed by this commit has been preventing that the refcount actually drops to 0, so the double-close bug has been fortunately(?) hidden. The double-close bug is fixed in this commit too.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves #118
First use of
PHP_UV_ZVAL_TO_FD()afterPHP_UV_FS_SETUP()already adds the refcount, and there are no matching DECREF for those explicit ADDREFs. So those extra additions cause resource leaks.Note: This leak is hiding another bug. In other words, fixing only the leak exposes a double-close race. Call to
PHP_UV_FD_TO_ZVAL()inuv_fs_open()wraps the fd in a php_stream viaphp_stream_fopen_from_fd(). The stream owns that fd: when the resource hits refcount 0, the stream destructor callsclose(fd). Butuv_fs_close()also closes the same fd via libuv. The libuv close happens promptly (async, completes before the cb returns to the script). The stream destructor's close happens later, when the PHP-side resource finally drops to refcount 0.The extra refcount removed by this PR has been preventing that the refcount actually drops to 0, so the double-close bug has been fortunately(?) hidden. The double-close bug is fixed in this PR too.
The following issues should be fixed by this PR.
amphp/file#88
reactphp/filesystem#120