Re: Re: Problems with the fix for the BC break introduced in 5.4.29 and 5.5.13

On Fri, Jun 20, 2014 at 2:58 AM, Stas Malyshev <smalyshev@sugarcrm.com> wrote: > Hi! > > > Wouldn't it be much easier to just revert for 5.4 and 5.5 and then > > patching ReflectionClass#newInstanceWithoutConstructor() > > to just > > handle internal PHP classes as well? > > We can not just handle internal PHP classes without calling their ctor. > That's the whole problem - for internal class, if ctor is not called, > you can get segfault on anything you do with the class. > > > Is there a technical limitation for this? > > Are there security problems related to the patch that broke > > phpunit/doctrine? If not, can the patch simply be delayed and only > > applied to 5.6? > > Right now we know this thing can produce segfaults. But running code on > unititialized data may be worse than that. > > -- > Stanislav Malyshev, Software Architect > SugarCRM: http://www.sugarcrm.com/ > (408)454-6900 ext. 227 > I have a longer draft reply about the technical parts, but I wanted to send a quick headsup: The original SplFileObject segfault can be achived without any unserialize trick, one can simply extend an internal class depending on it's constructor for providing the required initial state, and simply not call the parent::__construct() from the child: http://3v4l.org/fqFC6 And I also think that we can't fix/remove the O:X:"Y":0:{} trick without providing some usable alternatives for phpunit/doctrine(not providing it would really hurt the adoption imo). So at the moment I see two options: 1. Revert back to the original behavior for 5.4 and 5.5, and for 5.6 introduce the original clean fix (classes implementing Serializable can only be unserialized with the "C:" format) and remove the restriction from ReflectionClass::newInstanceWithoutConstructor about internal classes (and make sure that we put up a warning in the docs about the potential instability of internal classes instanitated this way. 2. Do the same as suggested for 5.6 in 1) but also for 5.4 and 5.5. I think the first one is better. If we would keep the current status, it would mean for the userland project that: - for 5.6 - they should use ReflectionClass::newInstanceWithoutConstructor for non-internal classes(using clunky checks like in https://github.com/sebastianbergmann/phpunit-mock-objects/commit/4a338e464a94d3e5a48ae28292e98e9b4e3ac898 ) - they should need to use the unserialize O:X:"Y":0:{} trick for internal(including non-internal classes extending internal classes) classes not implementing Serializable - they should use unserialize C:X:"Y":0:{} for internal classes implementing Serializable and because there are classes denying the unserialization or requiring specific unserialize payload (which we can't expect the userland to keep track of) they would have to check if the unserialize fails and throw and exception, making the instanitation of some classes outright impossible. - for 5.4 and 5.5 - they should use ReflectionClass::newInstanceWithoutConstructor for non-internal classes(using clunky checks like in https://github.com/sebastianbergmann/phpunit-mock-objects/commit/4a338e464a94d3e5a48ae28292e98e9b4e3ac898 ) - they should need to use the unserialize O:X:"Y":0:{} trick for the internal classes not implementing Serializable and for non-internal classes extending internal classes implementing Serializable - they should use unserialize C:X:"Y":0:{} for internal classes implementing Serializable and because there are classes denying the unserialization or requiring specific unserialize payload (which we can't expect the userland to keep track of) they would have to check if the unserialize fails and throw and exception, making the instanitation of some classes outright impossible. (and both phpunit and doctrine 2 supports 5.3 where ReflectionClass::newInstanceWithoutConstructor isn't available) -- Ferenc Kovács @Tyr43l - http://tyrael.hu