Re: Re: svn: /php/php-src/ branches/PHP_5_3/NEWS branches/PHP_5_3/Zend/zend_API.c trunk/NEWS trunk/Zend/zend_API.c

On Tue, Feb 28, 2012 at 10:38 AM, Xinchen Hui <laruence@gmail.com> wrote: > On Tue, Feb 28, 2012 at 1:10 AM, Anthony Ferrara <ircmaxell@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 <laruence@gmail.com> wrote: >>> Sent from my iPad >>> >>> 在 2012-2-28，0:10，Anthony Ferrara <ircmaxell@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 <ceo@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 <dmitry@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: >>>>> �K�oh�b�o-a
0� >>>>> �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