hi!
The fix for #53437 is around for some time now. It full fills the
requirements described by Derick when we discussed the possible fixes.
Unless there are strong objections in the next couple of days, I will
ask Anatol to apply it on Monday. This is the last remaining crash in
5.3/4 (already applied in 5.5) and needs to be fixed asap.
Cheers,
Pierre
@pierrejoye | http://www.libgd.org
The fix for #53437 is around for some time now. It full fills the
requirements described by Derick when we discussed the possible fixes.Unless there are strong objections in the next couple of days, I will
ask Anatol to apply it on Monday. This is the last remaining crash in
5.3/4 (already applied in 5.5) and needs to be fixed asap.
The last time I checked that it was using weird base64 encoding on stuff
and I am absolutely against that. Where is the new patch?
cheers,
Derick
The fix for #53437 is around for some time now. It full fills the
requirements described by Derick when we discussed the possible fixes.Unless there are strong objections in the next couple of days, I will
ask Anatol to apply it on Monday. This is the last remaining crash in
5.3/4 (already applied in 5.5) and needs to be fixed asap.The last time I checked that it was using weird base64 encoding on stuff
and I am absolutely against that. Where is the new patch?
It is in 5.5 and no, it does not used that as stated in the previous mails.
cheers,
Derick
The fix for #53437 is around for some time now. It full fills the
requirements described by Derick when we discussed the possible
fixes.Unless there are strong objections in the next couple of days, I
will ask Anatol to apply it on Monday. This is the last remaining
crash in 5.3/4 (already applied in 5.5) and needs to be fixed
asap.The last time I checked that it was using weird base64 encoding on
stuff and I am absolutely against that. Where is the new patch?It is in 5.5 and no, it does not used that as stated in the previous
mails.
That didn't answer my question though. I asked where the new patch was.
cheers,
Derick
Hi Derick,
The fix for #53437 is around for some time now. It full fills the
requirements described by Derick when we discussed the possible
fixes.Unless there are strong objections in the next couple of days, I
will ask Anatol to apply it on Monday. This is the last remaining
crash in 5.3/4 (already applied in 5.5) and needs to be fixed asap.The last time I checked that it was using weird base64 encoding on
stuff and I am absolutely against that. Where is the new patch?It is in 5.5 and no, it does not used that as stated in the previous
mails.That didn't answer my question though. I asked where the new patch was.
That's the one where conversion int <> string for serialization was
developed. It came into 5.5 with this patches (the originally proposed
patch is still attached to that ticket)
http://git.php.net/?p=php-src.git;a=commitdiff;h=0ee71557ffd285552659b6aa37ea236e3bad493f
http://git.php.net/?p=php-src.git;a=commitdiff;h=acd160530a3ef41403853fd983bef3082726b690
http://git.php.net/?p=php-src.git;a=commitdiff;h=add5420d89624550b1e6e72f960413ff7d0a69a2
Those are definitely crying to be backported into 5.3 and 5.4 as the
corresponding tests are still crashy.
Regards
Anatol
The fix for #53437 is around for some time now. It full fills the
requirements described by Derick when we discussed the possible
fixes.Unless there are strong objections in the next couple of days, I
will ask Anatol to apply it on Monday. This is the last remaining
crash in 5.3/4 (already applied in 5.5) and needs to be fixed
asap.The last time I checked that it was using weird base64 encoding on
stuff and I am absolutely against that. Where is the new patch?It is in 5.5 and no, it does not used that as stated in the
previous mails.That didn't answer my question though. I asked where the new patch
was.That's the one where conversion int <> string for serialization was
developed. It came into 5.5 with this patches (the originally proposed
patch is still attached to that ticket)http://git.php.net/?p=php-src.git;a=commitdiff;h=0ee71557ffd285552659b6aa37ea236e3bad493f
["days"]=>
- int(3)
- string(1) "3"
and
- 'days' => 0,
- 'days' => '0',
I see this in all test cases - this is a BC break. Even though days is
an int64, I think this should be a (platform) int and not a string in
case it fits. No need to punish people on 64bit platforms. I'd even go
as far as arguing that special_amount should be treated like that too.
The deserializer needs to understand both types anyway.
cheers,
Derick
--
http://derickrethans.nl | http://xdebug.org
Like Xdebug? Consider a donation: http://xdebug.org/donate.php
twitter: @derickr and @xdebug
Posted with an email client that doesn't mangle email: alpine
Hi Derick,
The fix for #53437 is around for some time now. It full fills
the requirements described by Derick when we discussed the
possible fixes.Unless there are strong objections in the next couple of days,
I
will ask Anatol to apply it on Monday. This is the last
remaining crash in 5.3/4 (already applied in 5.5) and needs to
be fixed asap.The last time I checked that it was using weird base64 encoding
on stuff and I am absolutely against that. Where is the new patch?It is in 5.5 and no, it does not used that as stated in the
previous mails.That didn't answer my question though. I asked where the new patch
was.That's the one where conversion int <> string for serialization was
developed. It came into 5.5 with this patches (the originally proposed
patch is still attached to that ticket)http://git.php.net/?p=php-src.git;a=commitdiff;h=0ee71557ffd285552659b6
aa37ea236e3bad493f["days"]=>
- int(3)
- string(1) "3"
and
- 'days' => 0,
- 'days' => '0',
I see this in all test cases - this is a BC break. Even though days is
an int64, I think this should be a (platform) int and not a string in case
it fits. No need to punish people on 64bit platforms. I'd even go as far
as arguing that special_amount should be treated like that too. The
deserializer needs to understand both types anyway.
Just to be said that the idea from the original patch with base64 encoding
was to export all the data without possible losses on 32 and 64 bit,
besides fixing the crash. So I just continued that way. Exporting a 64 int
as long can shrink the serialized value.
This is anyway easy fixable by changing the macros. Also exporting even a
shrinked value wouldn't cause a crash. Do I get it right we can backport
it for 5.3/5.4 when it's fixed?
Cheers
Anatol
That's the one where conversion int <> string for serialization was
developed. It came into 5.5 with this patches (the originally proposed
patch is still attached to that ticket)http://git.php.net/?p=php-src.git;a=commitdiff;h=0ee71557ffd285552659b6aa37ea236e3bad493f
["days"]=>
- int(3)
- string(1) "3"
and
- 'days' => 0,
- 'days' => '0',
I see this in all test cases - this is a BC break.
I don't think this is a BC break, or at least it's a very minor. As I
understand it, when you read the property directly you still get an int:
$iv = "2008-05-11T15:30:00Z/2007-03-01T13:00:00Z";
$di = new DateInterval($iv);
var_dump($di->days); //int(437)
So this applies only to var_dump() output, serialization output and
something exotic as an array cast (which anyway has its own peculiarities
wrt the key type conversion -- or the absence of it).
--
Gustavo Lopes
hi,
That's the one where conversion int <> string for serialization was
developed. It came into 5.5 with this patches (the originally proposed
patch is still attached to that ticket)http://git.php.net/?p=php-src.git;a=commitdiff;h=0ee71557ffd285552659b6aa37ea236e3bad493f
["days"]=>
- int(3)
- string(1) "3"
and
- 'days' => 0,
- 'days' => '0',
I see this in all test cases - this is a BC break.
I don't think this is a BC break, or at least it's a very minor. As I
understand it, when you read the property directly you still get an int:$iv = "2008-05-11T15:30:00Z/2007-03-01T13:00:00Z";
$di = new DateInterval($iv);
var_dump($di->days); //int(437)So this applies only to
var_dump()output, serialization output and
something exotic as an array cast (which anyway has its own peculiarities
wrt the key type conversion -- or the absence of it).
I agree.
Stas, Johannes? We have to fix this crash, in one way or another. The
current patch is good imo.
--
Pierre
@pierrejoye | http://www.libgd.org
Hi!
Stas, Johannes? We have to fix this crash, in one way or another. The
current patch is good imo.
I'm ok with this in 5.4 but I'd really like to fix the var_dump issue if
possible.
--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227
Hi Stas,
On Mon, 2013-06-10 at 10:54 -0700, Stas Malyshev wrote:
Hi!
Stas, Johannes? We have to fix this crash, in one way or another. The
current patch is good imo.I'm ok with this in 5.4 but I'd really like to fix the var_dump issue if
possible.
I've reworked the patch before applying to 5.3 and 5.4 as it was
suggested by Derick and Etienne. Also fixed the same for 5.5. Just
pushed that and doing one more test round. All the properties are
exported as int now, so that's the same in all the branches.
With not portable I meant - for instance the 'days' property is long in
the userspace, but is 64 bit internally. That means occasionally an
overflow might happen. That's why the idea was to serialize it as a
string, so it could work across platforms.
Regards
Anatol
On Fri, Jun 7, 2013 at 9:27 PM, Gustavo Lopes glopes@nebm.ist.utl.ptwrote:
That's the one where conversion int <> string for serialization was
developed. It came into 5.5 with this patches (the originally proposed
patch is still attached to that ticket)http://git.php.net/?p=php-src.git;a=commitdiff;h=
0ee71557ffd285552659b6aa37ea23**6e3bad493fhttp://git.php.net/?p=php-src.git;a=commitdiff;h=0ee71557ffd285552659b6aa37ea236e3bad493f["days"]=>
- int(3)
- string(1) "3"
and
- 'days' => 0,
- 'days' => '0',
I see this in all test cases - this is a BC break.
I don't think this is a BC break, or at least it's a very minor. As I
understand it, when you read the property directly you still get an int:$iv = "2008-05-11T15:30:00Z/2007-03-**01T13:00:00Z";
$di = new DateInterval($iv);
var_dump($di->days); //int(437)So this applies only to
var_dump()output, serialization output and
something exotic as an array cast (which anyway has its own peculiarities
wrt the key type conversion -- or the absence of it).
So if I understand correctly var_dump now indicates a different type than
what accessing the property returns?
Even if the change itself does not constitute a big BC break, this
behavior is confusing and seems like a big no-no to me.
--
Etienne Kneuss
So if I understand correctly var_dump now indicates a different type than
what accessing the property returns?Even if the change itself does not constitute a big BC break, this
behavior is confusing and seems like a big no-no to me.
So a slight change in a debugging function is enough to keep a crash in
many production code? What do you propose then?
--
Etienne Kneuss
So if I understand correctly var_dump now indicates a different type than
what accessing the property returns?Even if the change itself does not constitute a big BC break, this
behavior is confusing and seems like a big no-no to me.So a slight change in a debugging function is enough to keep a crash in
many production code? What do you propose then?
You make it sound like there are only those two options :)
I propose we fix the serialization/unserialization crash in a way that does
not affect var_dump.
--
Etienne Kneuss
I propose we fix the serialization/unserialization crash in a way that
does not affect var_dump.
That was the summary of my message a while back, too. Aside from that I
trust the assessment of extension maintainers.
I want to take this opportunity to remind everybody how close to EOL 5.3
is, though: PHP 5.3 is 26 point releases old and according to
https://wiki.php.net/rfc/php53eol 5.3 will go to security only once 5.5
comes out, which might be by next month or so. Changing behavior so late
in the game is bad, even if it is a small change only.
johannes
On Mon, Jun 10, 2013 at 2:33 PM, Johannes Schlüter
johannes@schlueters.de wrote:
I propose we fix the serialization/unserialization crash in a way that
does not affect var_dump.That was the summary of my message a while back, too. Aside from that I
trust the assessment of extension maintainers.I want to take this opportunity to remind everybody how close to EOL 5.3
is, though: PHP 5.3 is 26 point releases old and according to
https://wiki.php.net/rfc/php53eol 5.3 will go to security only once 5.5
comes out, which might be by next month or so. Changing behavior so late
in the game is bad, even if it is a small change only.
Not sure to follow, we are talking about fixing a crash, a crash is
always bad and as we are still in maintenance mode there is no issue
here. That being said, we need to choose a solution now, for two
reasons:
. fix is already in 5.5, we don't want different fixes in 5.3/4 and 5.5
. it takes way too long to approve the patches, Anatol has put many
efforts to full fill Derick's requirements, it is not a small bug.
Cheers,
Pierre
@pierrejoye | http://www.libgd.org
Hi Etienne,
On Fri, Jun 7, 2013 at 9:27 PM, Gustavo Lopes
glopes@nebm.ist.utl.ptwrote:On Fri, 07 Jun 2013 14:06:11 +0200, Derick Rethans derick@php.net
wrote:That's the one where conversion int <> string for serialization was
developed. It came into 5.5 with this patches (the originally
proposed patch is still attached to that ticket)http://git.php.net/?p=php-src.git;a=commitdiff;h=
0ee71557ffd285552659b6aa37ea23**6e3bad493f<http://git.php.net/?p=php
-src.git;a=commitdiff;h=0ee71557ffd285552659b6aa37ea236e3bad493f>["days"]=>
- int(3)
- string(1) "3"
and
- 'days' => 0,
- 'days' => '0',
I see this in all test cases - this is a BC break.
I don't think this is a BC break, or at least it's a very minor. As I
understand it, when you read the property directly you still get an int:$iv = "2008-05-11T15:30:00Z/2007-03-**01T13:00:00Z";
$di = new DateInterval($iv);
var_dump($di->days); //int(437)So this applies only to
var_dump()output, serialization output and
something exotic as an array cast (which anyway has its own
peculiarities wrt the key type conversion -- or the absence of it).So if I understand correctly var_dump now indicates a different type than
what accessing the property returns?Even if the change itself does not constitute a big BC break, this
behavior is confusing and seems like a big no-no to me.
As I mentioned previously, that small piece is easy fixable, though it
possibly makes that place not portable between 32 and 64 bit. The main
intention is to fix the unserialization crash, which IMHO interleaves this
small deviation.
Regards
Anatol
Hi Etienne,
On Fri, Jun 7, 2013 at 9:27 PM, Gustavo Lopes
glopes@nebm.ist.utl.ptwrote:On Fri, 07 Jun 2013 14:06:11 +0200, Derick Rethans derick@php.net
wrote:That's the one where conversion int <> string for serialization was
developed. It came into 5.5 with this patches (the originally
proposed patch is still attached to that ticket)http://git.php.net/?p=php-src.git;a=commitdiff;h=
0ee71557ffd285552659b6aa37ea23**6e3bad493f<http://git.php.net/?p=php
-src.git;a=commitdiff;h=0ee71557ffd285552659b6aa37ea236e3bad493f>["days"]=>
- int(3)
- string(1) "3"
and
- 'days' => 0,
- 'days' => '0',
I see this in all test cases - this is a BC break.
I don't think this is a BC break, or at least it's a very minor. As I
understand it, when you read the property directly you still get an int:$iv = "2008-05-11T15:30:00Z/2007-03-**01T13:00:00Z";
$di = new DateInterval($iv);
var_dump($di->days); //int(437)So this applies only to
var_dump()output, serialization output and
something exotic as an array cast (which anyway has its own
peculiarities wrt the key type conversion -- or the absence of it).So if I understand correctly var_dump now indicates a different type than
what accessing the property returns?Even if the change itself does not constitute a big BC break, this
behavior is confusing and seems like a big no-no to me.As I mentioned previously, that small piece is easy fixable, though it
possibly makes that place not portable between 32 and 64 bit.
This is the case for most string<->int conversions, isn't it?
The main
intention is to fix the unserialization crash, which IMHO interleaves this
small deviation.
Sure, I understand the intention. It's even more a new feature than a
bug-fix at this point (as there are other ways to prevent this without
actually implementing serialization).
We shouldn't make the output of var_dump less consistent for something that
feels as remote as serialization/unserialization, and especially if it is
easily fixable.
Best,
--
Etienne Kneuss
Hi Etienne,
On Fri, Jun 7, 2013 at 9:27 PM, Gustavo Lopes
glopes@nebm.ist.utl.ptwrote:On Fri, 07 Jun 2013 14:06:11 +0200, Derick Rethans derick@php.net
wrote:That's the one where conversion int <> string for serialization was
developed. It came into 5.5 with this patches (the originally
proposed patch is still attached to that ticket)http://git.php.net/?p=php-src.git;a=commitdiff;h=
0ee71557ffd285552659b6aa37ea23**6e3bad493f<http://git.php.net/?p
=php
-src.git;a=commitdiff;h=0ee71557ffd285552659b6aa37ea236e3bad493f["days"]=>
- int(3)
- string(1) "3"
and
- 'days' => 0,
- 'days' => '0',
I see this in all test cases - this is a BC break.
I don't think this is a BC break, or at least it's a very minor. As
I
understand it, when you read the property directly you still get an
int:$iv = "2008-05-11T15:30:00Z/2007-03-**01T13:00:00Z";
$di = new DateInterval($iv);
var_dump($di->days); //int(437)So this applies only to
var_dump()output, serialization output and
something exotic as an array cast (which anyway has its own
peculiarities wrt the key type conversion -- or the absence of it).So if I understand correctly var_dump now indicates a different type
than what accessing the property returns?Even if the change itself does not constitute a big BC break, this
behavior is confusing and seems like a big no-no to me.As I mentioned previously, that small piece is easy fixable, though it
possibly makes that place not portable between 32 and 64 bit.This is the case for most string<->int conversions, isn't it?
Exactly. The expressed concerns are about int to string part.
The main
intention is to fix the unserialization crash, which IMHO interleaves
this small deviation.Sure, I understand the intention. It's even more a new feature than a
bug-fix at this point (as there are other ways to prevent this without
actually implementing serialization).
Other ways will introduce other BC breaks as well :) The original ticket
was about unserialization, so it's sensible serialization to be touched as
well.
We shouldn't make the output of var_dump less consistent for something
that feels as remote as serialization/unserialization, and especially if
it is easily fixable.
The general question is - the small piece of deviation can be solved,
that's may be 3% of the whole patch. So then it were ready to be
backported. As despite that many hypothetical ways to fix it, the work is
done on this concrete patch, which fixes the crash and is already accepted
in 5.5
Regards
Anatol
Hi!
As I mentioned previously, that small piece is easy fixable, though it
possibly makes that place not portable between 32 and 64 bit. The main
intention is to fix the unserialization crash, which IMHO interleaves this
small deviation.
What you mean by not portable? Different code, different var_dump results?
--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227