Skip to content

Conversation

jmikola
Copy link
Member

@jmikola jmikola commented Jan 9, 2018

https://jira.mongodb.org/browse/PHPC-1092

This removes a call to zend_hash_clean() on successive calls to a BSON object's get_properties handler, which can lead to an infinite loop when iterating the object's properties with foreach on PHP 7.x.

For historical reference, the original call to zend_hash_clean() was introduced in ad1f3f0 for #607 (PHPC-939).

Note: I've only added tests for BSON classes that have a custom get_properties handler and utilize the PHONGO_GET_PROPERTY_HASH_INIT_PROPS() macro. I do not recall why I originally added zend_clean_hash(), but I assume I was trying to be strict about not inheriting hash state from previous calls.

While testing this, I was surprised to find that iterating the object with foreach actually invokes get_properties multiple times. If I had to guess, the fact that zend_hash_clean() resets the hash pointer was causing foreach to never advance past the first property.

@jmikola jmikola requested a review from derickr January 9, 2018 03:03
This removes a call to zend_hash_clean() on successive calls to a BSON object's get_properties handler, which can lead to an infinite loop when iterating the object's properties with foreach on PHP 7.x.

For historical reference, the original call to zend_hash_clean() was introduced in ad1f3f0 for mongodb#607 (PHPC-939).
Copy link
Contributor

@derickr derickr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jmikola jmikola merged commit 37b8eaf into mongodb:master Jan 9, 2018
jmikola added a commit that referenced this pull request Jan 9, 2018
@jmikola jmikola deleted the phpc-1092 branch January 9, 2018 13:27
@jmikola
Copy link
Member Author

jmikola commented Jan 9, 2018

While testing this, I was surprised to find that iterating the object with foreach actually invokes get_properties multiple times. If I had to guess, the fact that zend_hash_clean() resets the hash pointer was causing foreach to never advance past the first property.

Indeed, ZEND_FE_FETCH_R does invoke the get_properties handler on each execution: https://github.com/php/php-src/blob/php-7.2.1/Zend/zend_vm_def.h#L5912

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants