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
