# An BC issue in unserialize - Thread: https://externals.io/message/75633 - Original archive: https://news-web.php.net/php.internals/75633 - Started by Laruence on 2014-07-17T04:01:10+00:00 - Messages: 10 - 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 --- ## #75633 — Laruence — 2014-07-17T04:01:10+00:00 URL: https://externals.io/message/75633#75633 Hey: I was trying play with OroCRM. then I find a BC break in PHP-5.6 the usage is : /** * Creates a new instance of the mapped class, without invoking the constructor. * * @return object */ public function newInstance() { if ($this->_prototype === null) { $this->_prototype = unserialize(sprintf('O:%d:"%s":0:{}', strlen($this->name), $this->name)); } return clone $this->_prototype; } it works fine in PHP-5.5, but in 5.6 , the serialize will fail after fix 5328d4289946e260232f3195ba2e0f0eb173d5ef then I found, Stas did a re-fix for that: 342240fd7fb6ac0a287eb6f912c4d61d6274d68c but it seems doesn't go into 5.6.. I am not sure now. should this usage be supported? thanks -- Laruence Xinchen Hui http://www.laruence.com/ --- ## #75635 — Remi Collet — 2014-07-17T05:09:29+00:00 URL: https://externals.io/message/75633#75635 · In reply to: #75633 Le 17/07/2014 06:01, Laruence a écrit : > $this->_prototype = unserialize(sprintf('O:%d:"%s":0:{}', > strlen($this->name), $this->name)); > I am not sure now. should this usage be supported? In think unserialize is an horrible hack which should have never be used. This is described in UPGRADINGS First, try to fix the code, using newInstanceWithoutConstructor() when available (5.4+) and possible (userland classes) Remi. --- ## #75636 — Yasuo Ohgaki — 2014-07-17T05:34:12+00:00 URL: https://externals.io/message/75633#75636 · In reply to: #75633 Hi Laruence, > /** > * Creates a new instance of the mapped class, without invoking > the constructor. > * > * @return object > */ > public function newInstance() > { > if ($this->_prototype === null) { > $this->_prototype = unserialize(sprintf('O:%d:"%s":0:{}', > strlen($this->name), $this->name)); > } > > return clone $this->_prototype; > } > Just curious. This code is trying to initialize object with __wakeup() method rather than __construct(). Do you know what kind of object is initialized? What's the reason why it needs to use __wakeup() or avoid __construct()? If the usage is valid, we may try to find solutions. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #75637 — Laruence — 2014-07-17T05:56:33+00:00 URL: https://externals.io/message/75633#75637 · In reply to: #75635 > Le 17/07/2014 06:01, Laruence a écrit : > > > […] > > > […] > > In think unserialize is an horrible hack which should have never be used. > > This is described in UPGRADINGS > > First, try to fix the code, using newInstanceWithoutConstructor() when > available (5.4+) and possible (userland classes) > Hey: serialize maybe a sensitive area, in my opinion we should very careful about the BC issue in such area.. if a user want to migrate it's codes from 5.5 to 5.6, but he has multi servers, he might want to migrate them one by one. in such case, the serialized data could be shared by 5.5 and 5.6... so... thanks > Remi. > > > > -- > > > -- Laruence Xinchen Hui http://www.laruence.com/ --- ## #75638 — Stas Malyshev — 2014-07-17T06:08:27+00:00 URL: https://externals.io/message/75633#75638 · In reply to: #75637 Hi! > in such case, the serialized data could be shared by 5.5 and 5.6... This is true, but what you presented is not serialized data. Serialized data will be fine. But the code you shown instead tries to use serializer as a roundabout way of instantiating objects. This is not the right thing to do, this was never the purpose of the serializer and was never guaranteed to work. -- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/ --- ## #75642 — Zeev Suraski — 2014-07-17T06:20:59+00:00 URL: https://externals.io/message/75633#75642 · In reply to: #75638 > -----Original Message----- > From: Stas Malyshev [mailto:smalyshev@sugarcrm.com] > Sent: Thursday, July 17, 2014 9:08 AM > To: Laruence; Remi Collet > Cc: PHP Internals > Subject: Re: [PHP-DEV] An BC issue in unserialize > > Hi! > > > […] > > This is true, but what you presented is not serialized data. Serialized > data will > be fine. But the code you shown instead tries to use serializer as a > roundabout > way of instantiating objects. This is not the right thing to do, this was > never the > purpose of the serializer and was never guaranteed to work. My key concern is that I've now bumped into two applications - both OroCRM and also Taskada - that work fine with 5.5 but stop working with 5.6 because of that issue. Both are Symfony apps so it might be something that can be solved centrally in Symfony, but it may also be just coincidence. Either way, it's an indication that this would make upgrading to 5.6 harder than it needs to be, and a key tenet of our yearly release cycle was guaranteed pain-free upgrades. IMHO this isn't something we should change in a 2nd digit release, but on a major version, even if it wasn't documented. Zeev --- ## #75645 — Stas Malyshev — 2014-07-17T06:43:41+00:00 URL: https://externals.io/message/75633#75645 · In reply to: #75642 Hi! > IMHO this isn't something we should change in a 2nd digit release, but on a > major version, even if it wasn't documented. We don't have much option here. Keeping it leads to a remote triggerable segfaults. We've discussed this here just recently. This is a hack that does not work properly with internal classes, and should not work too - the whole reason C: was created is because O: can not work with such classes. -- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/ --- ## #75658 — Marco Pivetta — 2014-07-17T07:46:42+00:00 URL: https://externals.io/message/75633#75658 · In reply to: #75645 On Thu, Jul 17, 2014 at 8:43 AM, Stas Malyshev wrote: > Hi! > > > […] > on a > > […] > > We don't have much option here. Keeping it leads to a remote triggerable > segfaults. We've discussed this here just recently. This is a hack that > does not work properly with internal classes, and should not work too - > the whole reason C: was created is because O: can not work with such > classes. > As discussed in previous threads about this failure, we (doctrine) can move away from the `unserialize()` hack if `ReflectionClass#newInstanceWithoutConstructor()` provides support for internal classes. It doesn't need to cover ALL internal classes, just the most commonly extended ones. Marco Pivetta http://twitter.com/Ocramius http://ocramius.github.com/ --- ## #75659 — Stas Malyshev — 2014-07-17T07:58:23+00:00 URL: https://externals.io/message/75633#75659 · In reply to: #75658 Hi! > As discussed in previous threads about this failure, we (doctrine) can > move away from the `unserialize()` hack if > `ReflectionClass#newInstanceWithoutConstructor()` provides support for > internal classes. Could you explain why it is needed to instantiate internal classes without calling the ctor? I'd like to understand the use case more. > It doesn't need to cover ALL internal classes, just the most commonly > extended ones. The problem is we do not know which of these classes may fail if instantiated without initializing. With PHP classes, it's easy since the engine takes care of the basic plumbing. With C classes, if you don't call the function, you get nulls or worse, garbage, in places where values are expected. -- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/ --- ## #75660 — Marco Pivetta — 2014-07-17T08:22:42+00:00 URL: https://externals.io/message/75633#75660 · In reply to: #75659 On Thu, Jul 17, 2014 at 9:58 AM, Stas Malyshev wrote: > Hi! > > > […] > > Could you explain why it is needed to instantiate internal classes > without calling the ctor? I'd like to understand the use case more. > I honestly don't have a use-case for it myself, and I also strongly disagree with extending internal PHP classes due to their fragility. What I know is that many Doctrine users extend `ArrayObject` in entities, which is what caused us to apply a hotfix only for 5.4.29 and 5.5.13, and NOT 5.6.0. > > […] > > The problem is we do not know which of these classes may fail if > instantiated without initializing. With PHP classes, it's easy since the > engine takes care of the basic plumbing. With C classes, if you don't > call the function, you get nulls or worse, garbage, in places where > values are expected. Yes, that indeed. What I am suggesting is to provide a small map of internal PHP classes that are supported by `ReflectionClass#newInstanceWithoutConstructor()`, so that their internal data can be initialized even if the userland constructor is not invoked. Marco Pivetta http://twitter.com/Ocramius http://ocramius.github.com/