Skip to content

ext/pcre: Refactor preg_replace_callback(_array)() to not pass a useless FCI #17365

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.

Sign up for GitHub

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

Merged
merged 10 commits into from
Jan 8, 2025
Merged

ext/pcre: Refactor preg_replace_callback(_array)() to not pass a useless FCI #17365

Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
69283ba
ext/pcre: Refactor php_pcre_replace_func_impl() to not rely on an FCI
Girgias Jan 5, 2025
61c0db9
ext/pcre: Refactor populate_subpat_array() to take subject as a HashT…
Girgias Jan 5, 2025
ca2af27
ext/pcre: Refactor php_pcre_replace_func_impl()
Girgias Jan 5, 2025
d759628
ext/pcre: Refactor php_pcre_replace_func()
Girgias Jan 5, 2025
0980e45
ext/pcre: Refactor php_replace_in_subject_func()
Girgias Jan 5, 2025
f3c981a
ext/pcre: Refactor preg_replace_func_impl()
Girgias Jan 5, 2025
8766b4c
ext/pcre: Add trampoline tests for preg_replace_callback(_array)()
Girgias Jan 5, 2025
10435d2
ext/pcre: Handle trampolines properly for preg_replace_callback(_arra…
Girgias Jan 5, 2025
06a72af
Fix test
Girgias Jan 7, 2025
e60ebc0
Revert FCI passing removal
Girgias Jan 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 13 additions & 25 deletions ext/pcre/php_pcre.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1544,40 +1544,26 @@ static int preg_get_backref(char **str, int *backref)
}
/* }}} */

/* {{{ preg_do_repl_func */
static zend_string *preg_do_repl_func(zend_fcall_info *fci, zend_fcall_info_cache *fcc, const char *subject, PCRE2_SIZE *offsets, zend_string **subpat_names, uint32_t num_subpats, int count, const PCRE2_SPTR mark, zend_long flags)
/* Return NULL if an exception has occurred */
static zend_string *preg_do_repl_func(zend_fcall_info_cache *fcc, const char *subject, PCRE2_SIZE *offsets, zend_string **subpat_names, uint32_t num_subpats, int count, const PCRE2_SPTR mark, zend_long flags)
{
zend_string *result_str;
zend_string *result_str = NULL;
zval retval; /* Function return value */
zval arg; /* Argument to pass to function */

array_init_size(&arg, count + (mark ? 1 : 0));
populate_subpat_array(&arg, subject, offsets, subpat_names, num_subpats, count, mark, flags);

fci->retval = &retval;
fci->param_count = 1;
fci->params = &arg;

if (zend_call_function(fci, fcc) == SUCCESS && Z_TYPE(retval) != IS_UNDEF) {
if (EXPECTED(Z_TYPE(retval) == IS_STRING)) {
result_str = Z_STR(retval);
} else {
result_str = zval_get_string_func(&retval);
zval_ptr_dtor(&retval);
}
} else {
if (!EG(exception)) {
php_error_docref(NULL, E_WARNING, "Unable to call custom replacement function");
}

result_str = zend_string_init(&subject[offsets[0]], offsets[1] - offsets[0], 0);
}

zend_call_known_fcc(fcc, &retval, 1, &arg, NULL);
zval_ptr_dtor(&arg);
/* No Exception has occurred */
if (EXPECTED(Z_TYPE(retval) != IS_UNDEF)) {
result_str = zval_try_get_string(&retval);
}
zval_ptr_dtor(&retval);

return result_str;
}
/* }}} */

/* {{{ php_pcre_replace */
PHPAPI zend_string *php_pcre_replace(zend_string *regex,
Expand Down Expand Up @@ -1940,10 +1926,12 @@ static zend_string *php_pcre_replace_func_impl(pcre_cache_entry *pce, zend_strin

/* Use custom function to get replacement string and its length. */
eval_result = preg_do_repl_func(
fci, fcc, subject, offsets, subpat_names, num_subpats, count,
fcc, subject, offsets, subpat_names, num_subpats, count,
pcre2_get_mark(match_data), flags);

ZEND_ASSERT(eval_result);
if (UNEXPECTED(eval_result == NULL)) {
goto error;
Copy link
Member

Choose a reason for hiding this comment

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

I'm not entirely sure this is right. This error path calls into pcre_handle_exec_error(count) where count represents an error code normally. However, in this call this is a real count and not an error code.
I was first thinking that you should probably set PCRE_G(error_code) yourself. However, perhaps not even that is necessary because we're throwing a userland exception anyway (which is not a pcre error itself).

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, I wasn't exactly sure myself here.

}
new_len = zend_safe_address_guarded(1, ZSTR_LEN(eval_result) + ZSTR_MAX_OVERHEAD, new_len) -ZSTR_MAX_OVERHEAD;
if (new_len >= alloc_len) {
alloc_len = zend_safe_address_guarded(2, new_len, ZSTR_MAX_OVERHEAD) - ZSTR_MAX_OVERHEAD;
Expand Down