Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PHP 8.4.0RC4: Zend Observer does not work for PDO query() method when the new PDO connect() is used #16857

Open
sjayexec opened this issue Nov 19, 2024 · 6 comments
Labels
Bug Category: Documentation This is for documentation inside php-src, not on php.net Extension: pdo (core) Status: Needs Triage

Comments

@sjayexec
Copy link

Description

Problem Description
We have a custom PHP extension that registers as an observer for PDO extension's methods like __construct(), query(), etc., and our extension's handler functions get invoked when any of these PDO methods are being executed. This code is pretty generic from our side and is the same for all PDO methods we observe. But starting from PHP 8.4.0, our observer functions/function handlers are not being invoked when PDO query() method is being executed (when connect() is used) - the execution directly goes to PDO query() method instead of our registered function handlers. There is some crucial, unexplained behavioral difference I want to highlight here considering PHP introduced connect() method to create PDO objects. We adapted our code to also register observer & function handler for the new connect() method and our function handler is being invoked correctly for connect() method but not for query() method that is executed after the connect().

So:
If PDO::__construct() is used by PHP app, our extension's function handlers for __construct() AND query() are both correctly invoked by the Zend engine.
If PDO::connect() is used by PHP app, only connect() function handler in our extension is invoked but not the function handler for query().

This is inconsistent behaviour from the Zend engine.

Just to clarify how we use Zend observer API:
During MINIT:

  1. We use zend_observer_fcall_register() to register begin and end function handlers.
  2. We use the CG() macro to get class entry object for "PDO" class and overwrite the zend_function->internal_function_handler with our own function for __construct(), connect(), query()

We verified some aspects during runtime using a debugger :

  1. Looking into the CG hashtable after we register our functions - we can confirm that zend_function->internal_function_handler for both connect() and query() hold the pointers to our extension's overwritten function handler, not the original PDO methods. 2. Inside ZEND_DO_FCALL_SPEC_OBSERVER_HANDLER() in zend_vm_execute.h from where these two functions/PDO methods are eventually invoked, we can see that the fbc->internal_function.handler for during query() execution holds some unresolved address - that is neither pointing to zim_PDO_query() or our extension's overwritten function handler.

Strange thing is, for the PDO connect() execution, fbc->internal_function.handler does point to our extension's function handler. As I said, registering observer and overwriting function handlers for these functions in the CG hashtable is generic and has been working on PHP 8.3.

PHP Version

PHP 8.4.0RC4

Operating System

No response

@cmb69
Copy link
Member

cmb69 commented Nov 19, 2024

As of PHP 8.4.0, there are now possibly driver specific subclasses (implemented for all PDO extension in php-src; not necessarily for external PDO extensions). Calling PDO::connect() will return a driver specific subclass if available. new PDO() will not; however, the driver specific subclasses have their own constructors, so users could do new Pdo\Sqlite(). See https://wiki.php.net/rfc/pdo_driver_specific_subclasses for details.

That means, that your extension will not only fail for PDO::connect() (assuming that a driver speciifc subclass exists), but also when userland code directly instantiates a driver specific subclass. So I think you need to update your extension to observe all desired methods of driver specific subclasses, too.

I don't think there is anything to do in php-src.

@sjayexec
Copy link
Author

Thanks! I understand this part about driver-specific objects being returned - but if I'm not wrong, in extension domain, query() method of PDO base class will be executed at runtime if there is no driver-specific subclass implementation for query()?

In our case, the demo Symfony app is using a sqlite database, so the PDO driver is sqlite and it doesn't seem to have overridden query() method in pdo_sqlite internal extension. Moreover, while stepping through with a debugger, we can clearly see that PDO::query() is being executed after PDO::connect() in this case. Since we have registered our observer and overwritten the function handler for PDO::query(), we are expecting that our function handler is invoked when zend is processing PDO::query() but that is not happening. I understand that if PDO_SQlite::query() is being executed, we cannot expect our observer on PDO::query() to be invoked as that does not make sense - but that is not the case here so far as I see the behavior.

1. MINIT - pdo ext 
2. MINIT - pdo_sqlite ext
3. MINIT - our ext - we register our observer and overwrite function handles in CG() table for PDO class methods `connect()`, `query()`, `exec()`, etc.
4. Symfony app executes `PDO::connect()` passing the `sqlite` dsn
5. Our extension's registered/overwritten function handler gets invoked from `ZEND_DO_FCALL_SPEC_OBSERVER_HANDLER()`
6. Our extension's overwritten function executes original function handler i.e., `PDO::connect()`
7. Symfony executes a query
8. <Our extension is not being invoked here>
9. `PDO::query()` is being invoked from `ZEND_DO_FCALL_SPEC_OBSERVER_HANDLER()`

Pausing at step 5 in ZEND_DO_FCALL_SPEC_OBSERVER_HANDLER(), we see that fbc holds the pointer to our overwritten function handler for PDO::connect()

