On Tue, Jul 22, 2014 at 6:20 PM, Julien Pauli <jpa...@php.net> wrote:
> On Tue, Jul 22, 2014 at 2:04 PM, Ferenc Kovacs <tyr...@gmail.com> wrote: > > > > > > > > On Tue, Jul 22, 2014 at 1:48 PM, Nikita Popov <nikita....@gmail.com> > wrote: > >> > >> On Tue, Jul 22, 2014 at 12:27 PM, Ferenc Kovacs <tyr...@gmail.com> > wrote: > >>> > >>> sorry for the late reply. > >>> you are right and after looking into the implementation I think classes > >>> having custom object storage (eg. create_object) shouldn't assume that > >>> their __construct will be called, but either do the initialization in > the > >>> create_object hook or validate in the other methods that the object was > >>> properly initialized. > >>> given that this lack of initialization problem is already present(you > can > >>> extend such a class and have a __construct() in the subclass which > >>> doesn't > >>> call the parent constructor), and we want to keep the unserialize trick > >>> fixed (as that exposes this problems to the remote attacker when some > >>> code > >>> accepts arbitrary serialized data from the client) I see no reason to > >>> keep > >>> the limitation in the ReflectionClass::newInstanceWithoutConstructor() > >>> and > >>> allowing the instantiation of internal classes will provide a clean > >>> upgrade > >>> path to doctrine and co. > >>> ofc. we have to fix the internal classes misusing the constructor for > >>> proper initialization one by one, but that can happen after the initial > >>> 5.6.0 release (ofc it would be wonderful if people could lend me a hand > >>> for > >>> fixing as much as we can before the release), but we have to fix those > >>> anyways. > >> > >> > >> This sounds reasonable to me. newInstanceWithoutConstructor does not > have > >> the same remote exploitation concerns as serialize, so allowing crashes > here > >> that can also be caused by other means seems okay to me (especially if > we're > >> planning to fix them lateron). Only additional restriction I'd add is to > >> disallow calling it on an internal + final class. For those the > __construct > >> argument does not apply. For them the entity-extending-internal-class > >> usecase doesn't apply either, so that shouldn't be a problem. > >> > >> Nikita > >> > > > > Thanks for the prompt reply! > > I was considering mentioning the final constructors, but as we previously > > didn't checked that and from a quick look it seems that we are mostly > using > > it as an easy/cheap way to make sure that the object is initialized > properly > > (which could also happen when a subclass calls parent::__construct() from > > it's constructor) which isn't exactly the best usecase for final. > > But I don't really mind having that check. > > I'm +1 also with the idea. > > Just take care to have a clone_handler defined as well, as the default > clone handler doesn't call create_object. > http://lxr.php.net/xref/PHP_5_5/Zend/zend_objects.c#218 > > Julien > thanks, I will keep that in mind when we start moving the initialization from the constructors into the create_object functions. I've also went ahead and created a pull request for the proposed changes: https://github.com/php/php-src/pull/733 as you can see I've taken Nikita's advice and internal classes with final constructors are still not allowed to be instantiated. -- Ferenc Kovács @Tyr43l - http://tyrael.hu
