From 28afd5248d404f4d71a1f82341e6eb2a34210483 Mon Sep 17 00:00:00 2001 From: Robert Date: Mon, 21 Jul 2025 07:00:28 -0400 Subject: [PATCH] PDO - Added PHP::disconnect and PHP::isConnected and refactored free object. Added disconnect for __destruct alternative to disconnect from db, related to bug #40681 and requests #62065 and #67473. Added PHP::isConnected to yield current connection status. Fixed bug #63343 in rework of PDO pdo_dbh_free_storage logic to avoid transaction rollback against persistent db connection. Added pdo_is_persisted and pdo_list_entry_from_key helpers to determine when persistent db handler is actively registered as resource, avoiding opaque use of _pdo_dbh_t::refcount to this end. Updated php_pdo_internal_construct_driver to consume added list entry helper and to check _pdo_dbh_t::is_closed in addition to liveness when validating a discovered persistent db connection for re-use. Removed extraneous call to zend_list_close given that only a persistent destructor is defined for the _pdo_dbh_t resource that zend_list_close does not invoke. Fixed potential for bad memory access in persistent connections from former PDO objects, once the connection has been replaced due to liveness check failure. The resource was freed without invalidating other PDO object references, but now the resource will not be freed until the last referring PDO object is freed. Made _pdo_dbh_t::refcount accurate and consistent, regardless of persistent connection, to represent count of PDO object's referencing the handle. In the case of persistent connections, the count was +1, and in the case of non-persistent connections, never decremented. --- ext/pdo/pdo_dbh.c | 173 +++++++++++++------ ext/pdo/pdo_dbh.stub.php | 4 + ext/pdo/pdo_dbh_arginfo.h | 11 +- ext/pdo/php_pdo.h | 4 + ext/pdo/php_pdo_driver.h | 2 + ext/pdo/tests/bug_63343.phpt | 47 +++++ ext/pdo/tests/pdo_040.phpt | 89 ++++++++++ ext/pdo_mysql/tests/pdo_mysql_interface.phpt | 2 + 8 files changed, 278 insertions(+), 54 deletions(-) create mode 100644 ext/pdo/tests/bug_63343.phpt create mode 100644 ext/pdo/tests/pdo_040.phpt diff --git a/ext/pdo/pdo_dbh.c b/ext/pdo/pdo_dbh.c index 4ac271fefd4bf..b51777274ca35 100644 --- a/ext/pdo/pdo_dbh.c +++ b/ext/pdo/pdo_dbh.c @@ -149,7 +149,7 @@ PDO_API void pdo_handle_error(pdo_dbh_t *dbh, pdo_stmt_t *stmt) /* {{{ */ } ZVAL_UNDEF(&info); - if (dbh->methods->fetch_err) { + if (dbh->methods && dbh->methods->fetch_err) { zval *item; array_init(&info); @@ -221,6 +221,21 @@ static char *dsn_from_uri(char *uri, char *buf, size_t buflen) /* {{{ */ } /* }}} */ +/* Fetch the registered persistent PDO object for the given key */ +static pdo_dbh_t *pdo_list_entry_from_key(const char *hashkey, size_t len) +{ + pdo_dbh_t *pdbh = NULL; + zend_resource *le; + + if ((le = zend_hash_str_find_ptr(&EG(persistent_list), hashkey, len)) != NULL) { + if (le->type == php_pdo_list_entry()) { + pdbh = (pdo_dbh_t*)le->ptr; + } + } + + return pdbh; +} + /* {{{ */ PHP_METHOD(PDO, __construct) { @@ -297,7 +312,6 @@ PHP_METHOD(PDO, __construct) if (options) { int plen = 0; char *hashkey = NULL; - zend_resource *le; pdo_dbh_t *pdbh = NULL; zval *v; @@ -320,36 +334,22 @@ PHP_METHOD(PDO, __construct) if (is_persistent) { /* let's see if we have one cached.... */ - if ((le = zend_hash_str_find_ptr(&EG(persistent_list), hashkey, plen)) != NULL) { - if (le->type == php_pdo_list_entry()) { - pdbh = (pdo_dbh_t*)le->ptr; - - /* is the connection still alive ? */ - if (pdbh->methods->check_liveness && FAILURE == (pdbh->methods->check_liveness)(pdbh)) { - /* nope... need to kill it */ - pdbh->refcount--; - zend_list_close(le); - pdbh = NULL; - } - } - } - - if (pdbh) { - call_factory = 0; - } else { + pdbh = pdo_list_entry_from_key(hashkey, plen); + /* is the connection still alive ? */ + if (!pdbh || pdbh->is_closed || + (pdbh->methods->check_liveness && FAILURE == (pdbh->methods->check_liveness)(pdbh))) { /* need a brand new pdbh */ pdbh = pecalloc(1, sizeof(*pdbh), 1); - pdbh->refcount = 1; pdbh->is_persistent = 1; pdbh->persistent_id = pemalloc(plen + 1, 1); memcpy((char *)pdbh->persistent_id, hashkey, plen+1); pdbh->persistent_id_len = plen; pdbh->def_stmt_ce = dbh->def_stmt_ce; + } else { + /* found viable dbh persisted */ + call_factory = 0; } - } - - if (pdbh) { efree(dbh); /* switch over to the persistent one */ Z_PDO_OBJECT_P(object)->inner = pdbh; @@ -393,6 +393,8 @@ PHP_METHOD(PDO, __construct) /* register in the persistent list etc. */ /* we should also need to replace the object store entry, since it was created with emalloc */ + /* if a resource is already registered, then it failed a liveness check + * and will be replaced, prompting destruct. */ if ((zend_register_persistent_resource( (char*)dbh->persistent_id, dbh->persistent_id_len, dbh, php_pdo_list_entry())) == NULL) { php_error_docref(NULL, E_ERROR, "Failed to register persistent entry"); @@ -422,9 +424,6 @@ PHP_METHOD(PDO, __construct) } /* the connection failed; things will tidy up in free_storage */ - if (is_persistent) { - dbh->refcount--; - } /* XXX raise exception */ zend_restore_error_handling(&zeh); @@ -587,6 +586,9 @@ PHP_METHOD(PDO, prepare) static bool pdo_is_in_transaction(pdo_dbh_t *dbh) { + if (dbh->is_closed) { + return false; + } if (dbh->methods->in_transaction) { return dbh->methods->in_transaction(dbh); } @@ -684,6 +686,17 @@ PHP_METHOD(PDO, inTransaction) } /* }}} */ +/* {{{ Determine if connected */ +PHP_METHOD(PDO, isConnected) +{ + pdo_dbh_t *dbh = Z_PDO_DBH_P(ZEND_THIS); + + ZEND_PARSE_PARAMETERS_NONE(); + + RETURN_BOOL(!dbh->is_closed); +} +/* }}} */ + PDO_API bool pdo_get_long_param(zend_long *lval, zval *value) { switch (Z_TYPE_P(value)) { @@ -1027,8 +1040,6 @@ PHP_METHOD(PDO, errorCode) ZEND_PARSE_PARAMETERS_NONE(); - PDO_CONSTRUCT_CHECK; - if (dbh->query_stmt) { RETURN_STRING(dbh->query_stmt->error_code); } @@ -1056,8 +1067,6 @@ PHP_METHOD(PDO, errorInfo) ZEND_PARSE_PARAMETERS_NONE(); - PDO_CONSTRUCT_CHECK; - array_init(return_value); if (dbh->query_stmt) { @@ -1068,7 +1077,8 @@ PHP_METHOD(PDO, errorInfo) if(!strncmp(dbh->error_code, PDO_ERR_NONE, sizeof(PDO_ERR_NONE))) goto fill_array; } - if (dbh->methods->fetch_err) { + /* Driver-implemented error is not available once database is shutdown. */ + if (dbh->methods && dbh->methods->fetch_err) { dbh->methods->fetch_err(dbh, dbh->query_stmt, return_value); } @@ -1366,23 +1376,53 @@ void pdo_dbh_init(int module_number) pdo_dbh_object_handlers.get_gc = dbh_get_gc; } -static void dbh_free(pdo_dbh_t *dbh, bool free_persistent) +/* Disconnect from the database and free associated driver. */ +static void dbh_shutdown(pdo_dbh_t *dbh) { - int i; + if (dbh->driver_data && dbh->methods && dbh->methods->rollback && pdo_is_in_transaction(dbh)) { + dbh->methods->rollback(dbh); + dbh->in_txn = false; + } - if (dbh->query_stmt) { - zval_ptr_dtor(&dbh->query_stmt_zval); - dbh->query_stmt = NULL; + if (dbh->methods) { + dbh->methods->closer(dbh); } - if (dbh->is_persistent) { + /* Do not permit reference to driver methods to remain past closer(), which + * is responsible for both disconnecting the db and free-ing allocations. + * Ideally, this would only disconnect the database, not free the handle. */ + dbh->methods = NULL; + dbh->is_closed = true; +} + +/* {{{ Disconnect from the database. */ +PHP_METHOD(PDO, disconnect) +{ + pdo_dbh_t *dbh = Z_PDO_DBH_P(ZEND_THIS); + + ZEND_PARSE_PARAMETERS_NONE(); + + PDO_DBH_CLEAR_ERR(); + PDO_CONSTRUCT_CHECK; + + dbh_shutdown(dbh); + + PDO_HANDLE_DBH_ERR(); + + RETURN_TRUE; +} +/* }}} */ + +/* Free the database when the last pdo object referencing it is freed + * or when it has been registered as a php resource, i.e. is persistent, + * and the resource is destructed, whichever comes last. */ +static void dbh_free(pdo_dbh_t *dbh) +{ + int i; + #if ZEND_DEBUG - ZEND_ASSERT(!free_persistent || (dbh->refcount == 1)); + ZEND_ASSERT(dbh->refcount == 0); #endif - if (!free_persistent && (--dbh->refcount)) { - return; - } - } if (dbh->methods) { dbh->methods->closer(dbh); @@ -1416,25 +1456,48 @@ static void dbh_free(pdo_dbh_t *dbh, bool free_persistent) pefree(dbh, dbh->is_persistent); } +/* Whether the given database handler is presently registered as a resource. */ +static bool pdo_is_persisted(pdo_dbh_t *dbh) +{ + pdo_dbh_t *pdbh = NULL; + + if (dbh->persistent_id != NULL) { + pdbh = pdo_list_entry_from_key(dbh->persistent_id, dbh->persistent_id_len); + return dbh == pdbh; + } + + return false; +} + static void pdo_dbh_free_storage(zend_object *std) { pdo_dbh_t *dbh = php_pdo_dbh_fetch_inner(std); /* dbh might be null if we OOMed during object initialization. */ - if (!dbh) { - return; - } + if (dbh) { + if (dbh->is_persistent && dbh->methods && dbh->methods->persistent_shutdown) { + dbh->methods->persistent_shutdown(dbh); + } - if (dbh->driver_data && dbh->methods && dbh->methods->rollback && pdo_is_in_transaction(dbh)) { - dbh->methods->rollback(dbh); - dbh->in_txn = false; - } + /* stmt is not persistent, even if dbh is, so it must be freed with pdo. + * Consider copying stmt error code to dbh at this point, seemingly the reason + * that the stmt is even being held, or even better, to do that at the time of + * error and remove the reference altogether. */ + if (dbh->query_stmt) { + zval_ptr_dtor(&dbh->query_stmt_zval); + dbh->query_stmt = NULL; + } - if (dbh->is_persistent && dbh->methods && dbh->methods->persistent_shutdown) { - dbh->methods->persistent_shutdown(dbh); + /* a persisted dbh will be freed when the resource is destructed. */ + if (!(--dbh->refcount) && !pdo_is_persisted(dbh)) { + if (!dbh->is_closed) { + dbh_shutdown(dbh); + } + dbh_free(dbh); + } } + zend_object_std_dtor(std); - dbh_free(dbh, 0); } zend_object *pdo_dbh_new(zend_class_entry *ce) @@ -1447,6 +1510,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->refcount++; return &dbh->std; } @@ -1457,7 +1521,10 @@ ZEND_RSRC_DTOR_FUNC(php_pdo_pdbh_dtor) /* {{{ */ { if (res->ptr) { pdo_dbh_t *dbh = (pdo_dbh_t*)res->ptr; - dbh_free(dbh, 1); + if (!dbh->refcount) { + /* do not free if still referenced by pdo */ + dbh_free(dbh); + } res->ptr = NULL; } } diff --git a/ext/pdo/pdo_dbh.stub.php b/ext/pdo/pdo_dbh.stub.php index 45364147230f4..6dca99e5dea71 100644 --- a/ext/pdo/pdo_dbh.stub.php +++ b/ext/pdo/pdo_dbh.stub.php @@ -394,6 +394,8 @@ public function beginTransaction(): bool {} /** @tentative-return-type */ public function commit(): bool {} + public function disconnect(): bool {} + /** @tentative-return-type */ public function errorCode(): ?string {} @@ -412,6 +414,8 @@ public static function getAvailableDrivers(): array {} /** @tentative-return-type */ public function inTransaction(): bool {} + public function isConnected(): bool {} + /** @tentative-return-type */ public function lastInsertId(?string $name = null): string|false {} diff --git a/ext/pdo/pdo_dbh_arginfo.h b/ext/pdo/pdo_dbh_arginfo.h index afbbc935548e5..2de317af0eed9 100644 --- a/ext/pdo/pdo_dbh_arginfo.h +++ b/ext/pdo/pdo_dbh_arginfo.h @@ -1,5 +1,5 @@ /* This is a generated file, edit the .stub.php file instead. - * Stub hash: 7dcba884671fd90b891fab7e3f0d4cc9a4ac76a1 */ + * Stub hash: ba3a517f23ba76d450e933b2fdd33b30e73f8707 */ ZEND_BEGIN_ARG_INFO_EX(arginfo_class_PDO___construct, 0, 0, 1) ZEND_ARG_TYPE_INFO(0, dsn, IS_STRING, 0) @@ -13,6 +13,9 @@ ZEND_END_ARG_INFO() #define arginfo_class_PDO_commit arginfo_class_PDO_beginTransaction +ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_class_PDO_disconnect, 0, 0, _IS_BOOL, 0) +ZEND_END_ARG_INFO() + ZEND_BEGIN_ARG_WITH_TENTATIVE_RETURN_TYPE_INFO_EX(arginfo_class_PDO_errorCode, 0, 0, IS_STRING, 1) ZEND_END_ARG_INFO() @@ -31,6 +34,8 @@ ZEND_END_ARG_INFO() #define arginfo_class_PDO_inTransaction arginfo_class_PDO_beginTransaction +#define arginfo_class_PDO_isConnected arginfo_class_PDO_disconnect + ZEND_BEGIN_ARG_WITH_TENTATIVE_RETURN_TYPE_MASK_EX(arginfo_class_PDO_lastInsertId, 0, 0, MAY_BE_STRING|MAY_BE_FALSE) ZEND_ARG_TYPE_INFO_WITH_DEFAULT_VALUE(0, name, IS_STRING, 1, "null") ZEND_END_ARG_INFO() @@ -62,12 +67,14 @@ ZEND_END_ARG_INFO() ZEND_METHOD(PDO, __construct); ZEND_METHOD(PDO, beginTransaction); ZEND_METHOD(PDO, commit); +ZEND_METHOD(PDO, disconnect); ZEND_METHOD(PDO, errorCode); ZEND_METHOD(PDO, errorInfo); ZEND_METHOD(PDO, exec); ZEND_METHOD(PDO, getAttribute); ZEND_METHOD(PDO, getAvailableDrivers); ZEND_METHOD(PDO, inTransaction); +ZEND_METHOD(PDO, isConnected); ZEND_METHOD(PDO, lastInsertId); ZEND_METHOD(PDO, prepare); ZEND_METHOD(PDO, query); @@ -80,12 +87,14 @@ static const zend_function_entry class_PDO_methods[] = { ZEND_ME(PDO, __construct, arginfo_class_PDO___construct, ZEND_ACC_PUBLIC) ZEND_ME(PDO, beginTransaction, arginfo_class_PDO_beginTransaction, ZEND_ACC_PUBLIC) ZEND_ME(PDO, commit, arginfo_class_PDO_commit, ZEND_ACC_PUBLIC) + ZEND_ME(PDO, disconnect, arginfo_class_PDO_disconnect, ZEND_ACC_PUBLIC) ZEND_ME(PDO, errorCode, arginfo_class_PDO_errorCode, ZEND_ACC_PUBLIC) ZEND_ME(PDO, errorInfo, arginfo_class_PDO_errorInfo, ZEND_ACC_PUBLIC) ZEND_ME(PDO, exec, arginfo_class_PDO_exec, ZEND_ACC_PUBLIC) ZEND_ME(PDO, getAttribute, arginfo_class_PDO_getAttribute, ZEND_ACC_PUBLIC) ZEND_ME(PDO, getAvailableDrivers, arginfo_class_PDO_getAvailableDrivers, ZEND_ACC_PUBLIC|ZEND_ACC_STATIC) ZEND_ME(PDO, inTransaction, arginfo_class_PDO_inTransaction, ZEND_ACC_PUBLIC) + ZEND_ME(PDO, isConnected, arginfo_class_PDO_isConnected, ZEND_ACC_PUBLIC) ZEND_ME(PDO, lastInsertId, arginfo_class_PDO_lastInsertId, ZEND_ACC_PUBLIC) ZEND_ME(PDO, prepare, arginfo_class_PDO_prepare, ZEND_ACC_PUBLIC) ZEND_ME(PDO, query, arginfo_class_PDO_query, ZEND_ACC_PUBLIC) diff --git a/ext/pdo/php_pdo.h b/ext/pdo/php_pdo.h index 023372ebc8e6e..6567ad785a608 100644 --- a/ext/pdo/php_pdo.h +++ b/ext/pdo/php_pdo.h @@ -56,6 +56,10 @@ PHP_MINFO_FUNCTION(pdo); #define LONG_CONST(c) (zend_long) c #define PDO_CONSTRUCT_CHECK \ + if (dbh->is_closed) { \ + zend_throw_exception_ex(php_pdo_get_exception(), 0, "connection is closed"); \ + RETURN_THROWS(); \ + } \ if (!dbh->driver) { \ zend_throw_error(NULL, "PDO object is not initialized, constructor was not called"); \ RETURN_THROWS(); \ diff --git a/ext/pdo/php_pdo_driver.h b/ext/pdo/php_pdo_driver.h index c832284627750..8fe357343d420 100644 --- a/ext/pdo/php_pdo_driver.h +++ b/ext/pdo/php_pdo_driver.h @@ -478,6 +478,8 @@ struct _pdo_dbh_t { /* persistent hash key associated with this handle */ const char *persistent_id; size_t persistent_id_len; + + /* counter of _pdo_dbh_object_t referencing this handle */ unsigned int refcount; /* driver specific "class" methods for the dbh and stmt */ diff --git a/ext/pdo/tests/bug_63343.phpt b/ext/pdo/tests/bug_63343.phpt new file mode 100644 index 0000000000000..9775c4d000715 --- /dev/null +++ b/ext/pdo/tests/bug_63343.phpt @@ -0,0 +1,47 @@ +--TEST-- +PDO Common: Bug #63343 (Commit failure for repeated persistent connection) +--EXTENSIONS-- +pdo +--SKIPIF-- +beginTransaction(); +} catch (PDOException $exception) { + die("skip not relevant for driver - does not permit transactions"); +} +?> +--FILE-- + true))); + +$db1 = PDOTest::factory('PDO', false); +$db1->beginTransaction(); +var_dump($db1->inTransaction()); +/* db2 should assume persistent conn, including txn state */ +$db2 = PDOTest::factory('PDO', false); +var_dump($db2->inTransaction()); +/* destructing db1 should not rollback db2 */ +$db1 = null; +var_dump($db2->inTransaction()); +$db2 = null; +/* db3 should assume persistent conn, despite no remaining PDOs */ +$db3 = PDOTest::factory('PDO', false); +var_dump($db3->inTransaction()); +var_dump($db3->commit()); + +?> +--EXPECTF-- +bool(true) +bool(true) +bool(true) +bool(true) +bool(true) diff --git a/ext/pdo/tests/pdo_040.phpt b/ext/pdo/tests/pdo_040.phpt new file mode 100644 index 0000000000000..3418d4fb42d48 --- /dev/null +++ b/ext/pdo/tests/pdo_040.phpt @@ -0,0 +1,89 @@ +--TEST-- +PDO Common: Explicit disconnect of common and persistent PDO +--EXTENSIONS-- +pdo +--SKIPIF-- +beginTransaction(); +} catch (PDOException $exception) { + die("skip not relevant for driver - does not permit transactions"); +} +?> +--FILE-- +isConnected()); +$db2 = PDOTest::factory('PDO', false); +var_dump($db2->isConnected()); +putenv("PDOTEST_ATTR=" . serialize(array(PDO::ATTR_PERSISTENT => true))); +$pdb3 = PDOTest::factory('PDO', false); +var_dump($pdb3->isConnected()); +$pdb4 = PDOTest::factory('PDO', false); +var_dump($pdb4->isConnected()); + +/* disconnect regular PDO, confirm other PDOs still connected */ +var_dump($db1->disconnect()); +var_dump($db1->isConnected()); +try { + $db1->beginTransaction(); +} catch (PDOException $e) { + echo $e->getMessage(), \PHP_EOL; +} +var_dump($db2->isConnected()); +var_dump($pdb3->isConnected()); + +/* disconnect persistent PDO, confirm other persistent PDO disconnected */ +var_dump($pdb3->disconnect()); +var_dump($pdb3->isConnected()); +var_dump($pdb4->isConnected()); +try { + $pdb4->disconnect(); +} catch (PDOException $e) { + echo $e->getMessage(), \PHP_EOL; +} + +/* new persistent PDO should prompt new connection */ +$pdb5 = PDOTest::factory('PDO', false); +var_dump($pdb5->isConnected()); +var_dump($pdb5->inTransaction()); +var_dump($pdb5->beginTransaction()); + +$db1 = null; +$db2 = null; /* trigger shutdown without explicit disconnect */ +$pdb3 = null; +$pdb4 = null; +$pdb5 = null; /* destruct should not disconnect */ + +/* no PDOs remain, but persistent connection should remain */ +$pdb6 = PDOTest::factory('PDO', false); +var_dump($pdb6->inTransaction()); + +?> +--EXPECTF-- +bool(true) +bool(true) +bool(true) +bool(true) +bool(true) +bool(false) +connection is closed +bool(true) +bool(true) +bool(true) +bool(false) +bool(false) +connection is closed +bool(true) +bool(false) +bool(true) +bool(true) diff --git a/ext/pdo_mysql/tests/pdo_mysql_interface.phpt b/ext/pdo_mysql/tests/pdo_mysql_interface.phpt index aeb441bc0363a..679337c90b401 100644 --- a/ext/pdo_mysql/tests/pdo_mysql_interface.phpt +++ b/ext/pdo_mysql/tests/pdo_mysql_interface.phpt @@ -30,6 +30,8 @@ if (false == MySQLPDOTest::detect_transactional_mysql_engine($db)) 'getAttribute' => true, 'quote' => true, 'inTransaction' => true, + 'disconnect' => true, + 'isConnected' => true, 'getAvailableDrivers' => true, ); $classname = get_class($db);