Re: [PHP-DEV] Problems with the fix for the BC break introduced in 5.4.29 and 5.5.13
Hi! What I found *not safe*, throwing unwanted warnings hitting an undesired behavior, or even segfaulting, and thus needing patch : - Dom - Mysqli - sqlite3 (sqlite3stmt class segfaults) I've fixed all the instances of problematic behavior with no ctor that I could catch now. I could not reproduce problems in DOM or mysqli, so it'd be nice to have reproductions there. Otherwise, I think the core modules are fine in my tests. -- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/ -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Problems with the fix for the BC break introduced in 5.4.29 and 5.5.13
On Tue, Jul 29, 2014 at 9:48 AM, Stas Malyshev smalys...@sugarcrm.com wrote: Hi! What I found *not safe*, throwing unwanted warnings hitting an undesired behavior, or even segfaulting, and thus needing patch : - Dom - Mysqli - sqlite3 (sqlite3stmt class segfaults) I've fixed all the instances of problematic behavior with no ctor that I could catch now. I could not reproduce problems in DOM or mysqli, so it'd be nice to have reproductions there. Otherwise, I think the core modules are fine in my tests. Mmmm, I can't reproduce segfaults. I can only hit the check messages that check the instance is correctly constructed. When sqlite and SPL is patched, can we say we are safe about this issue ? For SPL, I have no deep knowledge about it, I couldn't find any bad behavior from my tests, all seemed to check the state, but I did not perform full 100% coverage tests. Thanks. Julien
Re: [PHP-DEV] Problems with the fix for the BC break introduced in 5.4.29 and 5.5.13
Hi!
It would be good to have a section in UPGRADING.INTERNALS explaining
in details what should be done, very important for non core extensions
(pecl or other repositories).
Probably a good idea but I'm not sure what exactly to write there,
besides initialize everything, check everything :) There are many
places (esp. in SPL, but not only) where checks are present in some
methods, but not others, or some values are checked while others do not,
there are also places where some checks produce errors and some
exceptions, sometimes even within one class. We don't need to clean that
up right now but it's a good idea to keep in mind we need to.
There's also a more tricky problem since many clone functions don't do
the right thing too. E.g. this script:
class MyPDO extends PDO { function __construct() {} }
$pdo = new MyPDO();
$pdo2 = clone $pdo;
$pdo2-query(123);
Produces a segfault, even though without clone it works fine. I wonder
if one overrides __clone wouldn't more things break.
I wrote a simple fuzzer to test classes, here:
https://gist.github.com/smalyshev/f4d0c1502fa2411478ff
Note: it runs random functions with random arguments, so don't run it
anywhere it could hurt real data.
--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Problems with the fix for the BC break introduced in 5.4.29 and 5.5.13
On Sat, Jul 26, 2014 at 1:26 AM, Stas Malyshev smalys...@sugarcrm.com wrote: Hi! yeah, that would work ofc, but as these libs seems to have instanitate arbitrary classes, that would require either generating files on the fly and including them or simply evaling them, but of those are a bit dirtier than using Reflection for the same job. True but that's what phpunit, etc. are doing for mocks anyway, aren't they? true, but it can also be used to argue for loosening the restriction, why restrict something from Reflection, which is already possible from simple class extension. I agree, probably makes sense to allow it if you can do it anyway. Reflection is not something you can trigger without explicit codding (unlike O: thing) so it's fine with me. a nice thing from OOP POV and also will cause problems if/when we introduce a reflection method removing final from classes/methods (this was already proposed not that long ago with a working patch but was turned down because other reasons). That probably wouldn't be a good idea, especially for internal classes. Like I said in a previous mail, only Dom, Mysqli and sqlite3 need patch to be able to work with only create_object invoked. I'm not sure about COM as I can't setup a test environment for it. I recently started patching mysqli. I think it is feasable to have all of them patched so that newInstanceWithoutConstructor() may be safely opened to internal classes, for 5.6.0. A quick word as well to say I'm against giving the opportunity to userland to remove the final attribute, as this will lead to lots of bugs, some of them not even fixable I think. Julien.Pauli
Re: [PHP-DEV] Problems with the fix for the BC break introduced in 5.4.29 and 5.5.13
On Sat, Jul 26, 2014 at 9:55 PM, Julien Pauli jpa...@php.net wrote: On Sat, Jul 26, 2014 at 1:26 AM, Stas Malyshev smalys...@sugarcrm.com wrote: Hi! yeah, that would work ofc, but as these libs seems to have instanitate arbitrary classes, that would require either generating files on the fly and including them or simply evaling them, but of those are a bit dirtier than using Reflection for the same job. True but that's what phpunit, etc. are doing for mocks anyway, aren't they? true, but it can also be used to argue for loosening the restriction, why restrict something from Reflection, which is already possible from simple class extension. I agree, probably makes sense to allow it if you can do it anyway. Reflection is not something you can trigger without explicit codding (unlike O: thing) so it's fine with me. a nice thing from OOP POV and also will cause problems if/when we introduce a reflection method removing final from classes/methods (this was already proposed not that long ago with a working patch but was turned down because other reasons). That probably wouldn't be a good idea, especially for internal classes. Like I said in a previous mail, only Dom, Mysqli and sqlite3 need patch to be able to work with only create_object invoked. I'm not sure about COM as I can't setup a test environment for it. I recently started patching mysqli. I think it is feasable to have all of them patched so that newInstanceWithoutConstructor() may be safely opened to internal classes, for 5.6.0. that would be nice. A quick word as well to say I'm against giving the opportunity to userland to remove the final attribute, as this will lead to lots of bugs, some of them not even fixable I think. just to clarify: I didn't suggested to introduce this method (it was proposed by somebody else in the past), just mentioned that I don't think that turning classes/constructors into final only to be safe from these kind of problems is a bad idea imo, and one of my arguments was that there is a chance that we would like to allow userland to override final, and it would cause problems anyways. -- Ferenc Kovács @Tyr43l - http://tyrael.hu
Re: [PHP-DEV] Problems with the fix for the BC break introduced in 5.4.29 and 5.5.13
On Sat, Jul 26, 2014 at 1:26 AM, Stas Malyshev smalys...@sugarcrm.com wrote: Hi! yeah, that would work ofc, but as these libs seems to have instanitate arbitrary classes, that would require either generating files on the fly and including them or simply evaling them, but of those are a bit dirtier than using Reflection for the same job. True but that's what phpunit, etc. are doing for mocks anyway, aren't they? nope, some mock frameworks do that (afair atoum for example) but phpunit(phpunit-mock-objects to be more precise) was using the serialize trick and newInstanceWithoutConstructor() ( https://github.com/sebastianbergmann/phpunit-mock-objects/blob/42f29d650744fd7ce785e7a4dd3598a09b0bfd2b/src/Framework/MockObject/Generator.php#L273), and now they switched to Instantiator from Ocramius(he also participated in this thread) which also uses the unserialize trick and newInstanceWithoutConstructor() ( https://github.com/Ocramius/Instantiator/blob/86e562799ceb87316fcb70e151551f47dd215a0f/src/Instantiator/Instantiator.php#L80 ). true, but it can also be used to argue for loosening the restriction, why restrict something from Reflection, which is already possible from simple class extension. I agree, probably makes sense to allow it if you can do it anyway. Reflection is not something you can trigger without explicit codding (unlike O: thing) so it's fine with me. yeah, exactly. a nice thing from OOP POV and also will cause problems if/when we introduce a reflection method removing final from classes/methods (this was already proposed not that long ago with a working patch but was turned down because other reasons). That probably wouldn't be a good idea, especially for internal classes. I should have left out this argument, because that has it's own can of worms, but if anybody interested in the original proposal: http://grokbase.com/t/php/php-internals/121gmxkz1c/php-dev-reflection-to-remove-final I wasn't proposing merging this in my mail, just argued that if this proposal ever merged, that would either require fixing up the finals anyways or it would be possible to trigger similar segfaults and stuff. -- Ferenc Kovács @Tyr43l - http://tyrael.hu
Re: [PHP-DEV] Problems with the fix for the BC break introduced in 5.4.29 and 5.5.13
Hi! nope, some mock frameworks do that (afair atoum for example) but phpunit(phpunit-mock-objects to be more precise) was using the serialize trick and newInstanceWithoutConstructor() (https://github.com/sebastianbergmann/phpunit-mock-objects/blob/42f29d650744fd7ce785e7a4dd3598a09b0bfd2b/src/Framework/MockObject/Generator.php#L273), It does that too. But at least in version 3 it was also generating and eval'ing classes, in PHPUnit_Framework_MockObject_Generator. Looking here: https://github.com/sebastianbergmann/phpunit-mock-objects/tree/master/src/Framework/MockObject It is still doing class generation and evals. Not sure why it doesn't also override the ctor, maybe it was for some implementation consideration. -- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/ -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Problems with the fix for the BC break introduced in 5.4.29 and 5.5.13
Hi! What I found *not safe*, throwing unwanted warnings hitting an undesired behavior, or even segfaulting, and thus needing patch : - Dom - Mysqli - sqlite3 (sqlite3stmt class segfaults) I'm testing a patch for sqlite3 right now and will commit it shortly, but I could not reproduce anything weird with DOM. What class/method crashes for you there? -- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/ -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Problems with the fix for the BC break introduced in 5.4.29 and 5.5.13
Hi! What I found *safe*, with checkers that check object is properly initialized, so not needing patch, but throwing exceptions or warnings when used bad constructed : - SPL SPL unfortunately is not safe at all - a lot of iterator classes segfault on no-ctor initialization. I'll make a patch for them. -- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/ -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Problems with the fix for the BC break introduced in 5.4.29 and 5.5.13
hi, On Sun, Jul 27, 2014 at 1:59 AM, Stas Malyshev smalys...@sugarcrm.com wrote: Hi! What I found *safe*, with checkers that check object is properly initialized, so not needing patch, but throwing exceptions or warnings when used bad constructed : - SPL SPL unfortunately is not safe at all - a lot of iterator classes segfault on no-ctor initialization. I'll make a patch for them. It would be good to have a section in UPGRADING.INTERNALS explaining in details what should be done, very important for non core extensions (pecl or other repositories). Cheers, -- Pierre @pierrejoye | http://www.libgd.org -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Problems with the fix for the BC break introduced in 5.4.29 and 5.5.13
Hi!
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.
I think we should ensure the objects will be in safe (i.e. no-crashing)
state following create_object even if ctor was not called. The question
is if we can ensure that and what would be the effort.
As for upgrade path for doctrine, etc. - if we're talking about
something like test mocking, wouldn't something like:
class Mock_Stub_FooClass extends FooClass {
function __construct() {}
}
$foo = new Mock_Stub_FooClass();
solve the problem? I understand it's not argument against
newInstanceWithoutConstructor as essentially it does the same, just
saying newInstanceWithoutConstructor strictly speaking would not be
necessary?
I think an approach for this would be to take pull 733 and make a script
(or a test) that takes all the classes we define internally (may be a
bit hard since some extensions need external libs, but we can check
those manually) and instantiate them using this (and all other
non-standard) method and try to call random methods and see if we get
any crashes. But we should be reasonably sure at least most common ones
would not crash if we enable this.
Another question would be if objects like COM would react to this
properly. But if not it would be possible to make them final too.
--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Problems with the fix for the BC break introduced in 5.4.29 and 5.5.13
On Sat, Jul 26, 2014 at 12:29 AM, Stas Malyshev smalys...@sugarcrm.com
wrote:
Hi!
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.
I think we should ensure the objects will be in safe (i.e. no-crashing)
state following create_object even if ctor was not called. The question
is if we can ensure that and what would be the effort.
we already discussed this via moving the init logic to the create/clone
handlers from __construct for such classes.
As for upgrade path for doctrine, etc. - if we're talking about
something like test mocking, wouldn't something like:
class Mock_Stub_FooClass extends FooClass {
function __construct() {}
}
$foo = new Mock_Stub_FooClass();
yeah, that would work ofc, but as these libs seems to have instanitate
arbitrary classes, that would require either generating files on the fly
and including them or simply evaling them, but of those are a bit dirtier
than using Reflection for the same job.
solve the problem? I understand it's not argument against
newInstanceWithoutConstructor as essentially it does the same, just
saying newInstanceWithoutConstructor strictly speaking would not be
necessary?
true, but it can also be used to argue for loosening the restriction, why
restrict something from Reflection, which is already possible from simple
class extension.
I think an approach for this would be to take pull 733 and make a script
(or a test) that takes all the classes we define internally (may be a
bit hard since some extensions need external libs, but we can check
those manually) and instantiate them using this (and all other
non-standard) method and try to call random methods and see if we get
any crashes.
Julien already went over the potential classes and did the same thing
(calling methods over them), the list of affected classes is just the
previous one in this thread.
But we should be reasonably sure at least most common ones
would not crash if we enable this.
this change wouldn't introduce any crash, which isn't already possible
through a subclass not calling parent::__construct, ofc. if we can fix the
crashes before the final release that's nice, but I wouldn't say it is
mandatory, how it was hidden for years(and even so that the remote trigger
from unserialize is fixed).
Another question would be if objects like COM would react to this
properly. But if not it would be possible to make them final too.
yeah, but I would rather fix them than move to final, as I mentioned
before, it seems that some/most of the internal classes with final
constructor are final so that this problem can't occur, but this doesn't a
nice thing from OOP POV and also will cause problems if/when we introduce a
reflection method removing final from classes/methods (this was already
proposed not that long ago with a working patch but was turned down because
other reasons).
--
Ferenc Kovács
@Tyr43l - http://tyrael.hu
Re: [PHP-DEV] Problems with the fix for the BC break introduced in 5.4.29 and 5.5.13
On Wed, Jul 23, 2014 at 7:58 AM, Ferenc Kovacs tyr...@gmail.com wrote: 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. When should we start patching those ? I guess asap ? Julien -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Problems with the fix for the BC break introduced in 5.4.29 and 5.5.13
On Wed, Jul 23, 2014 at 10:57 AM, Julien Pauli jpa...@php.net wrote: On Wed, Jul 23, 2014 at 7:58 AM, Ferenc Kovacs tyr...@gmail.com wrote: 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. When should we start patching those ? I guess asap ? Julien Not sure, on one hand, I would be glad seeing these fixed, but on the other hand I'm a little bit worried about introducing destabilizing changes this close to the release, and these problems existed for years (triggerable either through instantiating via the unserialize O: trick or through a subclass) without any reports before https://bugs.php.net/bug.php?id=67072 -- Ferenc Kovács @Tyr43l - http://tyrael.hu
Re: [PHP-DEV] Problems with the fix for the BC break introduced in 5.4.29 and 5.5.13
On Wed, Jul 23, 2014 at 11:09 AM, Ferenc Kovacs tyr...@gmail.com wrote: On Wed, Jul 23, 2014 at 10:57 AM, Julien Pauli jpa...@php.net wrote: On Wed, Jul 23, 2014 at 7:58 AM, Ferenc Kovacs tyr...@gmail.com wrote: 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. When should we start patching those ? I guess asap ? Julien Not sure, on one hand, I would be glad seeing these fixed, but on the other hand I'm a little bit worried about introducing destabilizing changes this close to the release, and these problems existed for years (triggerable either through instantiating via the unserialize O: trick or through a subclass) without any reports before https://bugs.php.net/bug.php?id=67072 Agree. We could start on a case by case basis, knowing we still got at least one RC to try that. Then anyway we're gonna fix them into next 5.6 revisions, so ... Julien.P -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Problems with the fix for the BC break introduced in 5.4.29 and 5.5.13
On Wed, Jul 23, 2014 at 11:12 AM, Julien Pauli jpa...@php.net wrote: On Wed, Jul 23, 2014 at 11:09 AM, Ferenc Kovacs tyr...@gmail.com wrote: On Wed, Jul 23, 2014 at 10:57 AM, Julien Pauli jpa...@php.net wrote: On Wed, Jul 23, 2014 at 7:58 AM, Ferenc Kovacs tyr...@gmail.com wrote: 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. When should we start patching those ? I guess asap ? Julien Not sure, on one hand, I would be glad seeing these fixed, but on the other hand I'm a little bit worried about introducing destabilizing changes this close to the release, and these problems existed for years (triggerable either through instantiating via the unserialize O: trick or through a subclass) without any reports before https://bugs.php.net/bug.php?id=67072 Agree. We could start on a case by case basis, knowing we still got at least one RC to try that. Then anyway we're gonna fix them into next 5.6 revisions, so ... Julien.P I spent some time today reviewing our internal bundled extensions searching the ones that overwrite create_object *and* have a dangerous __construct constructing internal object data. What I found *not safe*, throwing unwanted warnings hitting an undesired behavior, or even segfaulting, and thus needing patch : - Dom - Mysqli - sqlite3 (sqlite3stmt class segfaults) What I found *safe*, with checkers that check object is properly initialized, so not needing patch, but throwing exceptions or warnings when used bad constructed : - Date - fileinfo - Intl - Pdo - Reflection - SimpleXML* (not faulting, but could need a better implementation of the checks and the thrown error messages) - SPL Any extension not present in one of the two above lists is safe. The method I used is rather
Re: [PHP-DEV] Problems with the fix for the BC break introduced in 5.4.29 and 5.5.13
On Mon, Jun 30, 2014 at 8:10 PM, Alexey Zakhlestin indey...@gmail.com wrote: On 30 Jun 2014, at 11:10, Stas Malyshev smalys...@sugarcrm.com wrote: I think we should move away from the practice of using serializer for something it was never made for, namely a weird way of instantiating classes. Serializer should be working only with serialized data. Now, the question is can we instantiate the internal class without calling its ctor, and the answer here would probably be no, at least not safely. While in the case of user class the engine can be reasonable sure even if you don't call the ctor the basic structures are initialized properly, in the case of the internal class all bets are off. I'm not sure yet which use cases require ctor not to be called, but I'm not sure how we can deliver on internal classes here. Sorry, I’m a bit late to discussion and might be missing something obvious. As far as I remember, internal classes have 2 constructors. 1. implementation of __construct() function 2. create_object hook It should be possible to keep unsafe stuff in create_object which is called unconditionally and leave safe initialisation in __construct. so, in case when __construct is not called object will have properties initialised with nulls, empty strings, etc. I understand that is a lot of work on case-by-case basis but it is doable. 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. I consider this problem the biggest impediment for releasing the 5.6.0 final, so I would really like to hear what do you guys think about my proposed solution? -- Ferenc Kovács @Tyr43l - http://tyrael.hu
Re: [PHP-DEV] Problems with the fix for the BC break introduced in 5.4.29 and 5.5.13
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
Re: [PHP-DEV] Problems with the fix for the BC break introduced in 5.4.29 and 5.5.13
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. -- Ferenc Kovács @Tyr43l - http://tyrael.hu
Re: [PHP-DEV] Problems with the fix for the BC break introduced in 5.4.29 and 5.5.13
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
Re: [PHP-DEV] Problems with the fix for the BC break introduced in 5.4.29 and 5.5.13
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
