Skip to content

ext/pdo: Refactor function and class statement fetch modes to reduce memory requirements #17289

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 function and class statement fetch modes to reduce memory requirements #17289

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
82dd908
ext/pdo: Move PDO MySQL test to common
Girgias Jan 8, 2025
67bacc8
ext/pdo: Refactor FETCH_FUNCTION mode
Girgias Dec 29, 2024
ad42595
ext/pdo: Remove mostly useless retval field in statement struct
Girgias Dec 29, 2024
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
93 changes: 32 additions & 61 deletions ext/pdo/pdo_stmt.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -625,7 +625,6 @@ static bool do_fetch_class_prepare(pdo_stmt_t *stmt) /* {{{ */

if (ce->constructor) {
ZVAL_UNDEF(&fci->function_name);
fci->retval = &stmt->fetch.cls.retval;
fci->param_count = 0;
fci->params = NULL;

Expand All @@ -643,11 +642,11 @@ static bool do_fetch_class_prepare(pdo_stmt_t *stmt) /* {{{ */
}
/* }}} */

static bool make_callable_ex(pdo_stmt_t *stmt, zval *callable, zend_fcall_info * fci, zend_fcall_info_cache * fcc, int num_args) /* {{{ */
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, fci, fcc, NULL, &is_callable_error) == FAILURE) {
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);
Expand All @@ -656,33 +655,16 @@ static bool make_callable_ex(pdo_stmt_t *stmt, zval *callable, zend_fcall_info *
}
return false;
}
if (is_callable_error) {
/* Possible error message */
efree(is_callable_error);
}
ZEND_ASSERT(is_callable_error == NULL);
Copy link
Member

Choose a reason for hiding this comment

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

Is this really necessary? Shouldn't zend_fcall_info_init report failure reliably?

Copy link
Member Author

Choose a reason for hiding this comment

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

The point is that there shouldn't be an error message to free if zend_fcall_info_init has failed.

This just adds an assertion that this indeed can never be the case.


fci->param_count = num_args; /* probably less */
fci->params = safe_emalloc(sizeof(zval), num_args, 0);
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 bool do_fetch_func_prepare(pdo_stmt_t *stmt) /* {{{ */
{
zend_fcall_info *fci = &stmt->fetch.cls.fci;
zend_fcall_info_cache *fcc = &stmt->fetch.cls.fcc;

if (!make_callable_ex(stmt, &stmt->fetch.func.function, fci, fcc, stmt->column_count)) {
return false;
} else {
stmt->fetch.func.values = safe_emalloc(sizeof(zval), stmt->column_count, 0);
return true;
}
}
/* }}} */

static void do_fetch_opt_finish(pdo_stmt_t *stmt, int free_ctor_agrs) /* {{{ */
static void do_fetch_opt_finish(pdo_stmt_t *stmt, bool free_ctor_agrs) /* {{{ */
{
/* fci.size is used to check if it is valid */
if (stmt->fetch.cls.fci.size && stmt->fetch.cls.fci.params) {
Expand All @@ -701,10 +683,6 @@ static void do_fetch_opt_finish(pdo_stmt_t *stmt, int free_ctor_agrs) /* {{{ */
ZVAL_UNDEF(&stmt->fetch.cls.ctor_args);
stmt->fetch.cls.fci.param_count = 0;
}
if (stmt->fetch.func.values) {
efree(stmt->fetch.func.values);
stmt->fetch.func.values = NULL;
}
}
/* }}} */

