Skip to content

Commit 7e4c0c7

Browse files
committed
Fix GH-18139: Memory leak when overriding some settings via readline_info()
The reason why freeing was not done yet is because the pointer in these variables may be: - Static data set by the readline/libedit library initially, not heap data. - Data set by another thread. Although the libraries appear to be not thread-safe anyway. To solve this, introduce some TLS variables to hold a pointer for us when we override the settings, such that we can free them and are certain they are allocated by us.
1 parent bae78c6 commit 7e4c0c7

2 files changed

Lines changed: 37 additions & 9 deletions

File tree

ext/readline/readline.c

Lines changed: 19 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,9 @@ static zval _prepped_callback;
4747
static zval _readline_completion;
4848
static zval _readline_array;
4949

50+
ZEND_TLS char *php_readline_custom_readline_name = NULL;
51+
ZEND_TLS char *php_readline_custom_line_buffer = NULL;
52+
5053
PHP_MINIT_FUNCTION(readline);
5154
PHP_MSHUTDOWN_FUNCTION(readline);
5255
PHP_RSHUTDOWN_FUNCTION(readline);
@@ -146,7 +149,6 @@ PHP_FUNCTION(readline_info)
146149
zend_string *what = NULL;
147150
zval *value = NULL;
148151
size_t oldval;
149-
char *oldstr;
150152

151153
if (zend_parse_parameters(ZEND_NUM_ARGS(), "|S!z!", &what, &value) == FAILURE) {
152154
RETURN_THROWS();
@@ -181,15 +183,19 @@ PHP_FUNCTION(readline_info)
181183
add_assoc_long(return_value,"attempted_completion_over",rl_attempted_completion_over);
182184
} else {
183185
if (zend_string_equals_literal_ci(what,"line_buffer")) {
184-
oldstr = rl_line_buffer;
186+
RETVAL_STRING(SAFE_STRING(rl_line_buffer));
185187
if (value) {
186-
/* XXX if (rl_line_buffer) free(rl_line_buffer); */
187188
if (!try_convert_to_string(value)) {
188189
RETURN_THROWS();
189190
}
190-
rl_line_buffer = strdup(Z_STRVAL_P(value));
191+
char *copy = strdup(Z_STRVAL_P(value));
192+
/* XXX: This store would need to be atomic ideally or use a memory barrier */
193+
rl_line_buffer = copy;
194+
if (php_readline_custom_line_buffer) {
195+
free(php_readline_custom_line_buffer);
196+
}
197+
php_readline_custom_line_buffer = copy;
191198
}
192-
RETVAL_STRING(SAFE_STRING(oldstr));
193199
} else if (zend_string_equals_literal_ci(what, "point")) {
194200
RETVAL_LONG(rl_point);
195201
#ifndef PHP_WIN32
@@ -248,15 +254,19 @@ PHP_FUNCTION(readline_info)
248254
RETVAL_STRING((char *)SAFE_STRING(rl_library_version));
249255
#endif
250256
} else if (zend_string_equals_literal_ci(what, "readline_name")) {
251-
oldstr = (char*)rl_readline_name;
257+
RETVAL_STRING(SAFE_STRING(rl_readline_name));
252258
if (value) {
253-
/* XXX if (rl_readline_name) free(rl_readline_name); */
254259
if (!try_convert_to_string(value)) {
255260
RETURN_THROWS();
256261
}
257-
rl_readline_name = strdup(Z_STRVAL_P(value));
262+
char *copy = strdup(Z_STRVAL_P(value));
263+
/* XXX: This store would need to be atomic ideally or use a memory barrier */
264+
rl_readline_name = copy;
265+
if (php_readline_custom_readline_name) {
266+
free(php_readline_custom_readline_name);
267+
}
268+
php_readline_custom_readline_name = copy;
258269
}
259-
RETVAL_STRING(SAFE_STRING(oldstr));
260270
} else if (zend_string_equals_literal_ci(what, "attempted_completion_over")) {
261271
oldval = rl_attempted_completion_over;
262272
if (value) {

ext/readline/tests/gh18139.phpt

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
--TEST--
2+
GH-18139 (Memory leak when overriding some settings via readline_info())
3+
--EXTENSIONS--
4+
readline
5+
--FILE--
6+
<?php
7+
8+
var_dump(readline_info('readline_name', 'first'));
9+
var_dump(readline_info('readline_name', 'second'));
10+
var_dump(readline_info('line_buffer', 'third'));
11+
var_dump(readline_info('line_buffer', 'fourth'));
12+
13+
?>
14+
--EXPECTF--
15+
string(%d) "%S"
16+
string(5) "first"
17+
string(0) ""
18+
string(5) "third"

0 commit comments

Comments
 (0)