Skip to content

Commit 7038e67

Browse files
GirgiasGirgias
committed
ext/pdo: Convert def_stmt_ctor_args field from zval to Hashtable pointer
1 parent ec0d972 commit 7038e67

6 files changed

+209
-32
lines changed

‎ext/pdo/pdo_dbh.c

Lines changed: 37 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -434,11 +434,9 @@ PHP_METHOD(PDO, __construct)
434434
}
435435
/* }}} */
436436

437-
static zval *pdo_stmt_instantiate(pdo_dbh_t *dbh, zval *object, zend_class_entry *dbstmt_ce, zval *ctor_args) /* {{{ */
437+
static zval *pdo_stmt_instantiate(pdo_dbh_t *dbh, zval *object, zend_class_entry *dbstmt_ce, const HashTable *ctor_args) /* {{{ */
438438
{
439-
if (!Z_ISUNDEF_P(ctor_args)) {
440-
/* This implies an error within PDO if this does not hold */
441-
ZEND_ASSERT(Z_TYPE_P(ctor_args) == IS_ARRAY);
439+
if (ctor_args) {
442440
if (!dbstmt_ce->constructor) {
443441
zend_throw_error(NULL, "User-supplied statement does not accept constructor arguments");
444442
return NULL;
@@ -475,8 +473,9 @@ PHP_METHOD(PDO, prepare)
475473
{
476474
pdo_stmt_t *stmt;
477475
zend_string *statement;
478-
zval *options = NULL, *value, *item, ctor_args;
476+
zval *options = NULL, *value, *item;
479477
zend_class_entry *dbstmt_ce, *pce;
478+
/* const */ HashTable *ctor_args = NULL;
480479
pdo_dbh_object_t *dbh_obj = Z_PDO_OBJECT_P(ZEND_THIS);
481480
pdo_dbh_t *dbh = dbh_obj->inner;
482481

@@ -525,16 +524,14 @@ PHP_METHOD(PDO, prepare)
525524
zend_zval_value_name(value));
526525
RETURN_THROWS();
527526
}
528-
ZVAL_COPY_VALUE(&ctor_args, item);
529-
} else {
530-
ZVAL_UNDEF(&ctor_args);
527+
ctor_args = Z_ARRVAL_P(item);
531528
}
532529
} else {
533530
dbstmt_ce = dbh->def_stmt_ce;
534-
ZVAL_COPY_VALUE(&ctor_args, &dbh->def_stmt_ctor_args);
531+
ctor_args = dbh->def_stmt_ctor_args;
535532
}
536533

537-
if (!pdo_stmt_instantiate(dbh, return_value, dbstmt_ce, &ctor_args)) {
534+
if (!pdo_stmt_instantiate(dbh, return_value, dbstmt_ce, ctor_args)) {
538535
RETURN_THROWS();
539536
}
540537
stmt = Z_PDO_STMT_P(return_value);
@@ -549,11 +546,7 @@ PHP_METHOD(PDO, prepare)
549546
ZVAL_UNDEF(&stmt->lazy_object_ref);
550547

551548
if (dbh->methods->preparer(dbh, statement, stmt, options)) {
552-
if (Z_TYPE(ctor_args) == IS_ARRAY) {
553-
pdo_stmt_construct(stmt, return_value, dbstmt_ce, Z_ARRVAL(ctor_args));
554-
} else {
555-
pdo_stmt_construct(stmt, return_value, dbstmt_ce, /* ctor_args */ NULL);
556-
}
549+
pdo_stmt_construct(stmt, return_value, dbstmt_ce, ctor_args);
557550
return;
558551
}
559552

@@ -817,17 +810,19 @@ static bool pdo_dbh_attribute_set(pdo_dbh_t *dbh, zend_long attr, zval *value) /
817810
return false;
818811
}
819812
dbh->def_stmt_ce = pce;
820-
if (!Z_ISUNDEF(dbh->def_stmt_ctor_args)) {
821-
zval_ptr_dtor(&dbh->def_stmt_ctor_args);
822-
ZVAL_UNDEF(&dbh->def_stmt_ctor_args);
813+
if (dbh->def_stmt_ctor_args) {
814+
zend_array_release(dbh->def_stmt_ctor_args);
815+
dbh->def_stmt_ctor_args = NULL;
823816
}
824817
if ((item = zend_hash_index_find(Z_ARRVAL_P(value), 1)) != NULL) {
825818
if (Z_TYPE_P(item) != IS_ARRAY) {
826819
zend_type_error("PDO::ATTR_STATEMENT_CLASS constructor_args must be of type ?array, %s given",
827820
zend_zval_value_name(value));
828821
return false;
829822
}
830-
ZVAL_COPY(&dbh->def_stmt_ctor_args, item);
823+
dbh->def_stmt_ctor_args = Z_ARRVAL_P(item);
824+
/* Increase refcount */
825+
GC_TRY_ADDREF(dbh->def_stmt_ctor_args);
831826
}
832827
return true;
833828
}
@@ -906,9 +901,10 @@ PHP_METHOD(PDO, getAttribute)
906901
case PDO_ATTR_STATEMENT_CLASS:
907902
array_init(return_value);
908903
add_next_index_str(return_value, zend_string_copy(dbh->def_stmt_ce->name));
909-
if (!Z_ISUNDEF(dbh->def_stmt_ctor_args)) {
910-
Z_TRY_ADDREF(dbh->def_stmt_ctor_args);
911-
add_next_index_zval(return_value, &dbh->def_stmt_ctor_args);
904+
if (dbh->def_stmt_ctor_args) {
905+
/* Increment refcount of constructor arguments */
906+
GC_TRY_ADDREF(dbh->def_stmt_ctor_args);
907+
add_next_index_array(return_value, dbh->def_stmt_ctor_args);
912908
}
913909
return;
914910
case PDO_ATTR_DEFAULT_FETCH_MODE:
@@ -1093,7 +1089,7 @@ PHP_METHOD(PDO, query)
10931089

10941090
PDO_DBH_CLEAR_ERR();
10951091

1096-
if (!pdo_stmt_instantiate(dbh, return_value, dbh->def_stmt_ce, &dbh->def_stmt_ctor_args)) {
1092+
if (!pdo_stmt_instantiate(dbh, return_value, dbh->def_stmt_ce, dbh->def_stmt_ctor_args)) {
10971093
RETURN_THROWS();
10981094
}
10991095
stmt = Z_PDO_STMT_P(return_value);
@@ -1122,11 +1118,7 @@ PHP_METHOD(PDO, query)
11221118
stmt->executed = 1;
11231119
}
11241120
if (ret) {
1125-
if (Z_TYPE(dbh->def_stmt_ctor_args) == IS_ARRAY) {
1126-
pdo_stmt_construct(stmt, return_value, dbh->def_stmt_ce, Z_ARRVAL(dbh->def_stmt_ctor_args));
1127-
} else {
1128-
pdo_stmt_construct(stmt, return_value, dbh->def_stmt_ce, /* ctor_args */ NULL);
1129-
}
1121+
pdo_stmt_construct(stmt, return_value, dbh->def_stmt_ce, dbh->def_stmt_ctor_args);
11301122
return;
11311123
}
11321124
}
@@ -1320,7 +1312,9 @@ static HashTable *dbh_get_gc(zend_object *object, zval **gc_data, int *gc_count)
13201312
{
13211313
pdo_dbh_t *dbh = php_pdo_dbh_fetch_inner(object);
13221314
zend_get_gc_buffer *gc_buffer = zend_get_gc_buffer_create();
1323-
zend_get_gc_buffer_add_zval(gc_buffer, &dbh->def_stmt_ctor_args);
1315+
if (dbh->def_stmt_ctor_args) {
1316+
zend_get_gc_buffer_add_ht(gc_buffer, dbh->def_stmt_ctor_args);
1317+
}
13241318
if (dbh->methods && dbh->methods->get_gc) {
13251319
dbh->methods->get_gc(dbh, gc_buffer);
13261320
}
@@ -1382,8 +1376,18 @@ static void dbh_free(pdo_dbh_t *dbh, bool free_persistent)
13821376
pefree((char *)dbh->persistent_id, dbh->is_persistent);
13831377
}
13841378

1385-
if (!Z_ISUNDEF(dbh->def_stmt_ctor_args)) {
1386-
zval_ptr_dtor(&dbh->def_stmt_ctor_args);
1379+
/* When the GC is running, the ctor_args will be added to the GC buffer marked to be released.
1380+
* However, before freeing the buffer, it will release the object by calling zend_objects_store_del()
1381+
* which calls pdo_dbh_free_storage() which in turn calls this function.
1382+
* Thus we must not free the ctor_args when the GC is running. */
1383+
if (dbh->def_stmt_ctor_args) {
1384+
zend_gc_status gc_status;
1385+
zend_gc_get_status(&gc_status);
1386+
1387+
if (EXPECTED(!gc_status.active)) {
1388+
zend_array_release(dbh->def_stmt_ctor_args);
1389+
dbh->def_stmt_ctor_args = NULL;
1390+
}
13871391
}
13881392

13891393
for (i = 0; i < PDO_DBH_DRIVER_METHOD_KIND__MAX; i++) {
@@ -1413,6 +1417,7 @@ static void pdo_dbh_free_storage(zend_object *std)
14131417
if (dbh->is_persistent && dbh->methods && dbh->methods->persistent_shutdown) {
14141418
dbh->methods->persistent_shutdown(dbh);
14151419
}
1420+
14161421
zend_object_std_dtor(std);
14171422
dbh_free(dbh, 0);
14181423
}
@@ -1427,6 +1432,7 @@ zend_object *pdo_dbh_new(zend_class_entry *ce)
14271432
rebuild_object_properties(&dbh->std);
14281433
dbh->inner = ecalloc(1, sizeof(pdo_dbh_t));
14291434
dbh->inner->def_stmt_ce = pdo_dbstmt_ce;
1435+
dbh->inner->def_stmt_ctor_args = NULL;
14301436

14311437
return &dbh->std;
14321438
}

‎ext/pdo/php_pdo_driver.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -487,7 +487,7 @@ struct _pdo_dbh_t {
487487

488488
zend_class_entry *def_stmt_ce;
489489

490-
zval def_stmt_ctor_args;
490+
HashTable *def_stmt_ctor_args;
491491

492492
/* when calling PDO::query(), we need to keep the error
493493
* context from the statement around until we next clear it.

‎ext/pdo/tests/pdo_ATTR_STATEMENT_CLASS_ctor_arg_gc.phpt

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
--TEST--
2+
PDO: Set PDOStatement class with ctor_args that are freed with GC intervention
3+
--EXTENSIONS--
4+
pdo
5+
--SKIPIF--
6+
<?php
7+
$dir = getenv('REDIR_TEST_DIR');
8+
if (false == $dir) die('skip no driver');
9+
require_once $dir . 'pdo_test.inc';
10+
PDOTest::skip();
11+
?>
12+
--FILE--
13+
<?php
14+
if (getenv('REDIR_TEST_DIR') === false) putenv('REDIR_TEST_DIR='.__DIR__ . '/../../pdo/tests/');
15+
require_once getenv('REDIR_TEST_DIR') . 'pdo_test.inc';
16+
17+
class Foo extends PDOStatement {
18+
private function __construct($v) {
19+
var_dump($v);
20+
}
21+
}
22+
23+
class Bar extends PDO {
24+
public $statementClass = 'Foo';
25+
function __construct($dsn, $username, $password, $driver_options = []) {
26+
$driver_options[PDO::ATTR_ERRMODE] = PDO::ERRMODE_EXCEPTION;
27+
parent::__construct($dsn, $username, $password, $driver_options);
28+
29+
$this->setAttribute(PDO::ATTR_STATEMENT_CLASS, [$this->statementClass, [$this]]);
30+
}
31+
}
32+
33+
$db = PDOTest::factory(Bar::class);
34+
35+
$stmt = $db->query('SELECT 1');
36+
var_dump($stmt);
37+
38+
?>
39+
--EXPECT--
40+
object(Bar)#1 (1) {
41+
["statementClass"]=>
42+
string(3) "Foo"
43+
}
44+
object(Foo)#2 (1) {
45+
["queryString"]=>
46+
string(8) "SELECT 1"
47+
}

‎ext/pdo/tests/pdo_ATTR_STATEMENT_CLASS_ctor_arg_no-gc.phpt

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
--TEST--
2+
PDO: Set PDOStatement class with ctor_args that are freed without GC intervention
3+
--EXTENSIONS--
4+
pdo
5+
--SKIPIF--
6+
<?php
7+
$dir = getenv('REDIR_TEST_DIR');
8+
if (false == $dir) die('skip no driver');
9+
require_once $dir . 'pdo_test.inc';
10+
PDOTest::skip();
11+
?>
12+
--FILE--
13+
<?php
14+
if (getenv('REDIR_TEST_DIR') === false) putenv('REDIR_TEST_DIR='.__DIR__ . '/../../pdo/tests/');
15+
require_once getenv('REDIR_TEST_DIR') . 'pdo_test.inc';
16+
17+
class Foo extends PDOStatement {
18+
private function __construct($v) {
19+
var_dump($v);
20+
}
21+
}
22+
23+
$db = PDOTest::factory();
24+
25+
$db->setAttribute(PDO::ATTR_STATEMENT_CLASS, array('Foo', ['param1']));
26+
$stmt = $db->query('SELECT 1');
27+
var_dump($stmt);
28+
29+
?>
30+
--EXPECT--
31+
string(6) "param1"
32+
object(Foo)#2 (1) {
33+
["queryString"]=>
34+
string(8) "SELECT 1"
35+
}

‎ext/pdo/tests/pdo_prepare_ATTR_STATEMENT_CLASS_ctor_arg_gc.phpt

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
--TEST--
2+
PDO: Set PDOStatement class with ctor_args that are freed with GC intervention in PDO::prepare()
3+
--EXTENSIONS--
4+
pdo
5+
--SKIPIF--
6+
<?php
7+
$dir = getenv('REDIR_TEST_DIR');
8+
if (false == $dir) die('skip no driver');
9+
require_once $dir . 'pdo_test.inc';
10+
PDOTest::skip();
11+
?>
12+
--FILE--
13+
<?php
14+
if (getenv('REDIR_TEST_DIR') === false) putenv('REDIR_TEST_DIR='.__DIR__ . '/../../pdo/tests/');
15+
require_once getenv('REDIR_TEST_DIR') . 'pdo_test.inc';
16+
17+
class Foo extends PDOStatement {
18+
private function __construct($v) {
19+
var_dump($v);
20+
}
21+
}
22+
23+
class Bar extends PDO {
24+
public $statementClass = 'Foo';
25+
function __construct($dsn, $username, $password, $driver_options = []) {
26+
$driver_options[PDO::ATTR_ERRMODE] = PDO::ERRMODE_EXCEPTION;
27+
parent::__construct($dsn, $username, $password, $driver_options);
28+
29+
$this->setAttribute(PDO::ATTR_STATEMENT_CLASS, [$this->statementClass, [$this]]);
30+
}
31+
}
32+
33+
$db = PDOTest::factory(Bar::class);
34+
35+
$stmt = $db->prepare('SELECT 1');
36+
var_dump($stmt);
37+
$r = $stmt->execute();
38+
var_dump($r);
39+
40+
?>
41+
--EXPECT--
42+
object(Bar)#1 (1) {
43+
["statementClass"]=>
44+
string(3) "Foo"
45+
}
46+
object(Foo)#2 (1) {
47+
["queryString"]=>
48+
string(8) "SELECT 1"
49+
}
50+
bool(true)

‎ext/pdo/tests/pdo_prepare_ATTR_STATEMENT_CLASS_ctor_arg_no-gc.phpt

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
--TEST--
2+
PDO: Set PDOStatement class with ctor_args that are freed without GC intervention in PDO::prepare()
3+
--EXTENSIONS--
4+
pdo
5+
--SKIPIF--
6+
<?php
7+
$dir = getenv('REDIR_TEST_DIR');
8+
if (false == $dir) die('skip no driver');
9+
require_once $dir . 'pdo_test.inc';
10+
PDOTest::skip();
11+
?>
12+
--FILE--
13+
<?php
14+
if (getenv('REDIR_TEST_DIR') === false) putenv('REDIR_TEST_DIR='.__DIR__ . '/../../pdo/tests/');
15+
require_once getenv('REDIR_TEST_DIR') . 'pdo_test.inc';
16+
17+
class Foo extends PDOStatement {
18+
private function __construct($v) {
19+
var_dump($v);
20+
}
21+
}
22+
23+
$db = PDOTest::factory();
24+
$db->setAttribute(PDO::ATTR_STATEMENT_CLASS, ['Foo', ['param1']]);
25+
26+
//$stmt = $db->prepare('SELECT 1', [PDO::ATTR_STATEMENT_CLASS => ['Foo', ['param1']] ]);
27+
$stmt = $db->prepare('SELECT 1');
28+
var_dump($stmt);
29+
$r = $stmt->execute();
30+
var_dump($r);
31+
32+
?>
33+
--EXPECT--
34+
string(6) "param1"
35+
object(Foo)#2 (1) {
36+
["queryString"]=>
37+
string(8) "SELECT 1"
38+
}
39+
bool(true)

0 commit comments

Comments
 (0)