fix(agent): PDO database_object_handle zval to zend_object #1138
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
bduranleau-nr
force-pushed
the
feat/php-8.5
branch
from
b34ede3 to
c91f83b
Compare
bduranleau-nr
force-pushed
the
fix/php85-pdo
branch
from
cc1fe50 to
828fb8e
Compare
zsistla
reviewed
Dec 4, 2025
| #include "nr_datastore_instance.h" | ||
|
|
||
| #if ZEND_MODULE_API_NO >= ZEND_8_5_X_API_NO | ||
| #define PHP_PDO_DBH zend_object* |
Contributor
There was a problem hiding this comment.
Suggest naming this NR_PHP_PDO_DBH to clarify that it's our define vs a PHP define.
zsistla
reviewed
Dec 4, 2025
| */ | ||
| extern pdo_dbh_t* nr_php_pdo_get_database_object(zval* dbh TSRMLS_DC); | ||
|
|
||
| /* Purpose : Returns the pdo_dbh_t struct that is contained in the object store |
Contributor
There was a problem hiding this comment.
Suggested change
| /* Purpose : Returns the pdo_dbh_t struct that is contained in the object store | |
| /* | |
| * Purpose : Returns the pdo_dbh_t struct that is contained in the object store |
zsistla
reviewed
Dec 4, 2025
| #else | ||
| dbh = &pdo_stmt->database_object_handle; | ||
| #endif | ||
| dup = nr_php_pdo_duplicate(dbh TSRMLS_CC); |
Contributor
There was a problem hiding this comment.
It needs a null check somewhere along the path for dbh.
dbh is passed into nr_php_pdo_duplicate, which for 8.5 passes to nr_php_pdo_get_database_object_from_zend_object which dereferences here.
zsistla
reviewed
Dec 4, 2025
|
|
||
| pdo_dbh_t* nr_php_pdo_get_database_object_from_zend_object( | ||
| zend_object* dbh TSRMLS_DC) { | ||
| if (nr_php_class_entry_instanceof_class(dbh->ce, "PDO" TSRMLS_CC)) { |
Contributor
There was a problem hiding this comment.
NULL check before dereferencing dbh.
zsistla
reviewed
Dec 4, 2025
| } | ||
|
|
||
| #if ZEND_MODULE_API_NO >= ZEND_8_5_X_API_NO | ||
| return (zval*)nr_hashmap_index_get(NRTXNGLOBAL(pdo_link_options), |
Contributor
There was a problem hiding this comment.
NULL check before derefencing dbh.
zsistla
reviewed
Dec 4, 2025
Comment on lines
+538
to
+542
| extern int __attribute__((weak)) php_pdo_parse_data_source( | ||
| const char* data_source, | ||
| zend_ulong data_source_len, | ||
| struct pdo_data_src_parser* parsed, | ||
| int nparams); |
zsistla
reviewed
Dec 4, 2025
Comment on lines
+124
to
+129
| static inline pdo_dbh_t* | ||
| nr_php_pdo_get_database_object_internal_from_zend_object( | ||
| zend_object* dbh TSRMLS_DC) { | ||
| return php_pdo_dbh_fetch_inner(dbh); | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
Needs a NULL check for dbh since php_pdo_dbh_fetch_inner doesn't check.
Contributor
|
Great solution to address the issue! Could you please update the description with why the change was needed? |
bduranleau-nr
force-pushed
the
fix/php85-pdo
branch
from
828fb8e to
6017038
Compare
bduranleau-nr
force-pushed
the
fix/php85-pdo
branch
from
bebd53f to
6017038
Compare
bduranleau-nr
force-pushed
the
fix/php85-pdo
branch
from
6017038 to
23d9a34
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.