search

colder / php-weakref

PECL extension that implements weak references and weak maps in PHP
http://pecl.php.net/weakref
Other
36 stars 13 forks source link

Modifying const data #21

Closed pinepain closed 7 years ago

pinepain commented 8 years ago

From wr_store.c:L91:

((zend_object_handlers *)ref_obj->handlers)->dtor_obj = wr_store_tracked_object_dtor;

but handlers specified as const zend_object_handlers *handlers and modifying pointer to const value is something that specified as an undefined behavior.

Can you pleas clarify what is the reason do cast away constness apart of having handlers pointer value the same so spl_object_hash() will also be the same?

colder commented 8 years ago

This is indeed a tad bit hackish, because we cast the const away.

The alternative would be to replace the entire handlers struct (I'm guessing that's what you do?) but as you point out this means that spl_object_hash is no longer stable.

I would say that affecting the return value of spl_object_hash in an observable way is a big no-no. A solution would be for weakref to replace the implementation of spl_object_hash so that it returns the hash with respect to the old handlers pointer. That would work I guess but it seems almost more hackish that casting the const away.

pinepain commented 8 years ago

Yes, this is what I do. I thought about replacinf spl_object_hash() by own implementation, but I don't think it is better magic than spl_object_hash(). After discussing the problem in SO php chat there was suggestion to just remote that object handlers pointer from hash value generation, so I made this PR - https://github.com/php/php-src/pull/1724. We'll see how it pass or not. I personally don't like both current spl_object_hash() inconsistance introduced by changing handlers pointer and also don't like cast to non-const value, mostly because it changes handler globally.

colder commented 8 years ago

I have a tentative implementation here that makes spl_object_hash stable here: https://github.com/colder/php-weakref/tree/topic/noconst. I'm quite uncomfortable with it though, so I guess I will wait until your PR gets merged before fixing the const thing.

Another option would be to make hashing a handler itself, which would allow our extensions to override.

pinepain commented 8 years ago

JFYI, that PR got merged.

pinepain commented 8 years ago

I tried your approach to patch function at runtime and it works in most cases, except SplObjectHash uses in it getHash() method and get_debug_info handler internal spl_object_hash() implementation. At this point I'm not sure whether it worth to patch it too or not.