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

small fix - wr_store.c #26

Open
wants to merge 1 commit into
base: topic/fix-segfault
Choose a base branch
from

Conversation

commercebyte
Copy link

old_dtors it actually holds zend_object_dtor_obj_t's, not zval's, so Z_PTR_P() didn't do any good

old_dtors it actually holds zend_object_dtor_obj_t's, not zval's, so Z_PTR_P() didn't do any good
@colder
Copy link
Owner

colder commented Mar 15, 2016

I don't understand how this can be correct. In php7 hashtables always hold zvals. To store other contents, we have the _ptr API to automate the wrap/unwrap. Since we don't have it for apply_with_arguments, I really think we have to access through Z_PTR_P within the function.

See for instance http://lxr.php.net/xref/PHP_7_0/ext/reflection/php_reflection.c#_extension_class_string
called here: http://lxr.php.net/xref/PHP_7_0/ext/reflection/php_reflection.c#1120

@commercebyte
Copy link
Author

wr_store.c:115 - in wr_store_track() -
zend_hash_index_update_ptr(&store->old_dtors, handlers_key, ref_obj->handlers->dtor_obj);

the value assigned to the hash is zend_object_dtor_obj_t, not zval,
so we need to treat the hash value as zend_object_dtor_obj_t, treating it as zval causes another seg fault

i agree it might be dirty, but looks like it's been working and it still does
when i have more time i'll consider cleaning up the code and turning the hash values into zvals

@colder
Copy link
Owner

colder commented Mar 24, 2016

Yes, but: the _ptr family of functions will wrap/unwrap the value stored in the array in zvals. What ends up in the array remains a zval, even if you store it using update_ptr. (See for instance https://wiki.php.net/phpng-upgrading#hashtable_api)

It might be that the original issue is avoided with any non-NULL value for dtor, and thus it is not relevant if what is stored is invalid?

Did you experience any specific issue with my version of the patch (that does the unwrapping) ?

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.

None yet

2 participants