# #53437 - Thread: https://externals.io/message/67635 - Original archive: https://news-web.php.net/php.internals/67635 - Started by Pierre Joye on 2013-06-06T06:53:13+00:00 - Messages: 20 - Reply by email to internals@lists.php.net - More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt --- ## #67635 — Pierre Joye — 2013-06-06T06:53:13+00:00 URL: https://externals.io/message/67635#67635 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 --- ## #67633 — Derick Rethans — 2013-06-06T10:04:52+00:00 URL: https://externals.io/message/67635#67633 · In reply to: #67635 > 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 --- ## #67634 — Pierre Joye — 2013-06-06T16:17:52+00:00 URL: https://externals.io/message/67635#67634 · In reply to: #67633 > > > > > […] > > 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 --- ## #67640 — Derick Rethans — 2013-06-07T10:45:25+00:00 URL: https://externals.io/message/67635#67640 · In reply to: #67634 > > > […] > > 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 --- ## #67641 — Anatol Belski — 2013-06-07T11:11:15+00:00 URL: https://externals.io/message/67635#67641 · In reply to: #67640 Hi Derick, > > > > > […] > > 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 --- ## #67643 — Derick Rethans — 2013-06-07T12:06:11+00:00 URL: https://externals.io/message/67635#67643 · In reply to: #67641 > > > […] > > 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 --- ## #67645 — Anatol Belski — 2013-06-07T14:27:35+00:00 URL: https://externals.io/message/67635#67645 · In reply to: #67643 Hi Derick, > > > > > […] > > ["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 --- ## #67648 — Gustavo Lopes — 2013-06-07T19:27:30+00:00 URL: https://externals.io/message/67635#67648 · In reply to: #67643 > > […] > > ["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 --- ## #67671 — Pierre Joye — 2013-06-10T11:05:09+00:00 URL: https://externals.io/message/67635#67671 · In reply to: #67648 hi, > > > > […] > > > 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 --- ## #67672 — Etienne Kneuss — 2013-06-10T11:24:19+00:00 URL: https://externals.io/message/67635#67672 · In reply to: #67648 On Fri, Jun 7, 2013 at 9:27 PM, Gustavo Lopes wrote: > > > That's the one where conversion int <> string for serialization was > > […] > > 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 --- ## #67673 — Pierre Joye — 2013-06-10T11:47:01+00:00 URL: https://externals.io/message/67635#67673 · In reply to: #67672 > > 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 --- ## #67674 — Anatol Belski — 2013-06-10T11:56:02+00:00 URL: https://externals.io/message/67635#67674 · In reply to: #67672 Hi Etienne, > On Fri, Jun 7, 2013 at 9:27 PM, Gustavo Lopes > wrote: > > > > […] > 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 --- ## #67675 — Etienne Kneuss — 2013-06-10T12:04:19+00:00 URL: https://externals.io/message/67635#67675 · In reply to: #67674 > Hi Etienne, > > > > […] > > 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 --- ## #67676 — Etienne Kneuss — 2013-06-10T12:07:48+00:00 URL: https://externals.io/message/67635#67676 · In reply to: #67673 > > > > […] > > > […] > > 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 --- ## #67677 — johannes@schlueters.de — 2013-06-10T12:33:27+00:00 URL: https://externals.io/message/67635#67677 · In reply to: #67676 > 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 --- ## #67678 — Anatol Belski — 2013-06-10T12:34:25+00:00 URL: https://externals.io/message/67635#67678 · In reply to: #67675 > > > > > […] > > > This is the case for most string<->int conversions, isn't it? > Exactly. The expressed concerns are about int to string part. > > > […] > > 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 --- ## #67679 — Pierre Joye — 2013-06-10T13:14:34+00:00 URL: https://externals.io/message/67635#67679 · In reply to: #67677 On Mon, Jun 10, 2013 at 2:33 PM, Johannes Schlüter wrote: > > > > > […] > > 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 --- ## #67680 — Stas Malyshev — 2013-06-10T17:51:36+00:00 URL: https://externals.io/message/67635#67680 · In reply to: #67674 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 --- ## #67681 — Stas Malyshev — 2013-06-10T17:54:31+00:00 URL: https://externals.io/message/67635#67681 · In reply to: #67671 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 --- ## #67682 — Anatol Belski — 2013-06-10T18:23:46+00:00 URL: https://externals.io/message/67635#67682 · In reply to: #67681 Hi Stas, On Mon, 2013-06-10 at 10:54 -0700, Stas Malyshev wrote: > Hi! > > > […] > > 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