[PHP-CVS] Re: [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
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-xclickhosted_button_id=FS9NLTNEEKWBE -- PHP CVS Mailing List (http://www.php.net/) To unsubscribe, visit: http://www.php.net/unsub.php
[PHP-CVS] Re: [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
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 it?
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-xclickhosted_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
[PHP-CVS] Re: [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
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-xclickhosted_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
[PHP-CVS] Re: [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
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-xclickhosted_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
[PHP-CVS] Re: [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
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.
:)
(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-xclickhosted_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 CVS Mailing List (http://www.php.net/)
To unsubscribe, visit: http://www.php.net/unsub.php
[PHP-CVS] Re: [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
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-xclickhosted_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 CVS Mailing List (http://www.php.net/)
To unsubscribe, visit: http://www.php.net/unsub.php
