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 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. :) > (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: >>>> >>>> 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