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

Fix GH-17154: Memory leak on rl_line_buffer when no history (libedit). #17155

Open
wants to merge 1 commit into
base: PHP-8.3
Choose a base branch
from

Conversation

devnexen
Copy link
Member

When there is no history, any subsequent history api call initialising it will call rl_initialise() thus we lose the previous value in the process.
At module init time, we also call using_history() like with readline.

…it).

When there is no history, any subsequent history api call initialising
it will call rl_initialise() thus we lose the previous value in the
process.
At module init time, we also call using_history() like with readline.
@devnexen
Copy link
Member Author

devnexen commented Dec 14, 2024

Another approach is to call using_history at readline_info if there is no history (global history_length/history_length()), only for libedit.

@devnexen devnexen marked this pull request as ready for review December 14, 2024 07:52
@devnexen devnexen requested a review from cmb69 December 14, 2024 07:52
@devnexen devnexen linked an issue Dec 14, 2024 that may be closed by this pull request
@nielsdos
Copy link
Member

Another approach is to call using_history at readline_info if there is no history (global history_length/history_length()), only for libedit.

What you did now is probably better because there's less divergence in how the two libraries are set up that way. Makes it a bit more "clear" imo. I'll leave the final judgement to cmb though.
CI fails because there's a history file now which shows up in the git diff during validation of whether generated files are up to date.

@cmb69
Copy link
Member

cmb69 commented Dec 14, 2024

I've checked that on Windows and confirm a memory leak of 7 bytes. However, with this patch applied, I get a whole bunch of leaks (basically everything that has been setup by using_history()). That likely also happens when we call using_history() somewhere else, and I don't think we can do much about that (calling some finalization function during our shutdown sequence for non thread-safe libraries is always an issue; well, maybe not that much of a problem since the documentation discourages using ext/readline with ZTS SAPIs).

Maybe an even bigger problem with the suggested patch is that it likely unfixes https://bugs.php.net/65714.

Anyhow, it seems to me in this case we're just leaking rl_line_buffer which we may have allocated when readline_info() was called as setter as of b8ba6f6, so we could free it during request shutdown:

 ext/readline/readline.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/ext/readline/readline.c b/ext/readline/readline.c