Expand Down Expand Up @@ -860,18 +838,18 @@ static bool do_fetch(pdo_stmt_t *stmt, zval *return_value, enum pdo_fetch_type h
}
}
if (ce->constructor && (flags & PDO_FETCH_PROPS_LATE)) {
zval retval_constructor_call;
stmt->fetch.cls.fci.retval = &retval_constructor_call;
stmt->fetch.cls.fci.object = Z_OBJ_P(return_value);
stmt->fetch.cls.fcc.object = Z_OBJ_P(return_value);
if (zend_call_function(&stmt->fetch.cls.fci, &stmt->fetch.cls.fcc) == FAILURE) {
/* TODO Error? */
pdo_raise_impl_error(stmt->dbh, stmt, "HY000", "could not call class constructor");
return 0;
} else {
if (!Z_ISUNDEF(stmt->fetch.cls.retval)) {
zval_ptr_dtor(&stmt->fetch.cls.retval);
ZVAL_UNDEF(&stmt->fetch.cls.retval);
}
zend_call_function(&stmt->fetch.cls.fci, &stmt->fetch.cls.fcc);
if (Z_TYPE(retval_constructor_call) == IS_UNDEF) {
/* Exception has happened */
zval_ptr_dtor(return_value);
return false;
}
zval_ptr_dtor(&retval_constructor_call);
Copy link
Member

Choose a reason for hiding this comment

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

Why this doesn't need the Z_ISUNDEF check anymore?

Copy link
Member Author

Choose a reason for hiding this comment

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

IS_UNDEF only happens if there is an exception that is thrown by the constructor.
Actually the thing that is pointless is to check for FAILURE as the result of zend_call_function() as that can never happen as we are not in shutdown.

ZVAL_UNDEF(stmt->fetch.cls.fci.retval);
}
}
break;
Expand All @@ -894,16 +872,10 @@ 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 (Z_ISUNDEF(stmt->fetch.func.function)) {
if (!ZEND_FCI_INITIALIZED(stmt->fetch.func.fci)) {
/* TODO ArgumentCountError? */
pdo_raise_impl_error(stmt->dbh, stmt, "HY000", "No fetch function specified");
return 0;
}
if (!stmt->fetch.func.fci.size) {
if (!do_fetch_func_prepare(stmt))
{
return 0;
}
return false;
}
break;
EMPTY_SWITCH_DEFAULT_CASE();
Expand Down Expand Up @@ -1002,8 +974,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.values[idx], &val);
ZVAL_COPY_VALUE(&stmt->fetch.cls.fci.params[idx], &stmt->fetch.func.values[idx]);
ZVAL_COPY_VALUE(&stmt->fetch.func.fci.params[idx], &val);
break;