Pausing at step 9 in ZEND_DO_FCALL_SPEC_OBSERVER_HANDLER(), we see that fbc holds pointer to original PDO::query() method even though in Step 3 we have overwritten the handler with our own function similar to what you see in above image for connect().

Below are the chronological order printouts I've collected of the function handlers registered for PDO::connect() and PDO::query() methods via CG() macro:

MINIT -- BEFORE -- pdo__connect() handler == 55a0b4dc
MINIT -- BEFORE -- pdo__query() handler == 55a0e582

// register observer and overwrite the function handlers in CG() hashtable

MINIT -- AFTER -- pdo__connect() handler == e9522900
MINIT -- AFTER -- pdo__query() handler == e9522b80

//PHP request starts
RINIT -- pdo__connect() handler == e9522900
RINIT -- pdo__query() handler == e9522b80

//During exection, peering into fbc object via debugger from which the function/methods are invoked
p fbc.internal_function.handler
(zif_handler) 0x0000555555a0e582 (php`zim_PDO_query at pdo_dbh.c:1183:1)

//PHP request ends
RSHUTDOWN -- pdo__connect() handler == e9522900
RSHUTDOWN -- pdo__query() handler == e9522b80

The fact that PDO::query() is executed (as seen in call stack screenshot) is baffling because the class table shows that for PDO::query(), the function handler is our extension's overwritten function.

I don't know if I'm missing something basic here, sorry. But since PDO::query() is getting called, after PDO::connect() was used, we assume the overwritten function handler must be called - there is no subclass related query() calls or anything so far as we see. Could you please help make sense of the above observations and printouts?

@sjayexec
Copy link
Author

sjayexec commented Nov 21, 2024

I am able to reproduce the issue with a sample PHP extension that overwrites function handlers for PDO::__construct(), PDO::connect() and PDO::query() methods in MINIT. I've created 2 PHP scripts (uses PDO sqlite::memory driver) to test the extension & print logs:

pdo_test_php_scripts.zip
pdo_construct.php uses PDO::__construct() + PDO::query()
pdo_connect.php uses PDO::connect() + PDO::query().

The results are same as we reported with our extension.
log file test_pdo_construct.log -> both our handlers for PDO::__construct() and PDO::query() are invoked
log file test_pdo_connect.log -> our handler for only PDO::connect() is invoked

Here is the source code and prebuilt extension binary: reprod_source_and_sharedlib.zip.
The sample extension name is reprod built for PHP 8.4.0RC4 NTS DEBUG. I used ext_skel.php for skeleton & phpize - if you want to build from reprod.c.

Hope this helps.

@bwoebi
Copy link
Member

bwoebi commented Nov 21, 2024

@sjayexec As @cmb69 noted, it's related to the driver-subclasses. Thanks to inheritance, the internal_function is copied onto the class table of the PdoSqlite subclass (do_inherit_method calls zend_duplicate_function). This happens before your extensions MINIT is executed. And that duplicated function will still have the original handler.
When using new PDO() ... the class stays PDO (hence it works, because that one you replace). When using PDO::connect() you get returned one of these fancy new PDO subclasses, whose internal_function still has the original handler. If you also replaced the handler on PdoSqlite, it will work for you too.

@cmb69 cmb69 added the Category: Documentation This is for documentation inside php-src, not on php.net label Nov 21, 2024
@cmb69
Copy link
Member

cmb69 commented Nov 21, 2024

Thanks to inheritance, the internal_function is copied onto the class table of the PdoSqlite subclass (do_inherit_method calls zend_duplicate_function).

I don't think there is anything to do in php-src.

Maybe there is, namely to document the details of inheritance (and trait composition). Especially that an inherited function is actually copied, is not necessarily what a programmer might assume who is used to VMTs.

@sjayexec
Copy link
Author

sjayexec commented Nov 21, 2024

I think I understand now. So, pdo_sqlite extension registers a new class pdo\sqlite (the key for its entry in class_table seems to be pdo\sqlite) that inherits query() method into its function_table - but this is basically the same as the function handler for pdo extension's pdo class.

I might have been thrown off by the fact that even when query() method is being executed from pdo\sqlite 's class entry/function table, the method name in the call stack reads zim_pdo_query() with the extension name jammed between zim/zif and the actual function name, so I was on the lookout for zim_pdo_sqlite_query() or some such name. But it sort of makes sense now that the method name is zim_pdo_query for pdo\sqlite class entry. Without this insight into the internals that you've explained, this might be quite hard to grasp.

So, we should have to explicitly overwrite the function handlers of all possible PDO driver specific class entries of any given PDO method we want to overwrite. Thanks a lot for your answers and inputs, it is of great help. Appreciate it! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Category: Documentation This is for documentation inside php-src, not on php.net Extension: pdo (core) Status: Needs Triage
Projects
None yet
Development

No branches or pull requests

3 participants