index 30f84d6100..9e6a22903b 100644
--- a/ext/readline/readline.c
+++ b/ext/readline/readline.c
@@ -97,6 +97,10 @@ PHP_RSHUTDOWN_FUNCTION(readline)
 {
 	zval_ptr_dtor(&_readline_completion);
 	ZVAL_UNDEF(&_readline_completion);
+	if (rl_line_buffer) {
+		free(rl_line_buffer);
+		rl_line_buffer = NULL;
+	}
 #ifdef HAVE_RL_CALLBACK_READ_CHAR
 	if (Z_TYPE(_prepped_callback) != IS_UNDEF) {
 		rl_callback_handler_remove();

That would fix the leak on Windows at least (might be different with libeditline and libreadline).

By the way, without patch, I see 10 leaking tests in the ext/readline test suite; with my patch, gh16812.phpt no longer leaks. I think we should set up a CI job (probably nightly only) which runs with the Windows debug heap enabled, if we can't run LSan/MSan builds against uninstrumented libraries:

# msan requires all used libraries to be instrumented,
# so we should avoiding linking against anything but libc here

@devnexen
Copy link
Member Author

Anyhow, it seems to me in this case we're just leaking rl_line_buffer which we may have allocated when readline_info() was called as setter as of b8ba6f6, so we could free it during request shutdown:

 ext/readline/readline.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/ext/readline/readline.c b/ext/readline/readline.c
index 30f84d6100..9e6a22903b 100644
--- a/ext/readline/readline.c
+++ b/ext/readline/readline.c
@@ -97,6 +97,10 @@ PHP_RSHUTDOWN_FUNCTION(readline)
 {
 	zval_ptr_dtor(&_readline_completion);
 	ZVAL_UNDEF(&_readline_completion);
+	if (rl_line_buffer) {
+		free(rl_line_buffer);
+		rl_line_buffer = NULL;
+	}
 #ifdef HAVE_RL_CALLBACK_READ_CHAR
 	if (Z_TYPE(_prepped_callback) != IS_UNDEF) {
 		rl_callback_handler_remove();

That would fix the leak on Windows at least (might be different with libeditline and libreadline).

By the way, without patch, I see 10 leaking tests in the ext/readline test suite; with my patch, gh16812.phpt no longer leaks. I think we should set up a CI job (probably nightly only) which runs with the Windows debug heap enabled, if we can't run LSan/MSan builds against uninstrumented libraries:

# msan requires all used libraries to be instrumented,
# so we should avoiding linking against anything but libc here

But isn't rl_line_buffer value supposed to survive beyond a request ? It is also a library global (for better or worse).

@devnexen
Copy link
Member Author

However, with this patch applied, I get a whole bunch of leaks (basically everything that has been setup by using_history()).

Are they specific windows leaks ? locally all tests pass with ASAN

@cmb69
Copy link
Member

cmb69 commented Dec 14, 2024

But isn't rl_line_buffer value supposed to survive beyond a request ? It is also a library global (for better or worse).

Possibly, but then we could free it during module shutdown.

Are they specific windows leaks ? locally all tests pass with ASAN

I don't think they're Windows specific. At least some of these seem to be caused by calling using_history(), which allocates resources, but we never call anything to release these (I don't think there even is a finalize() or something like that).

And at least on Windows, ASan rarely reports a memory leak (if ever).

@devnexen
Copy link
Member Author

devnexen commented Dec 14, 2024

So I ve tried your approach (ie in request shutdown). (or did you mean in addition of my patch ?)

sapi/cli/php ~/a.php 

=================================================================
==1084733==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 7 byte(s) in 1 object(s) allocated from:
    #0 0x7fccee2eed80 in strdup ../../../../src/libsanitizer/asan/asan_interceptors.cpp:578
    #1 0x5625232d5d35 in zif_readline_info /home/dcarlier/php-src/ext/readline/readline.c:194
    #2 0x56252387e3e3 in ZEND_DO_ICALL_SPEC_RETVAL_UNUSED_HANDLER /home/dcarlier/php-src/Zend/zend_vm_execute.h:1275
    #3 0x5625239d228f in execute_ex /home/dcarlier/php-src/Zend/zend_vm_execute.h:57242
    #4 0x5625239e4459 in zend_execute /home/dcarlier/php-src/Zend/zend_vm_execute.h:61634
    #5 0x5625237de1ee in zend_execute_scripts /home/dcarlier/php-src/Zend/zend.c:1895
    #6 0x562523660434 in php_execute_script /home/dcarlier/php-src/main/main.c:2529
    #7 0x562523bbbabd in do_cli /home/dcarlier/php-src/sapi/cli/php_cli.c:966
    #8 0x562523bbd89f in main /home/dcarlier/php-src/sapi/cli/php_cli.c:1341
    #9 0x7fccedbdad67 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58

SUMMARY: AddressSanitizer: 7 byte(s) leaked in 1 allocation(s).

Note that the CI asan build is all green thus why I asked if your leaks are linked to win32 libedit implementation.

Copy link
Member

@cmb69 cmb69 left a comment

Choose a reason for hiding this comment

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

Ah, I was not aware that LSan is automatically enabled via ASan (there is no LSan for MSVC yet).

And after having a closer look at libeditland wineditline, I see that the implementations are quite different. write_history() may indeed call using_history() in libedit, but won't for wineditline (as such my patch fixed the issue there, but wouldn't work for libedit). Still it seems to me that rl_line_buffer is not freed with libedit, but maybe LSan is fine with that since it is a global variable (Windows debug heap, however, even reports that).

Anyhow, when calling using_history() (as with your patch), memory leaks are reported by the debug heap, since we're never calling free_history() (which appears to not be available in libedit). Calling free_history() during module shutdown helps with the test failures (and shows no more leaks for the given reproducer), but there are still a couple of issues. So indeed, that's Windows specific.

So I suggest to apply this PR; I will follow up with some improvements/fixes for Windows/wineditline.

And we really should drop support for libreadline (#15882), since otherwise we have to deal with three different libraries (even two are apparently difficult to handle).

@@ -0,0 +1,12 @@
--TEST--
GH-17154 readline_write_history()/readline_read_history(): memory leak
--EXTENSIONS--my
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
--EXTENSIONS--my
--EXTENSIONS--

Copy link
Member

@cmb69 cmb69 left a comment

Choose a reason for hiding this comment

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

Oops, forgot about https://bugs.php.net/65714. Not sure what to do about that. Maybe @remicollet can comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

memory leak readline
3 participants