From 5a437c717e8e170be5feccf14a676a0cce69d955 Mon Sep 17 00:00:00 2001 From: George Peter Banyard Date: Fri, 8 Sep 2023 14:51:08 +0100 Subject: [PATCH 1/4] Zend/GC: Add zend_get_gc_buffer_add_ht() function --- Zend/zend_gc.h | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/Zend/zend_gc.h b/Zend/zend_gc.h index 84519aa68ca96..cd40534000cb4 100644 --- a/Zend/zend_gc.h +++ b/Zend/zend_gc.h @@ -131,6 +131,15 @@ static zend_always_inline void zend_get_gc_buffer_add_zval( } } +static zend_always_inline void zend_get_gc_buffer_add_ht( + zend_get_gc_buffer *gc_buffer, HashTable *ht) { + if (UNEXPECTED(gc_buffer->cur == gc_buffer->end)) { + zend_get_gc_buffer_grow(gc_buffer); + } + ZVAL_ARR(gc_buffer->cur, ht); + gc_buffer->cur++; +} + static zend_always_inline void zend_get_gc_buffer_add_obj( zend_get_gc_buffer *gc_buffer, zend_object *obj) { if (UNEXPECTED(gc_buffer->cur == gc_buffer->end)) { From 6b056e260fb1404a43899de812196e27ea794486 Mon Sep 17 00:00:00 2001 From: George Peter Banyard Date: Sat, 16 Sep 2023 19:30:51 +0100 Subject: [PATCH 2/4] Add GC_DTOR_NOGC() macro function --- Zend/zend_types.h | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/Zend/zend_types.h b/Zend/zend_types.h index c4a07f58874ab..45cd9500cae99 100644 --- a/Zend/zend_types.h +++ b/Zend/zend_types.h @@ -721,6 +721,14 @@ static zend_always_inline uint8_t zval_get_type(const zval* pz) { } \ } while (0) +#define GC_DTOR_NOGC(p) \ + do { \ + zend_refcounted_h *_p = &(p)->gc; \ + if (zend_gc_delref(_p) == 0) { \ + rc_dtor_func((zend_refcounted *)_p); \ + } \ + } while (0) + #define GC_DTOR_NO_REF(p) \ do { \ zend_refcounted_h *_p = &(p)->gc; \ From 7bf91926974c40904d0b31545f3b59b75c291ab1 Mon Sep 17 00:00:00 2001 From: George Peter Banyard Date: Fri, 8 Sep 2023 14:51:22 +0100 Subject: [PATCH 3/4] ext/pdo: Convert def_stmt_ctor_args field from zval to Hashtable pointer --- ext/pdo/pdo_dbh.c | 57 +++++++++---------- ext/pdo/php_pdo_driver.h | 2 +- .../pdo_ATTR_STATEMENT_CLASS_ctor_arg_gc.phpt | 48 ++++++++++++++++ ...o_ATTR_STATEMENT_CLASS_ctor_arg_no-gc.phpt | 37 ++++++++++++ ...ENT_CLASS_ctor_arg_no-gc_var_modified.phpt | 39 +++++++++++++ ...TEMENT_CLASS_ctor_arg_no-gc_var_unset.phpt | 39 +++++++++++++ ...pare_ATTR_STATEMENT_CLASS_ctor_arg_gc.phpt | 52 +++++++++++++++++ ...e_ATTR_STATEMENT_CLASS_ctor_arg_no-gc.phpt | 40 +++++++++++++ ext/pdo/tests/pdo_test.inc | 19 ++++++- 9 files changed, 300 insertions(+), 33 deletions(-) create mode 100644 ext/pdo/tests/pdo_ATTR_STATEMENT_CLASS_ctor_arg_gc.phpt create mode 100644 ext/pdo/tests/pdo_ATTR_STATEMENT_CLASS_ctor_arg_no-gc.phpt create mode 100644 ext/pdo/tests/pdo_ATTR_STATEMENT_CLASS_ctor_arg_no-gc_var_modified.phpt create mode 100644 ext/pdo/tests/pdo_ATTR_STATEMENT_CLASS_ctor_arg_no-gc_var_unset.phpt create mode 100644 ext/pdo/tests/pdo_prepare_ATTR_STATEMENT_CLASS_ctor_arg_gc.phpt create mode 100644 ext/pdo/tests/pdo_prepare_ATTR_STATEMENT_CLASS_ctor_arg_no-gc.phpt diff --git a/ext/pdo/pdo_dbh.c b/ext/pdo/pdo_dbh.c index 2a41153e6d0fc..436e3eece40f7 100644 --- a/ext/pdo/pdo_dbh.c +++ b/ext/pdo/pdo_dbh.c @@ -434,11 +434,9 @@ PHP_METHOD(PDO, __construct) } /* }}} */ -static zval *pdo_stmt_instantiate(pdo_dbh_t *dbh, zval *object, zend_class_entry *dbstmt_ce, zval *ctor_args) /* {{{ */ +static zval *pdo_stmt_instantiate(pdo_dbh_t *dbh, zval *object, zend_class_entry *dbstmt_ce, const HashTable *ctor_args) /* {{{ */ { - if (!Z_ISUNDEF_P(ctor_args)) { - /* This implies an error within PDO if this does not hold */ - ZEND_ASSERT(Z_TYPE_P(ctor_args) == IS_ARRAY); + if (ctor_args) { if (!dbstmt_ce->constructor) { zend_throw_error(NULL, "User-supplied statement does not accept constructor arguments"); return NULL; @@ -475,8 +473,9 @@ PHP_METHOD(PDO, prepare) { pdo_stmt_t *stmt; zend_string *statement; - zval *options = NULL, *value, *item, ctor_args; + zval *options = NULL, *value, *item; zend_class_entry *dbstmt_ce, *pce; + /* const */ HashTable *ctor_args = NULL; pdo_dbh_object_t *dbh_obj = Z_PDO_OBJECT_P(ZEND_THIS); pdo_dbh_t *dbh = dbh_obj->inner; @@ -525,16 +524,14 @@ PHP_METHOD(PDO, prepare) zend_zval_value_name(value)); RETURN_THROWS(); } - ZVAL_COPY_VALUE(&ctor_args, item); - } else { - ZVAL_UNDEF(&ctor_args); + ctor_args = Z_ARRVAL_P(item); } } else { dbstmt_ce = dbh->def_stmt_ce; - ZVAL_COPY_VALUE(&ctor_args, &dbh->def_stmt_ctor_args); + ctor_args = dbh->def_stmt_ctor_args; } - if (!pdo_stmt_instantiate(dbh, return_value, dbstmt_ce, &ctor_args)) { + if (!pdo_stmt_instantiate(dbh, return_value, dbstmt_ce, ctor_args)) { RETURN_THROWS(); } stmt = Z_PDO_STMT_P(return_value); @@ -549,11 +546,7 @@ PHP_METHOD(PDO, prepare) ZVAL_UNDEF(&stmt->lazy_object_ref); if (dbh->methods->preparer(dbh, statement, stmt, options)) { - if (Z_TYPE(ctor_args) == IS_ARRAY) { - pdo_stmt_construct(stmt, return_value, dbstmt_ce, Z_ARRVAL(ctor_args)); - } else { - pdo_stmt_construct(stmt, return_value, dbstmt_ce, /* ctor_args */ NULL); - } + pdo_stmt_construct(stmt, return_value, dbstmt_ce, ctor_args); return; } @@ -817,9 +810,9 @@ static bool pdo_dbh_attribute_set(pdo_dbh_t *dbh, zend_long attr, zval *value) / return false; } dbh->def_stmt_ce = pce; - if (!Z_ISUNDEF(dbh->def_stmt_ctor_args)) { - zval_ptr_dtor(&dbh->def_stmt_ctor_args); - ZVAL_UNDEF(&dbh->def_stmt_ctor_args); + if (dbh->def_stmt_ctor_args) { + zend_array_release(dbh->def_stmt_ctor_args); + dbh->def_stmt_ctor_args = NULL; } if ((item = zend_hash_index_find(Z_ARRVAL_P(value), 1)) != NULL) { if (Z_TYPE_P(item) != IS_ARRAY) { @@ -827,7 +820,9 @@ static bool pdo_dbh_attribute_set(pdo_dbh_t *dbh, zend_long attr, zval *value) / zend_zval_value_name(value)); return false; } - ZVAL_COPY(&dbh->def_stmt_ctor_args, item); + dbh->def_stmt_ctor_args = Z_ARRVAL_P(item); + /* Increase refcount */ + GC_TRY_ADDREF(dbh->def_stmt_ctor_args); } return true; } @@ -906,9 +901,10 @@ PHP_METHOD(PDO, getAttribute) case PDO_ATTR_STATEMENT_CLASS: array_init(return_value); add_next_index_str(return_value, zend_string_copy(dbh->def_stmt_ce->name)); - if (!Z_ISUNDEF(dbh->def_stmt_ctor_args)) { - Z_TRY_ADDREF(dbh->def_stmt_ctor_args); - add_next_index_zval(return_value, &dbh->def_stmt_ctor_args); + if (dbh->def_stmt_ctor_args) { + /* Increment refcount of constructor arguments */ + GC_TRY_ADDREF(dbh->def_stmt_ctor_args); + add_next_index_array(return_value, dbh->def_stmt_ctor_args); } return; case PDO_ATTR_DEFAULT_FETCH_MODE: @@ -1093,7 +1089,7 @@ PHP_METHOD(PDO, query) PDO_DBH_CLEAR_ERR(); - if (!pdo_stmt_instantiate(dbh, return_value, dbh->def_stmt_ce, &dbh->def_stmt_ctor_args)) { + if (!pdo_stmt_instantiate(dbh, return_value, dbh->def_stmt_ce, dbh->def_stmt_ctor_args)) { RETURN_THROWS(); } stmt = Z_PDO_STMT_P(return_value); @@ -1122,11 +1118,7 @@ PHP_METHOD(PDO, query) stmt->executed = 1; } if (ret) { - if (Z_TYPE(dbh->def_stmt_ctor_args) == IS_ARRAY) { - pdo_stmt_construct(stmt, return_value, dbh->def_stmt_ce, Z_ARRVAL(dbh->def_stmt_ctor_args)); - } else { - pdo_stmt_construct(stmt, return_value, dbh->def_stmt_ce, /* ctor_args */ NULL); - } + pdo_stmt_construct(stmt, return_value, dbh->def_stmt_ce, dbh->def_stmt_ctor_args); return; } } @@ -1320,7 +1312,9 @@ static HashTable *dbh_get_gc(zend_object *object, zval **gc_data, int *gc_count) { pdo_dbh_t *dbh = php_pdo_dbh_fetch_inner(object); zend_get_gc_buffer *gc_buffer = zend_get_gc_buffer_create(); - zend_get_gc_buffer_add_zval(gc_buffer, &dbh->def_stmt_ctor_args); + if (dbh->def_stmt_ctor_args) { + zend_get_gc_buffer_add_ht(gc_buffer, dbh->def_stmt_ctor_args); + } if (dbh->methods && dbh->methods->get_gc) { dbh->methods->get_gc(dbh, gc_buffer); } @@ -1382,8 +1376,8 @@ static void dbh_free(pdo_dbh_t *dbh, bool free_persistent) pefree((char *)dbh->persistent_id, dbh->is_persistent); } - if (!Z_ISUNDEF(dbh->def_stmt_ctor_args)) { - zval_ptr_dtor(&dbh->def_stmt_ctor_args); + if (dbh->def_stmt_ctor_args) { + GC_DTOR_NOGC(dbh->def_stmt_ctor_args); } for (i = 0; i < PDO_DBH_DRIVER_METHOD_KIND__MAX; i++) { @@ -1427,6 +1421,7 @@ zend_object *pdo_dbh_new(zend_class_entry *ce) rebuild_object_properties(&dbh->std); dbh->inner = ecalloc(1, sizeof(pdo_dbh_t)); dbh->inner->def_stmt_ce = pdo_dbstmt_ce; + dbh->inner->def_stmt_ctor_args = NULL; return &dbh->std; } diff --git a/ext/pdo/php_pdo_driver.h b/ext/pdo/php_pdo_driver.h index c832284627750..f68fb6c16ce47 100644 --- a/ext/pdo/php_pdo_driver.h +++ b/ext/pdo/php_pdo_driver.h @@ -487,7 +487,7 @@ struct _pdo_dbh_t { zend_class_entry *def_stmt_ce; - zval def_stmt_ctor_args; + HashTable *def_stmt_ctor_args; /* when calling PDO::query(), we need to keep the error * context from the statement around until we next clear it. diff --git a/ext/pdo/tests/pdo_ATTR_STATEMENT_CLASS_ctor_arg_gc.phpt b/ext/pdo/tests/pdo_ATTR_STATEMENT_CLASS_ctor_arg_gc.phpt new file mode 100644 index 0000000000000..c0427fa9e0783 --- /dev/null +++ b/ext/pdo/tests/pdo_ATTR_STATEMENT_CLASS_ctor_arg_gc.phpt @@ -0,0 +1,48 @@ +--TEST-- +PDO: Set PDOStatement class with ctor_args that are freed with GC intervention +--EXTENSIONS-- +pdo +--SKIPIF-- + +--FILE-- +setAttribute(PDO::ATTR_STATEMENT_CLASS, [$this->statementClass, [$this]]); + } +} + +$db = PDOTest::factory(Bar::class); + +$dummy_query = get_dummy_sql_request(); + +$stmt = $db->query($dummy_query); +var_dump($stmt instanceof Foo); +var_dump($stmt->queryString === $dummy_query); + +?> +--EXPECT-- +object(Bar)#1 (1) { + ["statementClass"]=> + string(3) "Foo" +} +bool(true) +bool(true) diff --git a/ext/pdo/tests/pdo_ATTR_STATEMENT_CLASS_ctor_arg_no-gc.phpt b/ext/pdo/tests/pdo_ATTR_STATEMENT_CLASS_ctor_arg_no-gc.phpt new file mode 100644 index 0000000000000..f4adc3cc3ef3b --- /dev/null +++ b/ext/pdo/tests/pdo_ATTR_STATEMENT_CLASS_ctor_arg_no-gc.phpt @@ -0,0 +1,37 @@ +--TEST-- +PDO: Set PDOStatement class with ctor_args that are freed without GC intervention +--EXTENSIONS-- +pdo +--SKIPIF-- + +--FILE-- +setAttribute(PDO::ATTR_STATEMENT_CLASS, array('Foo', ['param1'])); + +$dummy_query = get_dummy_sql_request(); + +$stmt = $db->query($dummy_query); +var_dump($stmt instanceof Foo); +var_dump($stmt->queryString === $dummy_query); + +?> +--EXPECT-- +string(6) "param1" +bool(true) +bool(true) diff --git a/ext/pdo/tests/pdo_ATTR_STATEMENT_CLASS_ctor_arg_no-gc_var_modified.phpt b/ext/pdo/tests/pdo_ATTR_STATEMENT_CLASS_ctor_arg_no-gc_var_modified.phpt new file mode 100644 index 0000000000000..14ac431f6260f --- /dev/null +++ b/ext/pdo/tests/pdo_ATTR_STATEMENT_CLASS_ctor_arg_no-gc_var_modified.phpt @@ -0,0 +1,39 @@ +--TEST-- +PDO: Set PDOStatement class with ctor_args that are freed without GC intervention as a variable that is modified +--EXTENSIONS-- +pdo +--SKIPIF-- + +--FILE-- +setAttribute(PDO::ATTR_STATEMENT_CLASS, $a); +$a[0] = 'Bar'; + +$dummy_query = get_dummy_sql_request(); + +$stmt = $db->query($dummy_query); +var_dump($stmt instanceof Foo); +var_dump($stmt->queryString === $dummy_query); + +?> +--EXPECT-- +string(6) "param1" +bool(true) +bool(true) diff --git a/ext/pdo/tests/pdo_ATTR_STATEMENT_CLASS_ctor_arg_no-gc_var_unset.phpt b/ext/pdo/tests/pdo_ATTR_STATEMENT_CLASS_ctor_arg_no-gc_var_unset.phpt new file mode 100644 index 0000000000000..4c62b1f6d2c3a --- /dev/null +++ b/ext/pdo/tests/pdo_ATTR_STATEMENT_CLASS_ctor_arg_no-gc_var_unset.phpt @@ -0,0 +1,39 @@ +--TEST-- +PDO: Set PDOStatement class with ctor_args that are freed without GC intervention as a variable that is unset +--EXTENSIONS-- +pdo +--SKIPIF-- + +--FILE-- +setAttribute(PDO::ATTR_STATEMENT_CLASS, $a); +unset($a); + +$dummy_query = get_dummy_sql_request(); + +$stmt = $db->query($dummy_query); +var_dump($stmt instanceof Foo); +var_dump($stmt->queryString === $dummy_query); + +?> +--EXPECT-- +string(6) "param1" +bool(true) +bool(true) diff --git a/ext/pdo/tests/pdo_prepare_ATTR_STATEMENT_CLASS_ctor_arg_gc.phpt b/ext/pdo/tests/pdo_prepare_ATTR_STATEMENT_CLASS_ctor_arg_gc.phpt new file mode 100644 index 0000000000000..26843944a6f36 --- /dev/null +++ b/ext/pdo/tests/pdo_prepare_ATTR_STATEMENT_CLASS_ctor_arg_gc.phpt @@ -0,0 +1,52 @@ +--TEST-- +PDO: Set PDOStatement class with ctor_args that are freed with GC intervention in PDO::prepare() +--EXTENSIONS-- +pdo +--SKIPIF-- + +--FILE-- +setAttribute(PDO::ATTR_STATEMENT_CLASS, [$this->statementClass, [$this]]); + } +} + +$db = PDOTest::factory(Bar::class); + + +$dummy_query = get_dummy_sql_request(); + +$stmt = $db->prepare($dummy_query); +var_dump($stmt instanceof Foo); +var_dump($stmt->queryString === $dummy_query); +$r = $stmt->execute(); +var_dump($r); + +?> +--EXPECT-- +object(Bar)#1 (1) { + ["statementClass"]=> + string(3) "Foo" +} +bool(true) +bool(true) +bool(true) diff --git a/ext/pdo/tests/pdo_prepare_ATTR_STATEMENT_CLASS_ctor_arg_no-gc.phpt b/ext/pdo/tests/pdo_prepare_ATTR_STATEMENT_CLASS_ctor_arg_no-gc.phpt new file mode 100644 index 0000000000000..2025ad294c1c1 --- /dev/null +++ b/ext/pdo/tests/pdo_prepare_ATTR_STATEMENT_CLASS_ctor_arg_no-gc.phpt @@ -0,0 +1,40 @@ +--TEST-- +PDO: Set PDOStatement class with ctor_args that are freed without GC intervention in PDO::prepare() +--EXTENSIONS-- +pdo +--SKIPIF-- + +--FILE-- +setAttribute(PDO::ATTR_STATEMENT_CLASS, ['Foo', ['param1']]); + +$dummy_query = get_dummy_sql_request(); + +//$stmt = $db->prepare($dummy_query, [PDO::ATTR_STATEMENT_CLASS => ['Foo', ['param1']] ]); +$stmt = $db->prepare($dummy_query); +var_dump($stmt instanceof Foo); +var_dump($stmt->queryString === $dummy_query); +$r = $stmt->execute(); +var_dump($r); + +?> +--EXPECT-- +string(6) "param1" +bool(true) +bool(true) +bool(true) diff --git a/ext/pdo/tests/pdo_test.inc b/ext/pdo/tests/pdo_test.inc index ba9f159228fc2..093a881e5313c 100644 --- a/ext/pdo/tests/pdo_test.inc +++ b/ext/pdo/tests/pdo_test.inc @@ -83,4 +83,21 @@ class PDOTest { return $config; } } -?> + +/** See https://stackoverflow.com/a/3732466 */ +function get_dummy_sql_request(): string +{ + $dsn = getenv('PDOTEST_DSN'); + + // Firebird: https://www.firebirdfaq.org/faq30/ + if (str_starts_with($dsn, 'firebird')) { + return 'SELECT current_timestamp FROM RDB$DATABASE'; + } + + // Oracle + if (str_starts_with($dsn, 'oci')) { + return 'SELECT 1 FROM DUAL'; + } + + return 'SELECT 1'; +} From c82b47d7cb7c022567ab5f873b35fa608bccadb8 Mon Sep 17 00:00:00 2001 From: George Peter Banyard Date: Sun, 17 Sep 2023 16:00:38 +0100 Subject: [PATCH 4/4] Use existent GC macro --- ext/pdo/pdo_dbh.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ext/pdo/pdo_dbh.c b/ext/pdo/pdo_dbh.c index 436e3eece40f7..0f1cd08f8f0bd 100644 --- a/ext/pdo/pdo_dbh.c +++ b/ext/pdo/pdo_dbh.c @@ -1377,7 +1377,7 @@ static void dbh_free(pdo_dbh_t *dbh, bool free_persistent) } if (dbh->def_stmt_ctor_args) { - GC_DTOR_NOGC(dbh->def_stmt_ctor_args); + GC_DTOR_NO_REF(dbh->def_stmt_ctor_args); } for (i = 0; i < PDO_DBH_DRIVER_METHOD_KIND__MAX; i++) {