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 2 commits
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
76 changes: 32 additions & 44 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 @@ -716,8 +694,10 @@ static bool do_fetch(pdo_stmt_t *stmt, zval *return_value, enum pdo_fetch_type h
{
int flags, idx, old_arg_count = 0;
zend_class_entry *ce = NULL, *old_ce = NULL;
zval retval, old_ctor_args = {{0}, {0}, {0}};
zval 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,13 @@ 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;
}
/* We can probably infer items than stmt->column_count for some cases */
Copy link
Member

Choose a reason for hiding this comment

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

I didn't understand this sentence. Something gramatically makes this difficult for me to understand, I guess the "than" is out of place?

fetch_function_params = safe_emalloc(sizeof(zval), stmt->column_count, 0);
break;
EMPTY_SWITCH_DEFAULT_CASE();
}
Expand Down Expand Up @@ -971,7 +953,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 +988,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 @@ -1197,6 +1170,26 @@ PHP_METHOD(PDOStatement, fetchColumn)
}
/* }}} */

static bool pdo_get_fcc_from_zval(zend_fcall_info_cache *fcc, zval *callable) {
if (callable == NULL) {
/* TODO use "must be of type callable" format? */
zend_argument_type_error(2, "must be a callable, null given");
return false;
}

char *is_callable_error = NULL;
if (!zend_is_callable_ex(callable, NULL, 0, NULL, 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 false;
}
return true;
}

/* {{{ Returns an array of all of the results. */
PHP_METHOD(PDOStatement, fetchAll)
{
Expand Down Expand Up @@ -1263,12 +1256,7 @@ PHP_METHOD(PDOStatement, fetchAll)
zend_string_release(func);
RETURN_THROWS();
}
if (arg2 == NULL) {
/* TODO use "must be of type callable" format? */
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) {
if (!pdo_get_fcc_from_zval(&stmt->fetch.func.fcc, arg2)) {
RETURN_THROWS();
}
break;
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