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 -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
