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.

Author: rposky

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;
 	}
 }

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 {}
 

ext/pdo/pdo_dbh_arginfo.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(); \

ext/pdo/php_pdo.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 */