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
