On Tue, Feb 28, 2012 at 10:38 AM, Xinchen Hui <larue...@gmail.com> wrote: > On Tue, Feb 28, 2012 at 1:10 AM, Anthony Ferrara <ircmax...@gmail.com> wrote: >> 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 > Oops, you are right.. thanks for pointing this out. > :) Sorry, I miss-read your words. so I revoke my previous words.
the reason for why overwriting arg, is we should record that new temp zval(IS_STRING), then release it while doing cleanup parameters. and also, fo 5.3, no p and pl paramters. thanks >> (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 >>>>> > > > > -- > 惠新宸 laruence > Senior PHP Engineer > http://www.laruence.com -- 惠新宸 laruence Senior PHP Engineer http://www.laruence.com -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php