Fix GH-16812: UAF on readline_info() after readline_write_history() c… #16813
Conversation
There was a problem hiding this comment.
This doesn't seem to be related to calling readline_write_history() upfront, but rather the issue can be replicated with two consecutive calls to readline_info('line_buffer', …'), e.g.
var_dump(readline_info('line_buffer', 'one'));
var_dump(readline_info('line_buffer', 'two'));This causes a UAF for me, without the patch. With the patch, the UAF is resolved, but the function doesn't behave as expected. Output should be:
string(0) ""
string(3) "one"
ext/readline/tests/gh16812.phpt
Outdated
| var_dump(readline_info('line_buffer', 'test')); | ||
| ?> | ||
| --EXPECT-- | ||
| string(4) "test" |
There was a problem hiding this comment.
This doesn't look right; I think readline_info() is supposed to return the old line_buffer which should be an empty string.
| @@ -181,7 +181,7 @@ PHP_FUNCTION(readline_info) | |||
| add_assoc_long(return_value,"attempted_completion_over",rl_attempted_completion_over); | |||
| } else { | |||
| if (zend_string_equals_literal_ci(what,"line_buffer")) { | |||
| oldstr = rl_line_buffer; | |||
| oldstr = strdup(rl_line_buffer ? rl_line_buffer : ""); | |||
There was a problem hiding this comment.
The strdup("") doesn't seem to be necessary (that's already handled by SAFE_STRING(). It's not wrong though, and performance is likely irrelevant here.
| free(oldstr); | ||
| oldstr = strdup(rl_line_buffer ? rl_line_buffer : ""); |
There was a problem hiding this comment.
Since we touch this code path (libreadline only), we should also run the test against libreadline.
| @@ -208,6 +209,7 @@ PHP_FUNCTION(readline_info) | |||
| #endif | |||
| } | |||
| RETVAL_STRING(SAFE_STRING(oldstr)); | |||
| free(oldstr); | |||
…all.