default:
Expand All @@ -1016,17 +987,18 @@ static bool do_fetch(pdo_stmt_t *stmt, zval *return_value, enum pdo_fetch_type h
switch (how) {
case PDO_FETCH_CLASS:
if (ce->constructor && !(flags & (PDO_FETCH_PROPS_LATE | PDO_FETCH_SERIALIZE))) {
zval retval_constructor_call;
stmt->fetch.cls.fci.retval = &retval_constructor_call;
stmt->fetch.cls.fci.object = Z_OBJ_P(return_value);
stmt->fetch.cls.fcc.object = Z_OBJ_P(return_value);
if (zend_call_function(&stmt->fetch.cls.fci, &stmt->fetch.cls.fcc) == FAILURE) {
/* TODO Error? */
pdo_raise_impl_error(stmt->dbh, stmt, "HY000", "could not call class constructor");
return 0;
} else {
if (!Z_ISUNDEF(stmt->fetch.cls.retval)) {
zval_ptr_dtor(&stmt->fetch.cls.retval);
}
zend_call_function(&stmt->fetch.cls.fci, &stmt->fetch.cls.fcc);
if (Z_TYPE(retval_constructor_call) == IS_UNDEF) {
/* Exception has happened */
zval_ptr_dtor(return_value);
return false;
}
zval_ptr_dtor(&retval_constructor_call);
ZVAL_UNDEF(stmt->fetch.cls.fci.retval);
}
if (flags & PDO_FETCH_CLASSTYPE) {
do_fetch_opt_finish(stmt, 0);
Expand All @@ -1048,8 +1020,9 @@ static bool do_fetch(pdo_stmt_t *stmt, zval *return_value, enum pdo_fetch_type h
ZVAL_COPY_VALUE(return_value, &retval);
}
}
while (idx--) {
zval_ptr_dtor(&stmt->fetch.func.values[idx]);
/* 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]);
}
break;

Expand Down Expand Up @@ -1298,9 +1271,7 @@ PHP_METHOD(PDOStatement, fetchAll)
zend_argument_type_error(2, "must be a callable, null given");
RETURN_THROWS();
}
/* TODO Check it is a callable? */
ZVAL_COPY_VALUE(&stmt->fetch.func.function, arg2);
if (do_fetch_func_prepare(stmt) == false) {
if (pdo_stmt_init_fci_fcc_for_function_fetch_mode(stmt, arg2) == false) {
RETURN_THROWS();
}
break;
Expand Down
6 changes: 1 addition & 5 deletions ext/pdo/php_pdo_driver.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -615,16 +615,12 @@ struct _pdo_stmt_t {
zval ctor_args; /* freed */
zend_fcall_info fci;
zend_fcall_info_cache fcc;
zval retval;
zend_class_entry *ce;
} cls;
struct {
zval fetch_args; /* freed */
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;
zval object;
zval function;
zval *values; /* freed */
} func;
zval into;
} fetch;
Expand Down
42 changes: 18 additions & 24 deletions ...pdo_mysql_stmt_fetchobject_ctor_args.phpt → ...tests/pdo_stmt_fetchobject_ctor_args.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,33 +1,27 @@
--TEST--
MySQL PDO: PDOStatement->fetchObject() with $constructorArgs
PDO Common: PDOStatement->fetchObject() with $constructorArgs
--EXTENSIONS--
pdo_mysql
pdo
--SKIPIF--
<?php
require_once __DIR__ . '/inc/mysql_pdo_test.inc';
MySQLPDOTest::skip();
$db = MySQLPDOTest::factory();

try {
$query = "SELECT '', NULL, \"\" FROM DUAL";
$stmt = $db->prepare($query);
$ok = $stmt->execute();
} catch (PDOException $e) {
die("skip: Test cannot be run with SQL mode ANSI");
}
if (!$ok)
die("skip: Test cannot be run with SQL mode ANSI");
$dir = getenv('REDIR_TEST_DIR');
if (false == $dir) die('skip no driver');
require_once $dir . 'pdo_test.inc';
PDOTest::skip();
?>
--FILE--
<?php
require_once __DIR__ . '/inc/mysql_pdo_test.inc';
/** @var PDO $db */
$db = MySQLPDOTest::factory();
if (getenv('REDIR_TEST_DIR') === false) putenv('REDIR_TEST_DIR='.__DIR__ . '/../../pdo/tests/');
require_once getenv('REDIR_TEST_DIR') . 'pdo_test.inc';
$db = PDOTest::factory();

$table = 'pdo_mysql_stmt_fetchobject_ctor_args';
MySQLPDOTest::createTestTable($table, $db);
$table = 'pdo_stmt_fetchobject_ctor_args';
$db->exec("CREATE TABLE {$table} (id INT, label CHAR(1), PRIMARY KEY(id))");
$db->exec("INSERT INTO {$table} (id, label) VALUES (1, 'a')");
$db->exec("INSERT INTO {$table} (id, label) VALUES (2, 'b')");
$db->exec("INSERT INTO {$table} (id, label) VALUES (3, 'c')");

$query = "SELECT id FROM {$table} ORDER BY id ASC LIMIT 1";
$query = "SELECT id FROM {$table} ORDER BY id ASC";
$stmt = $db->prepare($query);

class Foo {
Expand Down Expand Up @@ -79,9 +73,9 @@ try {
?>
--CLEAN--
<?php
require_once __DIR__ . '/inc/mysql_pdo_test.inc';
$db = MySQLPDOTest::factory();
$db->exec('DROP TABLE IF EXISTS pdo_mysql_stmt_fetchobject_ctor_args');
require_once getenv('REDIR_TEST_DIR') . 'pdo_test.inc';
$db = PDOTest::factory();
PDOTest::dropTableIfExists($db, "pdo_stmt_fetchobject_ctor_args");
?>
--EXPECTF--
Too few arguments to function Foo::__construct(), 0 passed and exactly 1 expected
Expand Down
Loading