Skip to content

ext/pdo: Refactor PDO::FETCH_FUNC to not rely on an FCI on the struct #17420

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 3 commits into from
Jan 9, 2025
Merged

ext/pdo: Refactor PDO::FETCH_FUNC to not rely on an FCI on the struct #17420

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
718d7d1
ext/pdo: Refactor PDO::FETCH_FUNC to not rely on an FCI on the struct
Girgias Jan 9, 2025
5d279f5
Review and remove unused var
Girgias Jan 9, 2025
e7e0f6f
[skip ci] Improve comment
Girgias Jan 9, 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
61 changes: 22 additions & 39 deletions ext/pdo/pdo_stmt.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -642,28 +642,6 @@ static bool do_fetch_class_prepare(pdo_stmt_t *stmt) /* {{{ */
}
/* }}} */

static bool pdo_stmt_init_fci_fcc_for_function_fetch_mode(pdo_stmt_t *stmt, zval *callable)
{
char *is_callable_error = NULL;

if (zend_fcall_info_init(callable, 0, &stmt->fetch.func.fci, &stmt->fetch.func.fcc, NULL, &is_callable_error) == FAILURE) {
if (is_callable_error) {
zend_type_error("%s", is_callable_error);
efree(is_callable_error);
} else {
zend_type_error("User-supplied function must be a valid callback");
}
return false;
}
ZEND_ASSERT(is_callable_error == NULL);

uint32_t num_args = stmt->column_count;
stmt->fetch.func.fci.param_count = num_args; /* probably less */
stmt->fetch.func.fci.params = safe_emalloc(sizeof(zval), num_args, 0);

return true;
}

static void do_fetch_opt_finish(pdo_stmt_t *stmt, bool free_ctor_agrs) /* {{{ */
{
/* fci.size is used to check if it is valid */
Expand Down Expand Up @@ -718,6 +696,8 @@ static bool do_fetch(pdo_stmt_t *stmt, zval *return_value, enum pdo_fetch_type h
zend_class_entry *ce = NULL, *old_ce = NULL;
zval retval, old_ctor_args = {{0}, {0}, {0}};
int i = 0;
zval *fetch_function_params = NULL;
uint32_t fetch_function_param_num = 0;

if (how == PDO_FETCH_USE_DEFAULT) {
how = stmt->default_fetch_type;
Expand Down Expand Up @@ -872,11 +852,14 @@ static bool do_fetch(pdo_stmt_t *stmt, zval *return_value, enum pdo_fetch_type h

case PDO_FETCH_FUNC:
/* TODO: Make this an assertion and ensure this is true higher up? */
if (!ZEND_FCI_INITIALIZED(stmt->fetch.func.fci)) {
if (!ZEND_FCC_INITIALIZED(stmt->fetch.func.fcc)) {
/* TODO ArgumentCountError? */
pdo_raise_impl_error(stmt->dbh, stmt, "HY000", "No fetch function specified");
return false;
}
fetch_function_param_num = stmt->column_count; /* probably less */
Copy link
Member

@kamil-tekiela kamil-tekiela Jan 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Isn't this variable redundant now? You set it to 0 after the call and then use it as idx was before.

Copy link
Member Author

Choose a reason for hiding this comment

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

In the follow-up PR, I'm getting rid of idx as it only serves for a check with the SERIALIZE flag

fetch_function_params = safe_emalloc(sizeof(zval), fetch_function_param_num, 0);
fetch_function_param_num = 0;
break;
EMPTY_SWITCH_DEFAULT_CASE();
}
Expand Down Expand Up @@ -971,7 +954,7 @@ static bool do_fetch(pdo_stmt_t *stmt, zval *return_value, enum pdo_fetch_type h
break;

case PDO_FETCH_FUNC:
ZVAL_COPY_VALUE(&stmt->fetch.func.fci.params[idx], &val);
ZVAL_COPY_VALUE(&fetch_function_params[fetch_function_param_num++], &val);
break;

default:
Expand Down Expand Up @@ -1006,21 +989,12 @@ static bool do_fetch(pdo_stmt_t *stmt, zval *return_value, enum pdo_fetch_type h
break;

case PDO_FETCH_FUNC:
stmt->fetch.func.fci.param_count = idx;
stmt->fetch.func.fci.retval = &retval;
if (zend_call_function(&stmt->fetch.func.fci, &stmt->fetch.func.fcc) == FAILURE) {
/* TODO Error? */
pdo_raise_impl_error(stmt->dbh, stmt, "HY000", "could not call user-supplied function");
return 0;
} else {
if (!Z_ISUNDEF(retval)) {
ZVAL_COPY_VALUE(return_value, &retval);
}
}
zend_call_known_fcc(&stmt->fetch.func.fcc, return_value, fetch_function_param_num, fetch_function_params, NULL);
Copy link
Member

Choose a reason for hiding this comment

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

I don't know what the difference is, so someone else will have to check if you're using this correctly. Maybe @nielsdos would like to take a look?

Copy link
Member

Choose a reason for hiding this comment

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

The difference is just that we no longer store an fci, but that we construct it on the fly. This reduces the memory usage a bit overall.

Copy link
Member

Choose a reason for hiding this comment

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

I also wonder what happens if the callback returns a reference, but that would be a pre-existing problem I suppose...

/* Free FCI parameters that were allocated in the previous loop */
for (uint32_t param_num = 0; param_num < stmt->fetch.func.fci.param_count; param_num++) {
zval_ptr_dtor(&stmt->fetch.func.fci.params[param_num]);
for (uint32_t param_num = 0; param_num < fetch_function_param_num; param_num++) {
zval_ptr_dtor(&fetch_function_params[param_num]);
}
efree(fetch_function_params);
break;

default:
Expand Down Expand Up @@ -1255,7 +1229,7 @@ PHP_METHOD(PDOStatement, fetchAll)
do_fetch_class_prepare(stmt);
break;

case PDO_FETCH_FUNC: /* Cannot be a default fetch mode */
case PDO_FETCH_FUNC: /* Cannot be a default fetch mode */ {
if (ZEND_NUM_ARGS() != 2) {
zend_string *func = get_active_function_or_method_name();
zend_argument_count_error("%s() expects exactly 2 argument for PDO::FETCH_FUNC, %d given",
Expand All @@ -1268,9 +1242,18 @@ PHP_METHOD(PDOStatement, fetchAll)
zend_argument_type_error(2, "must be a callable, null given");
RETURN_THROWS();
}
if (pdo_stmt_init_fci_fcc_for_function_fetch_mode(stmt, arg2) == false) {

char *is_callable_error = NULL;
if (!zend_is_callable_ex(arg2, NULL, 0, NULL, &stmt->fetch.func.fcc, &is_callable_error)) {
if (is_callable_error) {
zend_type_error("%s", is_callable_error);
efree(is_callable_error);
} else {
zend_type_error("User-supplied function must be a valid callback");
}
RETURN_THROWS();
}
}
break;

case PDO_FETCH_COLUMN:
Expand Down
3 changes: 1 addition & 2 deletions ext/pdo/php_pdo_driver.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -613,13 +613,12 @@ struct _pdo_stmt_t {
int column;
struct {
zval ctor_args; /* freed */
zend_fcall_info fci;
zend_fcall_info_cache fcc;
zend_fcall_info fci;
zend_class_entry *ce;
} cls;
struct {
zval dummy; /* This exists due to alignment reasons with fetch.into and fetch.cls.ctor_args */
zend_fcall_info fci;
zend_fcall_info_cache fcc;
} func;
zval into;
Expand Down
Loading