I initially looked at the final fix when I discovered the issue. Follow me out on this. This is the current code as-implemented in r323563:
265 zval *obj; 266 MAKE_STD_ZVAL(obj); 267 if (Z_OBJ_HANDLER_P(*arg, cast_object)(*arg, obj, type TSRMLS_CC) == SUCCESS) { 268 zval_ptr_dtor(arg); 269 *arg = obj; 270 *pl = Z_STRLEN_PP(arg); 271 *p = Z_STRVAL_PP(arg); 272 return SUCCESS; 273 } 274 efree(obj); The issue that I originally identified (overwriting the argument pointer) is still happening. Is there any reason for overwriting the arg pointer? Wouldn't it be better to just do the Z_STRLEN_PP and Z_STRVAL_PP operations on obj instead, and zval_ptr_dtor it as well (instead of efree, as that way if a reference is stored somewhere it won't result in a double free, or a segfault for accessing freed memory)? Thanks, Anthony On Mon, Feb 27, 2012 at 11:39 AM, Xinchen Hui <larue...@gmail.com> wrote: > Sent from my iPad > > 在 2012-2-28,0:10,Anthony Ferrara <ircmax...@gmail.com> 写道: > >> Out of curiosity, why are you changing it to copy the object for the >> result of the cast operation? cast_object should init the result >> zval, so why go through the step of copying the starting object to > plz look at the final fix: r323563 > > thanks >> r323563 >> Wouldn't it be easier just to do: >> >> if (Z_OBJ_HANDLER_PP(arg, cast_object)) { >> zval *result; >> ALLOC_ZVAL(result); >> if (Z_OBJ_HANDLER_P(*arg, cast_object)(*arg, result, type TSRMLS_CC) >> == SUCCESS) { >> zval_ptr_dtor(arg); >> *pl = Z_STRLEN_PP(result); >> *p = Z_STRVAL_PP(result); >> zval_ptr_dtor(result); >> return SUCCESS; >> } >> zval_ptr_dtor(result); >> } >> >> Keeping both completely separate, and not having the possibility of >> corrupting the arg object pointer? As it is right now (with the patch >> in the first mail), wouldn't the possibility still exist of nuking the >> arg object pointer which could be used elsewhere (and hence cause the >> memory leak and segfault when that variable is referenced again)? >> >> (Un tested as of yet, just throwing it out there as it seems kind of >> weird to overwrite the arg pointer for what seems like no reason)... >> >> Anthony >> >> >> >> On Mon, Feb 27, 2012 at 10:22 AM, Richard Lynch <c...@l-i-e.com> wrote: >>> On Mon, February 27, 2012 2:31 am, Laruence wrote: >>>> On Mon, Feb 27, 2012 at 4:00 PM, Dmitry Stogov <dmi...@zend.com> >>>> wrote: >>>>> Hi Laruence, >>>>> >>>>> The attached patch looks wired. The patch on top of it (r323563) >>>>> makes it >>>>> better. However, in my opinion it fixes a common problem just in a >>>>> single >>>>> place. Each call to __toString() that makes "side effects" may cause >>>>> the >>>>> similar problem. It would be great to make a "right" fix in >>>>> zend_std_cast_object_tostring() itself, but probably it would >>>>> require API >>>> Hi: >>>> before this fix, I thought about the same idea of that. >>>> >>>> but, you know, such change will need all exts who implmented >>>> their own cast_object handler change there codes too. >>>> >>>> for now, I exam the usage of std_cast_object_tostring, most of >>>> them do the similar things like this fix to avoid this issues(like >>>> ZEND_CAST handler). >>>> >>>> so I think, maybe it's okey for a temporary fix :) >>> >>> Perhaps a better solution would be to make a NEW function that uses >>> zval** and deprecate the old one with memory leaks. >>> >>> Old extensions remain functional, new extension consume less memory. >>> >>> (This presumes I actually understand the issue, which is questionable.) >>> >>> -- >>> brain cancer update: >>> http://richardlynch.blogspot.com/search/label/brain%20tumor >>> Donate: >>> https://www.paypal.com/cgi-bin/webscr?cmd=_s-xclick&hosted_button_id=FS9NLTNEEKWBE >>> >>> >>> >>> -- >>> PHP Internals - PHP Runtime Development Mailing List >>> To unsubscribe, visit: http://www.php.net/unsub.php >>> -- PHP CVS Mailing List (http://www.php.net/) To unsubscribe, visit: http://www.php.net/unsub.php