Fix GH-16131: Prevent mixing PDO sub-classes with different DSN #16167

kocsismate · 2024-10-02T05:54:07Z

No description provided.

TimWolla · 2024-10-02T07:17:23Z

Should this PR target PHP-8.4?

kocsismate · 2024-10-02T07:21:41Z

Should this PR target PHP-8.4?

Yes, of course, I just always forget to set the target on GitHub :) Thanks for the reminder though!

cmb69 · 2024-10-02T10:32:38Z

There appears to be no ABI break.

NattyNarwhal

It's a little annoying it has to be in each driver subclass, but my internals-fu is weak enough I can't offer any alternatives.

ext/pdo/php_pdo_driver.h

kocsismate · 2024-10-02T20:22:20Z

It's a little annoying it has to be in each driver subclass, but my internals-fu is weak enough I can't offer any alternatives.

Yeah, agreed... Neither I have any better idea which would work... But maybe Ilija will have one if he gets a chance to review the PR.

NattyNarwhal · 2024-10-09T18:43:41Z

FWIW, I'm also realizing this approach is going to be annoying for third-party PDO drivers; it'll involve a last-minute addition to UPGRADING.INTERNALS and some ifdef in the stub and a short stub function for the class implementation.

kocsismate · 2024-10-10T08:32:38Z

FWIW, I'm also realizing this approach is going to be annoying for third-party PDO drivers; it'll involve a last-minute addition to UPGRADING.INTERNALS and some ifdef in the stub and a short stub function for the class implementation.

Yes, agreed... @iluuu1994 if you have capacity... do you by chance have any better approach to fix the issue than what I found?

iluuu1994 · 2024-10-10T14:46:25Z

@kocsismate I can have a look, but please know that I have 0 experience with pdo, both as a user and developer. Maybe there's somebody more qualified, @SakiTakamachi?

kocsismate · 2024-10-11T10:36:48Z

I can have a look, but please know that I have 0 experience with pdo, both as a user and developer

@iluuu1994 I think this problem is more about general OOP behavior than about PDO specific stuff: when instantiating a PDO children, we have to check whether the DSN is appropriate for the called internal PDO subclass (i.e. Pdo\Mysql, Pdo\Pgsql etc.). The problem is that these subclasses can also have children in userland, so we cannot simply use the currently called object, but we have to retrieve the internal driver-specific PDO subclass which is extended. That's why I added a separate constructor for each internal PDO subclass.

cmb69 · 2024-10-11T11:18:02Z

The problem is that these subclasses can also have children in userland, so we cannot simply use the currently called object, but we have to retrieve the name first PDO subclass.

Can't we "just" look up the first internal class in the ancestor chain? Might not work if the class is not linked, though.

nielsdos · 2024-10-17T21:20:35Z

Can't we "just" look up the first internal class in the ancestor chain? Might not work if the class is not linked, though.

In what scenario would it not be linked?
e.g. DOM walks the ancestor chain when dealing with overriden child classes.

cmb69 · 2024-10-17T21:42:42Z

In what scenario would it not be linked?
e.g. DOM walks the ancestor chain when dealing with overriden child classes.

Yeah, you're right. I had just looked up zend_class_entry, and didn't think about runtime (where linking should have been happened).

kocsismate · 2024-10-18T06:49:07Z

I'm going to rewrite my solution with the one suggested by Christoph ASAP

kocsismate

It turned out that neither adding a dedicated constructor for each driver-specific class, nor iterating over the class hierarchy is needed to fix the issue.

kocsismate · 2024-10-20T20:52:06Z

ext/pdo/pdo_dbh.c

 }
 /* }}} */

 /* {{{ */
 PHP_METHOD(PDO, connect)
 {
-	internal_construct(INTERNAL_FUNCTION_PARAM_PASSTHRU, Z_OBJ(EX(This)), EX(This).value.ce, return_value);
+	php_pdo_internal_construct_driver(INTERNAL_FUNCTION_PARAM_PASSTHRU, NULL, Z_CE_P(ZEND_THIS), return_value);


EX(This).value.ce is Z_OBJCE_P(ZEND_THIS) which is different than Z_CE_P(ZEND_THIS). For a static method, the latter is needed to properly support the static return type

kocsismate · 2024-10-20T20:52:29Z

ext/pdo/pdo_dbh.c

-		}
+	ZEND_ASSERT((driver->driver_name != NULL) && "PDO driver name is null");
+
+	if (!create_driver_specific_pdo_object(driver, called_scope, new_zval_object)) {


create_driver_specific_pdo_object() has to be called for the constructor as well. Previously, only connect() calls eere validated...

kocsismate · 2024-10-20T20:56:01Z

ext/pdo/pdo_dbh.c

 			return false;
 		}
 	}

