Welcome! Log In Create A New Profile

Advanced

[PHP-DEV] Re: [PHP-CVS] svn: /php/php-src/ branches/PHP_5_3/NEWS branches/PHP_5_3/Zend/zend_API.c trunk/NEWS trunk/Zend/zend_API.c

Posted by Laruence 
Dmitry:
you might want to review this fix.

let me explain why crash before this fix.

when doing parse_parameter, then convert the object to string by
calling the ce->cast_object,

and passed the same pointer(although there was a separation), to
the cast_object..

then if __toString method stash $this somewhere, after the
parameters clean up, the $this pointer will be impending..

then in the next loop, the return_value will happen used the same adress,,

then balalala, cause the segfault..

sorry for my poor english, and hope I have made myself clearly,
if there is any question , plz write me.

thanks

On Sat, Feb 25, 2012 at 12:36 PM, Xinchen Hui <[email protected]> wrote:
> laruence                                 Sat, 25 Feb 2012 04:36:08 +0000
>
> Revision: http://svn.php.net/viewvc?view=revision&revision=323489
>
> Log:
> Fixed bug #61165 (Segfault - strip_tags())
>
> Bug: https://bugs.php.net/61165 (Assigned) Segfault - strip_tags()
>
> Changed paths:
>    U   php/php-src/branches/PHP_5_3/NEWS
>    U   php/php-src/branches/PHP_5_3/Zend/zend_API.c
>    U   php/php-src/trunk/NEWS
>    U   php/php-src/trunk/Zend/zend_API.c
>
> Modified: php/php-src/branches/PHP_5_3/NEWS
> ===================================================================
> --- php/php-src/branches/PHP_5_3/NEWS   2012-02-25 03:19:27 UTC (rev 323488)
> +++ php/php-src/branches/PHP_5_3/NEWS   2012-02-25 04:36:08 UTC (rev 323489)
> @@ -3,6 +3,7 @@
>  ?? ??? 2012, PHP 5.3.11
>
>  - Core:
> +  . Fixed bug #61165 (Segfault - strip_tags()). (Laruence)
>   . Improved max_input_vars directive to check nested variables (Dmitry).
>   . Fixed bug #61095 (Incorect lexing of 0x00*+<NUM>). (Etienne)
>   . Fixed bug #61072 (Memory leak when restoring an exception handler).
>
> Modified: php/php-src/branches/PHP_5_3/Zend/zend_API.c
> ===================================================================
> --- php/php-src/branches/PHP_5_3/Zend/zend_API.c        2012-02-25 03:19:27 UTC (rev 323488)
> +++ php/php-src/branches/PHP_5_3/Zend/zend_API.c        2012-02-25 04:36:08 UTC (rev 323489)
> @@ -254,10 +254,15 @@
>  static int parse_arg_object_to_string(zval **arg TSRMLS_DC) /* {{{ */
>  {
>        if (Z_OBJ_HANDLER_PP(arg, cast_object)) {
> -               SEPARATE_ZVAL_IF_NOT_REF(arg);
> -               if (Z_OBJ_HANDLER_PP(arg, cast_object)(*arg, *arg, IS_STRING TSRMLS_CC) == SUCCESS) {
> +               zval *obj;
> +               ALLOC_ZVAL(obj);
> +               MAKE_COPY_ZVAL(arg, obj);
> +               if (Z_OBJ_HANDLER_P(*arg, cast_object)(*arg, obj, IS_STRING TSRMLS_CC) == SUCCESS) {
> +                       zval_ptr_dtor(arg);
> +                       *arg = obj;
>                        return SUCCESS;
>                }
> +               zval_ptr_dtor(&obj);
>        }
>        /* Standard PHP objects */
>        if (Z_OBJ_HT_PP(arg) == &std_object_handlers || !Z_OBJ_HANDLER_PP(arg, cast_object)) {
>
> Modified: php/php-src/trunk/NEWS
> ===================================================================
> --- php/php-src/trunk/NEWS      2012-02-25 03:19:27 UTC (rev 323488)
> +++ php/php-src/trunk/NEWS      2012-02-25 04:36:08 UTC (rev 323489)
> @@ -6,6 +6,7 @@
>   . World domination
>
>  - Core:
> +  . Fixed bug #61165 (Segfault - strip_tags()). (Laruence)
>   . Fixed bug #61072 (Memory leak when restoring an exception handler).
>     (Nikic, Laruence)
>   . Fixed bug #61000 (Exceeding max nesting level doesn't delete numerical
>
> Modified: php/php-src/trunk/Zend/zend_API.c
> ===================================================================
> --- php/php-src/trunk/Zend/zend_API.c   2012-02-25 03:19:27 UTC (rev 323488)
> +++ php/php-src/trunk/Zend/zend_API.c   2012-02-25 04:36:08 UTC (rev 323489)
> @@ -262,12 +262,17 @@
>  static int parse_arg_object_to_string(zval **arg, char **p, int *pl, int type TSRMLS_DC) /* {{{ */
>  {
>        if (Z_OBJ_HANDLER_PP(arg, cast_object)) {
> -               SEPARATE_ZVAL_IF_NOT_REF(arg);
> -               if (Z_OBJ_HANDLER_PP(arg, cast_object)(*arg, *arg, type TSRMLS_CC) == SUCCESS) {
> +               zval *obj;
> +               ALLOC_ZVAL(obj);
> +               MAKE_COPY_ZVAL(arg, obj);
> +               if (Z_OBJ_HANDLER_P(*arg, cast_object)(*arg, obj, type TSRMLS_CC) == SUCCESS) {
> +                       zval_ptr_dtor(arg);
> +                       *arg = obj;
>                        *pl = Z_STRLEN_PP(arg);
>                        *p = Z_STRVAL_PP(arg);
>                        return SUCCESS;
>                }
> +               zval_ptr_dtor(&obj);
>        }
>        /* Standard PHP objects */
>        if (Z_OBJ_HT_PP(arg) == &std_object_handlers || !Z_OBJ_HANDLER_PP(arg, cast_object)) {
>
>
> --
> PHP CVS Mailing List (http://www.php.net/)
> To unsubscribe, visit: http://www.php.net/unsub.php



--
Laruence  Xinchen Hui
http://www.laruence.com/

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
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 change (e.g. sending zval** instead of zval*). So it could be fixed
properly only in trunk.

Thanks. Dmitry.

On 02/25/2012 08:41 AM, Laruence wrote:
> Dmitry:
> you might want to review this fix.
>
> let me explain why crash before this fix.
>
> when doing parse_parameter, then convert the object to string by
> calling the ce->cast_object,
>
> and passed the same pointer(although there was a separation), to
> the cast_object..
>
> then if __toString method stash $this somewhere, after the
> parameters clean up, the $this pointer will be impending..
>
> then in the next loop, the return_value will happen used the same adress,,
>
> then balalala, cause the segfault..
>
> sorry for my poor english, and hope I have made myself clearly,
> if there is any question , plz write me.
>
> thanks
>
> On Sat, Feb 25, 2012 at 12:36 PM, Xinchen Hui<[email protected]> wrote:
>> laruence Sat, 25 Feb 2012 04:36:08 +0000
>>
>> Revision: http://svn.php.net/viewvc?view=revision&revision=323489
>>
>> Log:
>> Fixed bug #61165 (Segfault - strip_tags())
>>
>> Bug: https://bugs.php.net/61165 (Assigned) Segfault - strip_tags()
>>
>> Changed paths:
>> U php/php-src/branches/PHP_5_3/NEWS
>> U php/php-src/branches/PHP_5_3/Zend/zend_API.c
>> U php/php-src/trunk/NEWS
>> U php/php-src/trunk/Zend/zend_API.c
>>
>> Modified: php/php-src/branches/PHP_5_3/NEWS
>> ===================================================================
>> --- php/php-src/branches/PHP_5_3/NEWS 2012-02-25 03:19:27 UTC (rev 323488)
>> +++ php/php-src/branches/PHP_5_3/NEWS 2012-02-25 04:36:08 UTC (rev 323489)
>> @@ -3,6 +3,7 @@
>> ?? ??? 2012, PHP 5.3.11
>>
>> - Core:
>> + . Fixed bug #61165 (Segfault - strip_tags()). (Laruence)
>> . Improved max_input_vars directive to check nested variables (Dmitry).
>> . Fixed bug #61095 (Incorect lexing of 0x00*+<NUM>). (Etienne)
>> . Fixed bug #61072 (Memory leak when restoring an exception handler).
>>
>> Modified: php/php-src/branches/PHP_5_3/Zend/zend_API.c
>> ===================================================================
>> --- php/php-src/branches/PHP_5_3/Zend/zend_API.c 2012-02-25 03:19:27 UTC (rev 323488)
>> +++ php/php-src/branches/PHP_5_3/Zend/zend_API.c 2012-02-25 04:36:08 UTC (rev 323489)
>> @@ -254,10 +254,15 @@
>> static int parse_arg_object_to_string(zval **arg TSRMLS_DC) /* {{{ */
>> {
>> if (Z_OBJ_HANDLER_PP(arg, cast_object)) {
>> - SEPARATE_ZVAL_IF_NOT_REF(arg);
>> - if (Z_OBJ_HANDLER_PP(arg, cast_object)(*arg, *arg, IS_STRING TSRMLS_CC) == SUCCESS) {
>> + zval *obj;
>> + ALLOC_ZVAL(obj);
>> + MAKE_COPY_ZVAL(arg, obj);
>> + if (Z_OBJ_HANDLER_P(*arg, cast_object)(*arg, obj, IS_STRING TSRMLS_CC) == SUCCESS) {
>> + zval_ptr_dtor(arg);
>> + *arg = obj;
>> return SUCCESS;
>> }
>> + zval_ptr_dtor(&obj);
>> }
>> /* Standard PHP objects */
>> if (Z_OBJ_HT_PP(arg) ==&std_object_handlers || !Z_OBJ_HANDLER_PP(arg, cast_object)) {
>>
>> Modified: php/php-src/trunk/NEWS
>> ===================================================================
>> --- php/php-src/trunk/NEWS 2012-02-25 03:19:27 UTC (rev 323488)
>> +++ php/php-src/trunk/NEWS 2012-02-25 04:36:08 UTC (rev 323489)
>> @@ -6,6 +6,7 @@
>> . World domination
>>
>> - Core:
>> + . Fixed bug #61165 (Segfault - strip_tags()). (Laruence)
>> . Fixed bug #61072 (Memory leak when restoring an exception handler).
>> (Nikic, Laruence)
>> . Fixed bug #61000 (Exceeding max nesting level doesn't delete numerical
>>
>> Modified: php/php-src/trunk/Zend/zend_API.c
>> ===================================================================
>> --- php/php-src/trunk/Zend/zend_API.c 2012-02-25 03:19:27 UTC (rev 323488)
>> +++ php/php-src/trunk/Zend/zend_API.c 2012-02-25 04:36:08 UTC (rev 323489)
>> @@ -262,12 +262,17 @@
>> static int parse_arg_object_to_string(zval **arg, char **p, int *pl, int type TSRMLS_DC) /* {{{ */
>> {
>> if (Z_OBJ_HANDLER_PP(arg, cast_object)) {
>> - SEPARATE_ZVAL_IF_NOT_REF(arg);
>> - if (Z_OBJ_HANDLER_PP(arg, cast_object)(*arg, *arg, type TSRMLS_CC) == SUCCESS) {
>> + zval *obj;
>> + ALLOC_ZVAL(obj);
>> + MAKE_COPY_ZVAL(arg, obj);
>> + if (Z_OBJ_HANDLER_P(*arg, cast_object)(*arg, obj, type TSRMLS_CC) == SUCCESS) {
>> + zval_ptr_dtor(arg);
>> + *arg = obj;
>> *pl = Z_STRLEN_PP(arg);
>> *p = Z_STRVAL_PP(arg);
>> return SUCCESS;
>> }
>> + zval_ptr_dtor(&obj);
>> }
>> /* Standard PHP objects */
>> if (Z_OBJ_HT_PP(arg) ==&std_object_handlers || !Z_OBJ_HANDLER_PP(arg, cast_object)) {
>>
>>
>> --
>> PHP CVS Mailing List (http://www.php.net/)
>> To unsubscribe, visit: http://www.php.net/unsub.php
>
>
>


--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
On Mon, Feb 27, 2012 at 4:00 PM, Dmitry Stogov <[email protected]> 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 :)

thanks
> change (e.g. sending zval** instead of zval*). So it could be fixed properly
> only in trunk.
>
> Thanks. Dmitry.
>
>
> On 02/25/2012 08:41 AM, Laruence wrote:
>>
>> Dmitry:
>>    you might want to review this fix.
>>
>>    let me explain why crash before this fix.
>>
>>    when doing parse_parameter,  then convert the object to string by
>> calling the ce->cast_object,
>>
>>    and passed the same pointer(although there was a separation), to
>> the cast_object..
>>
>>    then if __toString method stash $this somewhere, after the
>> parameters clean up,  the $this pointer will be impending..
>>
>>    then in the next loop, the return_value will happen used the same
>> adress,,
>>
>>    then balalala, cause the segfault..
>>
>>    sorry for my poor english,  and hope I have made myself clearly,
>> if there is any question , plz write me.
>>
>> thanks
>>
>> On Sat, Feb 25, 2012 at 12:36 PM, Xinchen Hui<[email protected]>  wrote:
>>>
>>> laruence                                 Sat, 25 Feb 2012 04:36:08 +0000
>>>
>>> Revision: http://svn.php.net/viewvc?view=revision&revision=323489
>>>
>>> Log:
>>> Fixed bug #61165 (Segfault - strip_tags())
>>>
>>> Bug: https://bugs.php.net/61165 (Assigned) Segfault - strip_tags()
>>>
>>> Changed paths:
>>>    U   php/php-src/branches/PHP_5_3/NEWS
>>>    U   php/php-src/branches/PHP_5_3/Zend/zend_API.c
>>>    U   php/php-src/trunk/NEWS
>>>    U   php/php-src/trunk/Zend/zend_API.c
>>>
>>> Modified: php/php-src/branches/PHP_5_3/NEWS
>>> ===================================================================
>>> --- php/php-src/branches/PHP_5_3/NEWS   2012-02-25 03:19:27 UTC (rev
>>> 323488)
>>> +++ php/php-src/branches/PHP_5_3/NEWS   2012-02-25 04:36:08 UTC (rev
>>> 323489)
>>> @@ -3,6 +3,7 @@
>>>  ?? ??? 2012, PHP 5.3.11
>>>
>>>  - Core:
>>> +  . Fixed bug #61165 (Segfault - strip_tags()). (Laruence)
>>>   . Improved max_input_vars directive to check nested variables (Dmitry).
>>>   . Fixed bug #61095 (Incorect lexing of 0x00*+<NUM>). (Etienne)
>>>   . Fixed bug #61072 (Memory leak when restoring an exception handler).
>>>
>>> Modified: php/php-src/branches/PHP_5_3/Zend/zend_API.c
>>> ===================================================================
>>> --- php/php-src/branches/PHP_5_3/Zend/zend_API.c        2012-02-25
>>> 03:19:27 UTC (rev 323488)
>>> +++ php/php-src/branches/PHP_5_3/Zend/zend_API.c        2012-02-25
>>> 04:36:08 UTC (rev 323489)
>>> @@ -254,10 +254,15 @@
>>>  static int parse_arg_object_to_string(zval **arg TSRMLS_DC) /* {{{ */
>>>  {
>>>        if (Z_OBJ_HANDLER_PP(arg, cast_object)) {
>>> -               SEPARATE_ZVAL_IF_NOT_REF(arg);
>>> -               if (Z_OBJ_HANDLER_PP(arg, cast_object)(*arg, *arg,
>>> IS_STRING TSRMLS_CC) == SUCCESS) {
>>> +               zval *obj;
>>> +               ALLOC_ZVAL(obj);
>>> +               MAKE_COPY_ZVAL(arg, obj);
>>> +               if (Z_OBJ_HANDLER_P(*arg, cast_object)(*arg, obj,
>>> IS_STRING TSRMLS_CC) == SUCCESS) {
>>> +                       zval_ptr_dtor(arg);
>>> +                       *arg = obj;
>>>                        return SUCCESS;
>>>                }
>>> +               zval_ptr_dtor(&obj);
>>>        }
>>>        /* Standard PHP objects */
>>>        if (Z_OBJ_HT_PP(arg) ==&std_object_handlers ||
>>> !Z_OBJ_HANDLER_PP(arg, cast_object)) {
>>>
>>>
>>> Modified: php/php-src/trunk/NEWS
>>> ===================================================================
>>> --- php/php-src/trunk/NEWS      2012-02-25 03:19:27 UTC (rev 323488)
>>> +++ php/php-src/trunk/NEWS      2012-02-25 04:36:08 UTC (rev 323489)
>>> @@ -6,6 +6,7 @@
>>>   . World domination
>>>
>>>  - Core:
>>> +  . Fixed bug #61165 (Segfault - strip_tags()). (Laruence)
>>>   . Fixed bug #61072 (Memory leak when restoring an exception handler).
>>>     (Nikic, Laruence)
>>>   . Fixed bug #61000 (Exceeding max nesting level doesn't delete
>>> numerical
>>>
>>> Modified: php/php-src/trunk/Zend/zend_API.c
>>> ===================================================================
>>> --- php/php-src/trunk/Zend/zend_API.c   2012-02-25 03:19:27 UTC (rev
>>> 323488)
>>> +++ php/php-src/trunk/Zend/zend_API.c   2012-02-25 04:36:08 UTC (rev
>>> 323489)
>>> @@ -262,12 +262,17 @@
>>>  static int parse_arg_object_to_string(zval **arg, char **p, int *pl, int
>>> type TSRMLS_DC) /* {{{ */
>>>  {
>>>        if (Z_OBJ_HANDLER_PP(arg, cast_object)) {
>>> -               SEPARATE_ZVAL_IF_NOT_REF(arg);
>>> -               if (Z_OBJ_HANDLER_PP(arg, cast_object)(*arg, *arg, type
>>> TSRMLS_CC) == SUCCESS) {
>>> +               zval *obj;
>>> +               ALLOC_ZVAL(obj);
>>> +               MAKE_COPY_ZVAL(arg, obj);
>>> +               if (Z_OBJ_HANDLER_P(*arg, cast_object)(*arg, obj, type
>>> TSRMLS_CC) == SUCCESS) {
>>> +                       zval_ptr_dtor(arg);
>>> +                       *arg = obj;
>>>                        *pl = Z_STRLEN_PP(arg);
>>>                        *p = Z_STRVAL_PP(arg);
>>>                        return SUCCESS;
>>>                }
>>> +               zval_ptr_dtor(&obj);
>>>        }
>>>        /* Standard PHP objects */
>>>        if (Z_OBJ_HT_PP(arg) ==&std_object_handlers ||
>>> !Z_OBJ_HANDLER_PP(arg, cast_object)) {
>>>
>>>
>>>
>>> --
>>> PHP CVS Mailing List (http://www.php.net/)
>>> To unsubscribe, visit: http://www.php.net/unsub.php
>>
>>
>>
>>
>



--
Laruence  Xinchen Hui
http://www.laruence.com/

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
On Mon, Feb 27, 2012 at 4:31 PM, Laruence <[email protected]> wrote:
> On Mon, Feb 27, 2012 at 4:00 PM, Dmitry Stogov <[email protected]> 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 :)
what I mean temporary is, apply this fix to 5.3 and 5.4

then do the "right" fix which you said to 5.4.1 :)

thanks
>
> thanks
>> change (e.g. sending zval** instead of zval*). So it could be fixed properly
>> only in trunk.
>>
>> Thanks. Dmitry.
>>
>>
>> On 02/25/2012 08:41 AM, Laruence wrote:
>>>
>>> Dmitry:
>>>    you might want to review this fix.
>>>
>>>    let me explain why crash before this fix.
>>>
>>>    when doing parse_parameter,  then convert the object to string by
>>> calling the ce->cast_object,
>>>
>>>    and passed the same pointer(although there was a separation), to
>>> the cast_object..
>>>
>>>    then if __toString method stash $this somewhere, after the
>>> parameters clean up,  the $this pointer will be impending..
>>>
>>>    then in the next loop, the return_value will happen used the same
>>> adress,,
>>>
>>>    then balalala, cause the segfault..
>>>
>>>    sorry for my poor english,  and hope I have made myself clearly,
>>> if there is any question , plz write me.
>>>
>>> thanks
>>>
>>> On Sat, Feb 25, 2012 at 12:36 PM, Xinchen Hui<[email protected]>  wrote:
>>>>
>>>> laruence                                 Sat, 25 Feb 2012 04:36:08 +0000
>>>>
>>>> Revision: http://svn.php.net/viewvc?view=revision&revision=323489
>>>>
>>>> Log:
>>>> Fixed bug #61165 (Segfault - strip_tags())
>>>>
>>>> Bug: https://bugs.php.net/61165 (Assigned) Segfault - strip_tags()
>>>>
>>>> Changed paths:
>>>>    U   php/php-src/branches/PHP_5_3/NEWS
>>>>    U   php/php-src/branches/PHP_5_3/Zend/zend_API.c
>>>>    U   php/php-src/trunk/NEWS
>>>>    U   php/php-src/trunk/Zend/zend_API.c
>>>>
>>>> Modified: php/php-src/branches/PHP_5_3/NEWS
>>>> ===================================================================
>>>> --- php/php-src/branches/PHP_5_3/NEWS   2012-02-25 03:19:27 UTC (rev
>>>> 323488)
>>>> +++ php/php-src/branches/PHP_5_3/NEWS   2012-02-25 04:36:08 UTC (rev
>>>> 323489)
>>>> @@ -3,6 +3,7 @@
>>>>  ?? ??? 2012, PHP 5.3.11
>>>>
>>>>  - Core:
>>>> +  . Fixed bug #61165 (Segfault - strip_tags()). (Laruence)
>>>>   . Improved max_input_vars directive to check nested variables (Dmitry).
>>>>   . Fixed bug #61095 (Incorect lexing of 0x00*+<NUM>). (Etienne)
>>>>   . Fixed bug #61072 (Memory leak when restoring an exception handler).
>>>>
>>>> Modified: php/php-src/branches/PHP_5_3/Zend/zend_API.c
>>>> ===================================================================
>>>> --- php/php-src/branches/PHP_5_3/Zend/zend_API.c        2012-02-25
>>>> 03:19:27 UTC (rev 323488)
>>>> +++ php/php-src/branches/PHP_5_3/Zend/zend_API.c        2012-02-25
>>>> 04:36:08 UTC (rev 323489)
>>>> @@ -254,10 +254,15 @@
>>>>  static int parse_arg_object_to_string(zval **arg TSRMLS_DC) /* {{{ */
>>>>  {
>>>>        if (Z_OBJ_HANDLER_PP(arg, cast_object)) {
>>>> -               SEPARATE_ZVAL_IF_NOT_REF(arg);
>>>> -               if (Z_OBJ_HANDLER_PP(arg, cast_object)(*arg, *arg,
>>>> IS_STRING TSRMLS_CC) == SUCCESS) {
>>>> +               zval *obj;
>>>> +               ALLOC_ZVAL(obj);
>>>> +               MAKE_COPY_ZVAL(arg, obj);
>>>> +               if (Z_OBJ_HANDLER_P(*arg, cast_object)(*arg, obj,
>>>> IS_STRING TSRMLS_CC) == SUCCESS) {
>>>> +                       zval_ptr_dtor(arg);
>>>> +                       *arg = obj;
>>>>                        return SUCCESS;
>>>>                }
>>>> +               zval_ptr_dtor(&obj);
>>>>        }
>>>>        /* Standard PHP objects */
>>>>        if (Z_OBJ_HT_PP(arg) ==&std_object_handlers ||
>>>> !Z_OBJ_HANDLER_PP(arg, cast_object)) {
>>>>
>>>>
>>>> Modified: php/php-src/trunk/NEWS
>>>> ===================================================================
>>>> --- php/php-src/trunk/NEWS      2012-02-25 03:19:27 UTC (rev 323488)
>>>> +++ php/php-src/trunk/NEWS      2012-02-25 04:36:08 UTC (rev 323489)
>>>> @@ -6,6 +6,7 @@
>>>>   . World domination
>>>>
>>>>  - Core:
>>>> +  . Fixed bug #61165 (Segfault - strip_tags()). (Laruence)
>>>>   . Fixed bug #61072 (Memory leak when restoring an exception handler).
>>>>     (Nikic, Laruence)
>>>>   . Fixed bug #61000 (Exceeding max nesting level doesn't delete
>>>> numerical
>>>>
>>>> Modified: php/php-src/trunk/Zend/zend_API.c
>>>> ===================================================================
>>>> --- php/php-src/trunk/Zend/zend_API.c   2012-02-25 03:19:27 UTC (rev
>>>> 323488)
>>>> +++ php/php-src/trunk/Zend/zend_API.c   2012-02-25 04:36:08 UTC (rev
>>>> 323489)
>>>> @@ -262,12 +262,17 @@
>>>>  static int parse_arg_object_to_string(zval **arg, char **p, int *pl, int
>>>> type TSRMLS_DC) /* {{{ */
>>>>  {
>>>>        if (Z_OBJ_HANDLER_PP(arg, cast_object)) {
>>>> -               SEPARATE_ZVAL_IF_NOT_REF(arg);
>>>> -               if (Z_OBJ_HANDLER_PP(arg, cast_object)(*arg, *arg, type
>>>> TSRMLS_CC) == SUCCESS) {
>>>> +               zval *obj;
>>>> +               ALLOC_ZVAL(obj);
>>>> +               MAKE_COPY_ZVAL(arg, obj);
>>>> +               if (Z_OBJ_HANDLER_P(*arg, cast_object)(*arg, obj, type
>>>> TSRMLS_CC) == SUCCESS) {
>>>> +                       zval_ptr_dtor(arg);
>>>> +                       *arg = obj;
>>>>                        *pl = Z_STRLEN_PP(arg);
>>>>                        *p = Z_STRVAL_PP(arg);
>>>>                        return SUCCESS;
>>>>                }
>>>> +               zval_ptr_dtor(&obj);
>>>>        }
>>>>        /* Standard PHP objects */
>>>>        if (Z_OBJ_HT_PP(arg) ==&std_object_handlers ||
>>>> !Z_OBJ_HANDLER_PP(arg, cast_object)) {
>>>>
>>>>
>>>>
>>>> --
>>>> PHP CVS Mailing List (http://www.php.net/)
>>>> To unsubscribe, visit: http://www.php.net/unsub.php
>>>
>>>
>>>
>>>
>>
>
>
>
> --
> Laruence  Xinchen Hui
> http://www.laruence.com/



--
Laruence  Xinchen Hui
http://www.laruence.com/

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Hi!

>> so I think, maybe it's okey for a temporary fix :)
> what I mean temporary is, apply this fix to 5.3 and 5.4

Don't apply anything to 5.4 now, please.

--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
On Mon, Feb 27, 2012 at 4:53 PM, Stas Malyshev <[email protected]> wrote:
> Hi!
>
>
>>>   so I think,  maybe it's okey for a temporary fix :)
>>
>> what I mean temporary is, apply this fix to 5.3 and 5.4
>
>
> Don't apply anything to 5.4 now, please.
Sure, won't do it without your permission. :)

thanks
>
> --
> Stanislav Malyshev, Software Architect
> SugarCRM: http://www.sugarcrm.com/
> (408)454-6900 ext. 227



--
Laruence  Xinchen Hui
http://www.laruence.com/

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
On 02/27/2012 12:37 PM, Laruence wrote:
> On Mon, Feb 27, 2012 at 4:31 PM, Laruence<[email protected]> wrote:
>> On Mon, Feb 27, 2012 at 4:00 PM, Dmitry Stogov<[email protected]> 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 :)
> what I mean temporary is, apply this fix to 5.3 and 5.4
>
> then do the "right" fix which you said to 5.4.1 :)

we won't be able to change API in 5.4.1, so it's for 5.5.

Thanks. Dmitry.

> thanks
>>
>> thanks
>>> change (e.g. sending zval** instead of zval*). So it could be fixed properly
>>> only in trunk.
>>>
>>> Thanks. Dmitry.
>>>
>>>
>>> On 02/25/2012 08:41 AM, Laruence wrote:
>>>>
>>>> Dmitry:
>>>> you might want to review this fix.
>>>>
>>>> let me explain why crash before this fix.
>>>>
>>>> when doing parse_parameter, then convert the object to string by
>>>> calling the ce->cast_object,
>>>>
>>>> and passed the same pointer(although there was a separation), to
>>>> the cast_object..
>>>>
>>>> then if __toString method stash $this somewhere, after the
>>>> parameters clean up, the $this pointer will be impending..
>>>>
>>>> then in the next loop, the return_value will happen used the same
>>>> adress,,
>>>>
>>>> then balalala, cause the segfault..
>>>>
>>>> sorry for my poor english, and hope I have made myself clearly,
>>>> if there is any question , plz write me.
>>>>
>>>> thanks
>>>>
>>>> On Sat, Feb 25, 2012 at 12:36 PM, Xinchen Hui<[email protected]> wrote:
>>>>>
>>>>> laruence Sat, 25 Feb 2012 04:36:08 +0000
>>>>>
>>>>> Revision: http://svn.php.net/viewvc?view=revision&revision=323489
>>>>>
>>>>> Log:
>>>>> Fixed bug #61165 (Segfault - strip_tags())
>>>>>
>>>>> Bug: https://bugs.php.net/61165 (Assigned) Segfault - strip_tags()
>>>>>
>>>>> Changed paths:
>>>>> U php/php-src/branches/PHP_5_3/NEWS
>>>>> U php/php-src/branches/PHP_5_3/Zend/zend_API.c
>>>>> U php/php-src/trunk/NEWS
>>>>> U php/php-src/trunk/Zend/zend_API.c
>>>>>
>>>>> Modified: php/php-src/branches/PHP_5_3/NEWS
>>>>> ===================================================================
>>>>> --- php/php-src/branches/PHP_5_3/NEWS 2012-02-25 03:19:27 UTC (rev
>>>>> 323488)
>>>>> +++ php/php-src/branches/PHP_5_3/NEWS 2012-02-25 04:36:08 UTC (rev
>>>>> 323489)
>>>>> @@ -3,6 +3,7 @@
>>>>> ?? ??? 2012, PHP 5.3.11
>>>>>
>>>>> - Core:
>>>>> + . Fixed bug #61165 (Segfault - strip_tags()). (Laruence)
>>>>> . Improved max_input_vars directive to check nested variables (Dmitry).
>>>>> . Fixed bug #61095 (Incorect lexing of 0x00*+<NUM>). (Etienne)
>>>>> . Fixed bug #61072 (Memory leak when restoring an exception handler).
>>>>>
>>>>> Modified: php/php-src/branches/PHP_5_3/Zend/zend_API.c
>>>>> ===================================================================
>>>>> --- php/php-src/branches/PHP_5_3/Zend/zend_API.c 2012-02-25
>>>>> 03:19:27 UTC (rev 323488)
>>>>> +++ php/php-src/branches/PHP_5_3/Zend/zend_API.c 2012-02-25
>>>>> 04:36:08 UTC (rev 323489)
>>>>> @@ -254,10 +254,15 @@
>>>>> static int parse_arg_object_to_string(zval **arg TSRMLS_DC) /* {{{ */
>>>>> {
>>>>> if (Z_OBJ_HANDLER_PP(arg, cast_object)) {
>>>>> - SEPARATE_ZVAL_IF_NOT_REF(arg);
>>>>> - if (Z_OBJ_HANDLER_PP(arg, cast_object)(*arg, *arg,
>>>>> IS_STRING TSRMLS_CC) == SUCCESS) {
>>>>> + zval *obj;
>>>>> + ALLOC_ZVAL(obj);
>>>>> + MAKE_COPY_ZVAL(arg, obj);
>>>>> + if (Z_OBJ_HANDLER_P(*arg, cast_object)(*arg, obj,
>>>>> IS_STRING TSRMLS_CC) == SUCCESS) {
>>>>> + zval_ptr_dtor(arg);
>>>>> + *arg = obj;
>>>>> return SUCCESS;
>>>>> }
>>>>> + zval_ptr_dtor(&obj);
>>>>> }
>>>>> /* Standard PHP objects */
>>>>> if (Z_OBJ_HT_PP(arg) ==&std_object_handlers ||
>>>>> !Z_OBJ_HANDLER_PP(arg, cast_object)) {
>>>>>
>>>>>
>>>>> Modified: php/php-src/trunk/NEWS
>>>>> ===================================================================
>>>>> --- php/php-src/trunk/NEWS 2012-02-25 03:19:27 UTC (rev 323488)
>>>>> +++ php/php-src/trunk/NEWS 2012-02-25 04:36:08 UTC (rev 323489)
>>>>> @@ -6,6 +6,7 @@
>>>>> . World domination
>>>>>
>>>>> - Core:
>>>>> + . Fixed bug #61165 (Segfault - strip_tags()). (Laruence)
>>>>> . Fixed bug #61072 (Memory leak when restoring an exception handler).
>>>>> (Nikic, Laruence)
>>>>> . Fixed bug #61000 (Exceeding max nesting level doesn't delete
>>>>> numerical
>>>>>
>>>>> Modified: php/php-src/trunk/Zend/zend_API.c
>>>>> ===================================================================
>>>>> --- php/php-src/trunk/Zend/zend_API.c 2012-02-25 03:19:27 UTC (rev
>>>>> 323488)
>>>>> +++ php/php-src/trunk/Zend/zend_API.c 2012-02-25 04:36:08 UTC (rev
>>>>> 323489)
>>>>> @@ -262,12 +262,17 @@
>>>>> static int parse_arg_object_to_string(zval **arg, char **p, int *pl, int
>>>>> type TSRMLS_DC) /* {{{ */
>>>>> {
>>>>> if (Z_OBJ_HANDLER_PP(arg, cast_object)) {
>>>>> - SEPARATE_ZVAL_IF_NOT_REF(arg);
>>>>> - if (Z_OBJ_HANDLER_PP(arg, cast_object)(*arg, *arg, type
>>>>> TSRMLS_CC) == SUCCESS) {
>>>>> + zval *obj;
>>>>> + ALLOC_ZVAL(obj);
>>>>> + MAKE_COPY_ZVAL(arg, obj);
>>>>> + if (Z_OBJ_HANDLER_P(*arg, cast_object)(*arg, obj, type
>>>>> TSRMLS_CC) == SUCCESS) {
>>>>> + zval_ptr_dtor(arg);
>>>>> + *arg = obj;
>>>>> *pl = Z_STRLEN_PP(arg);
>>>>> *p = Z_STRVAL_PP(arg);
>>>>> return SUCCESS;
>>>>> }
>>>>> + zval_ptr_dtor(&obj);
>>>>> }
>>>>> /* Standard PHP objects */
>>>>> if (Z_OBJ_HT_PP(arg) ==&std_object_handlers ||
>>>>> !Z_OBJ_HANDLER_PP(arg, cast_object)) {
>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> PHP CVS Mailing List (http://www.php.net/)
>>>>> To unsubscribe, visit: http://www.php.net/unsub.php
>>>>
>>>>
>>>>
>>>>
>>>
>>
>>
>>
>> --
>> Laruence Xinchen Hui
>> http://www.laruence.com/
>
>
>


--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
On 02/27/2012 12:53 PM, Stas Malyshev wrote:
> Hi!
>
>>> so I think, maybe it's okey for a temporary fix :)
>> what I mean temporary is, apply this fix to 5.3 and 5.4
>
> Don't apply anything to 5.4 now, please.

No, we are thinking about the trunk, and "temporary" fix for 5.4 is
delayed.

Thanks. Dmitry.



--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
On Mon, Feb 27, 2012 at 11:17 AM, Derick Rethans <[email protected]> wrote:

> You can't break extension APIs between 5.4.0 and 5.4.1 either, API
> changes can only into trunk.


And ABI neither.

--
Pierre

@pierrejoye | http://blog.thepimp.net | http://www.libgd.org

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
On Mon, February 27, 2012 2:31 am, Laruence wrote:
> On Mon, Feb 27, 2012 at 4:00 PM, Dmitry Stogov <[email protected]>
> 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
Sent from my iPad

在 2012-2-28,0:10,Anthony Ferrara <[email protected]> 写道:

> 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 <[email protected]> wrote:
>> On Mon, February 27, 2012 2:31 am, Laruence wrote:
>>> On Mon, Feb 27, 2012 at 4:00 PM, Dmitry Stogov <[email protected]>
>>> 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 Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
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 <[email protected]> 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 <[email protected]> wrote:
>>> On Mon, February 27, 2012 2:31 am, Laruence wrote:
>>>> On Mon, Feb 27, 2012 at 4:00 PM, Dmitry Stogov <[email protected]>
>>>> 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 Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
On Tue, Feb 28, 2012 at 1:10 AM, Anthony Ferrara <[email protected]> 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 <[email protected]> wrote:
>> Sent from my iPad
>>
>> 在 2012-2-28,0:10,Anthony Ferrara <[email protected]> 写道:
>>
>>> 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 <[email protected]> wrote:
>>>> On Mon, February 27, 2012 2:31 am, Laruence wrote:
>>>>> On Mon, Feb 27, 2012 at 4:00 PM, Dmitry Stogov <[email protected]>
>>>>> 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

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
On Tue, Feb 28, 2012 at 10:38 AM, Xinchen Hui <[email protected]> wrote:
> On Tue, Feb 28, 2012 at 1:10 AM, Anthony Ferrara <[email protected]> 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 <[email protected]> wrote:
>>> Sent from my iPad
>>>
>>> 在 2012-2-28,0:10,Anthony Ferrara <[email protected]> 写道:
>>>
>>>> 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 <[email protected]> wrote:
>>>>> On Mon, February 27, 2012 2:31 am, Laruence wrote:
>>>>>> On Mon, Feb 27, 2012 at 4:00 PM, Dmitry Stogov <[email protected]>
>>>>>> 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
Sorry, only registered users may post in this forum.

Click here to login