It is possible to mix PDO sub-classes with different dsn and crash PHP

[evaikene]

Description

PHP 8.4 added PDO driver specific sub-classes and it is now possible to write the following code, which crashes the PHP process:

$db = new Pdo\Mysql('sqlite:test.db');
$db->getWarningCount();
<?php

* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x94048479aa1403e0)
  * frame #0: 0x00000479aa1403e0
    frame #1: 0x00000001002e2694 php`zim_Pdo_Mysql_getWarningCount(execute_data=0x0000000101215140, return_value=0x0000000101215120) at pdo_mysql.c:99:2
    frame #2: 0x00000001007a926c php`ZEND_DO_FCALL_SPEC_RETVAL_USED_HANDLER(execute_data=0x0000000101215020) at zend_vm_execute.h:2025:4
    frame #3: 0x000000010075c248 php`execute_ex(ex=0x0000000101215020) at zend_vm_execute.h:58486:7
...

The dsn requested a sqlite driver, but the PDO sub-class is Pdo\Mysql, which assumes that it has a mysql driver and can use it causing a crash.

pdo_mysql.c line 99:

	H = (pdo_mysql_db_handle *)dbh->driver_data;
	RETURN_LONG(mysql_warning_count(H->server));

Expected result

Either an exception throw when the PDO sub-class and dsn don't match or not possible to create sub-class objects directly (only with PDO::connect).

PHP Version

PHP 8.4.0RC1

Operating System

No response

[TimWolla]

I had the same idea of testing that just recently. There is some code that is intended to verify this:

php-src/ext/pdo/pdo_dbh.c

Lines 222 to 276 in ebee8df

	static bool create_driver_specific_pdo_object(pdo_driver_t *driver, zend_class_entry *called_scope, zval *new_object)
	{
	zend_class_entry *ce;
	zend_class_entry *ce_based_on_driver_name = NULL, *ce_based_on_called_object = NULL;
	ce_based_on_driver_name = zend_hash_str_find_ptr(&pdo_driver_specific_ce_hash, driver->driver_name, driver->driver_name_len);
	ZEND_HASH_MAP_FOREACH_PTR(&pdo_driver_specific_ce_hash, ce) {
	if (called_scope != pdo_dbh_ce && instanceof_function(called_scope, ce)) {
	ce_based_on_called_object = called_scope;
	break;
	}
	} ZEND_HASH_FOREACH_END();
	if (ce_based_on_called_object) {
	if (ce_based_on_driver_name) {
	if (instanceof_function(ce_based_on_called_object, ce_based_on_driver_name) == false) {
	zend_throw_exception_ex(pdo_exception_ce, 0,
	"%s::connect() cannot be called when connecting to the \"%s\" driver, "
	"either %s::connect() or PDO::connect() must be called instead",
	ZSTR_VAL(called_scope->name), driver->driver_name, ZSTR_VAL(ce_based_on_driver_name->name));
	return false;
	}
	/* A driver-specific implementation was instantiated via the connect() method of the appropriate driver class */
	object_init_ex(new_object, ce_based_on_called_object);
	return true;
	} else {
	zend_throw_exception_ex(pdo_exception_ce, 0,
	"%s::connect() cannot be called when connecting to an unknown driver, "
	"PDO::connect() must be called instead",
	ZSTR_VAL(called_scope->name));
	return false;
	}
	}
	if (ce_based_on_driver_name) {
	if (called_scope != pdo_dbh_ce) {
	/* A driver-specific implementation was instantiated via the connect method of a wrong driver class */
	zend_throw_exception_ex(pdo_exception_ce, 0,
	"%s::connect() cannot be called when connecting to the \"%s\" driver, "
	"either %s::connect() or PDO::connect() must be called instead",
	ZSTR_VAL(called_scope->name), driver->driver_name, ZSTR_VAL(ce_based_on_driver_name->name));
	return false;
	}
	/* A driver-specific implementation was instantiated via PDO::__construct() */
	object_init_ex(new_object, ce_based_on_driver_name);
	} else {
	/* No driver-specific implementation found */
	object_init_ex(new_object, called_scope);
	}
	return true;
	}

[TimWolla]

Ah, it's checked when calling ::connect() on subclasses, but not when calling a subclass constructor:

<?php

$db = Pdo\Mysql::connect('sqlite:test.db');
$db->getWarningCount();

correctly results in:

Fatal error: Uncaught PDOException: Pdo\Mysql::connect() cannot be called when connecting to the "sqlite" driver, either Pdo\Sqlite::connect() or PDO::connect() must be called instead in php-src/test2.php:3
Stack trace:
#0 php-src/test2.php(3): PDO::connect('sqlite:test.db')
#1 {main}
  thrown in php-src/test2.php on line 3

[kocsismate]

sorry for the bug, I'll take a stab at it tonight

[NattyNarwhal]

I did take a stab at it as well (in calvin/fix-gh-16131-php8.4), trying to reuse the existing logic in the ::connect case as much as possible. Not filing a PR yet because it does seem to break user subclassing of it with the naive fix I tried. I suspect you probably can figure out a better fix faster (and a test, since hardcoding drivers in tests seems gross, yet you need two drivers).

(As a side note, it seems the current class entry fetch in ctor/connect seems... wrong? At least when I was poking at it in a debugger. The zend_class_entry it pulls out seems bogus (i.e. tons of fields look like parts of strings), but the CE done via ->ce on the zend_object we're already fetching seems valid. I might be missing some subtlety w/ objects and class entries here though.) Ignore this, I'm missing some subtleties in execute_data I need to look into more.

[kocsismate]

@NattyNarwhal thank you for attempting to fix the issue! My changes are very similar to yours, but a small trick is needed to really make it work for userland subclasses. I will likely have time to create a PR tonight or tomorrow morning.

[kocsismate]

I filed #16167 that is supposed to fix the problem. I didn't yet have time for tidying it up, so I'm leaving it in draft for now.

The "small trick" I mentioned was to add the explicit constructor for each implementation. Doing so made it possible not to break subclasses of internal PDO classes.