+	/* A non-driver specific PDO subclass is instantiated via the constructor. This results in the legacy behavior. */


Without this, older tests using legacy "quasi classes" would fail, now that constructor calls are also validated.

kocsismate · 2024-10-20T20:59:31Z

ext/pdo_sqlite/tests/subclasses/pdosqlite_005.phpt

@@ -14,4 +14,4 @@ try {

 ?>
 --EXPECT--
-Pdo\Pgsql::connect() cannot be called when connecting to the "sqlite" driver, either Pdo\Sqlite::connect() or PDO::connect() must be called instead
+Pdo\Pgsql::connect() cannot be used for connecting to the "sqlite" driver, either call Pdo\Sqlite::connect() or PDO::connect() instead


The new wording is more clear IMO.

kocsismate · 2024-10-20T21:01:19Z

ext/pdo/php_pdo_driver.h

@@ -694,6 +694,8 @@ PDO_API void php_pdo_dbh_delref(pdo_dbh_t *dbh);
 PDO_API void php_pdo_free_statement(pdo_stmt_t *stmt);
 PDO_API void php_pdo_stmt_set_column_count(pdo_stmt_t *stmt, int new_count);

+PDO_API void php_pdo_internal_construct_driver(INTERNAL_FUNCTION_PARAMETERS, zend_object *current_object, zend_class_entry *called_scope, zval *new_zval_object);


I guess it makes sense to expose this function for extensions which override the PDO constructor (?)

NattyNarwhal

This seems to be a much better solution that minimize impact elsewhere. The only weird part is the stack trace does mention the PDO base class, even if I use a driver subclass instead. i.e.

Fatal error: Uncaught PDOException: Pdo\Mysql::__construct() cannot be used for connecting to the "sqlite" driver, either call Pdo\Sqlite::__construct() or PDO::__construct() instead in /Users/calvin/src/evil.php:2
Stack trace:
#0 /Users/calvin/src/evil.php(2): PDO->__construct('sqlite:test.db')
#1 {main}
  thrown in /Users/calvin/src/evil.php on line 2

(where line 2 is $db = new Pdo\Mysql('sqlite:test.db');)

cmb69 · 2024-10-22T13:55:47Z

The only weird part is the stack trace does mention the PDO base class, even if I use a driver subclass instead.

I think we need to fix that somehow (thinking about debug_backtrace() and friends, too).

kocsismate · 2024-10-22T20:39:08Z

The only weird part is the stack trace does mention the PDO base class, even if I use a driver subclass instead.

I wasn't 100% sure at first, but It seems like this is a general behavior: https://3v4l.org/aLUDr#v8.3.12

Anyway, I agree that this should be improved.

kocsismate · 2024-10-22T20:48:11Z

Since I'm leaving for vacation, I'm merging this. I can do followup changes/fixes if someone reviews it later.

kocsismate · 2024-10-22T20:56:06Z

Closing via 5892991

github-actions bot added Extension: pdo (core) Extension: pdo_dblib Extension: pdo_firebird Extension: pdo_mysql Extension: pdo_odbc Extension: pdo_pgsql Extension: pdo_sqlite ABI break labels Oct 2, 2024

kocsismate force-pushed the fix-gh-16131 branch 3 times, most recently from a017be7 to 63e1e32 Compare October 2, 2024 07:01

kocsismate changed the base branch from master to PHP-8.4 October 2, 2024 07:21

kocsismate mentioned this pull request Oct 2, 2024

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

Closed

cmb69 removed the ABI break label Oct 2, 2024

NattyNarwhal reviewed Oct 2, 2024

View reviewed changes

ext/pdo/php_pdo_driver.h Outdated Show resolved Hide resolved

github-actions bot added the ABI break label Oct 2, 2024

kocsismate marked this pull request as ready for review October 2, 2024 20:45

kocsismate requested review from SakiTakamachi and devnexen as code owners October 2, 2024 20:45

kocsismate requested a review from iluuu1994 October 3, 2024 09:55

cmb69 linked an issue Oct 9, 2024 that may be closed by this pull request

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

Closed

kocsismate force-pushed the fix-gh-16131 branch from 787c9ce to c80949e Compare October 11, 2024 10:37

kocsismate mentioned this pull request Oct 11, 2024

Fix GH-16314 "Pdo\Mysql object is uninitialized" when opening a persistent connection #16369

Merged

Fix phpGH-16131: Prevent mixing PDO sub-classes with different DSN

89bf97f

kocsismate force-pushed the fix-gh-16131 branch from c80949e to 89bf97f Compare October 20, 2024 20:41

kocsismate commented Oct 20, 2024

View reviewed changes

NattyNarwhal approved these changes Oct 22, 2024

View reviewed changes

kocsismate closed this Oct 22, 2024

kocsismate deleted the fix-gh-16131 branch October 22, 2024 20:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix GH-16131: Prevent mixing PDO sub-classes with different DSN #16167

Fix GH-16131: Prevent mixing PDO sub-classes with different DSN #16167

kocsismate commented Oct 2, 2024

TimWolla commented Oct 2, 2024

kocsismate commented Oct 2, 2024

cmb69 commented Oct 2, 2024

NattyNarwhal left a comment •

edited

Loading

kocsismate commented Oct 2, 2024

NattyNarwhal commented Oct 9, 2024

kocsismate commented Oct 10, 2024

iluuu1994 commented Oct 10, 2024

kocsismate commented Oct 11, 2024 •

edited

Loading

cmb69 commented Oct 11, 2024

nielsdos commented Oct 17, 2024

cmb69 commented Oct 17, 2024

kocsismate commented Oct 18, 2024

kocsismate left a comment

kocsismate Oct 20, 2024

kocsismate Oct 20, 2024 •

edited

Loading

kocsismate Oct 20, 2024 •

edited

Loading

kocsismate Oct 20, 2024

kocsismate Oct 20, 2024

NattyNarwhal left a comment

cmb69 commented Oct 22, 2024

kocsismate commented Oct 22, 2024 •

edited

Loading

kocsismate commented Oct 22, 2024

kocsismate commented Oct 22, 2024

Fix GH-16131: Prevent mixing PDO sub-classes with different DSN #16167

Fix GH-16131: Prevent mixing PDO sub-classes with different DSN #16167

Conversation

kocsismate commented Oct 2, 2024

TimWolla commented Oct 2, 2024

kocsismate commented Oct 2, 2024

cmb69 commented Oct 2, 2024

NattyNarwhal left a comment • edited Loading

Choose a reason for hiding this comment

kocsismate commented Oct 2, 2024

NattyNarwhal commented Oct 9, 2024

kocsismate commented Oct 10, 2024

iluuu1994 commented Oct 10, 2024

kocsismate commented Oct 11, 2024 • edited Loading

cmb69 commented Oct 11, 2024

nielsdos commented Oct 17, 2024

cmb69 commented Oct 17, 2024

kocsismate commented Oct 18, 2024

kocsismate left a comment

Choose a reason for hiding this comment

kocsismate Oct 20, 2024

Choose a reason for hiding this comment

kocsismate Oct 20, 2024 • edited Loading

Choose a reason for hiding this comment

kocsismate Oct 20, 2024 • edited Loading

Choose a reason for hiding this comment

kocsismate Oct 20, 2024

Choose a reason for hiding this comment

kocsismate Oct 20, 2024

Choose a reason for hiding this comment

NattyNarwhal left a comment

Choose a reason for hiding this comment

cmb69 commented Oct 22, 2024

kocsismate commented Oct 22, 2024 • edited Loading

kocsismate commented Oct 22, 2024

kocsismate commented Oct 22, 2024

NattyNarwhal left a comment •

edited

Loading

kocsismate commented Oct 11, 2024 •

edited

Loading

kocsismate Oct 20, 2024 •

edited

Loading

kocsismate Oct 20, 2024 •

edited

Loading

kocsismate commented Oct 22, 2024 •

edited

Loading