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

On Thu, Jun 19, 2014 at 11:13 AM, Stas Malyshev <smalyshev@sugarcrm.com> wrote: > Hi! > > > For me, it's a safe solution, however, it breaks BC, I think it's a > > no-go for 5.4 and 5.5. > > It breaks BC in something that never was part of the official API, never > was promised to work and works only by accident for internal classes. > Each such object can segfault at smallest provocation, so keeping them > is essentially requiring that we keep segfault compatibility. I don't > think we should promise that. > > > - Revert the BC break in 5.5 and 5.4 > > - Keep the segfault, we've been living with it for ages > > That's not the reason not to fix segfaults. Virtually every bug we're > fixing in the code we have been "living with for ages". That's not the > reason to keep them now that we know it segfaults. > > > - Patch the manual to clearly show one should never try to unserialize > > hand-made strings : we just do not support such behavior (thus, it > > could lead to segfaults) > > Well, here we contradict ourselves then - if we do not support such > behavior, why we are taking so much effort to enable it? Because > declaring that changing something even in small part is inacceptable > even if the price is known crashes in the application (and I wonder what > security people can make of it - uninitialized objects might be way > worse than just null pointer deref...) - that looks a lot like > supporting to me. So in what meaning we don't support it then? > -- > Stanislav Malyshev, Software Architect > SugarCRM: http://www.sugarcrm.com/ > (408)454-6900 ext. 227 > > -- > PHP Internals - PHP Runtime Development Mailing List > To unsubscribe, visit: http://www.php.net/unsub.php > > I've linked the discussion and provide a really compact summary (I think it is too compact) at https://bugs.php.net/bug.php?id=67072 I'm a bit indecisive about the current situation, because I think that we should fix the root cause of these problems, but I'm also wanna make sure that we don't totally criple phpunit-mock-objects and doctrine with a micro release. I think it would be safe to prohibit the unserialization of classes implementing the Serializable interface with the "O:" format, userland should use the "C:" format for these classes, we don't have that many classes outright denying the unserialization, and if a class validates the incoming data in the unserialize method, then the userland should make sure to provide appropriate data for the validation to pass. (Ofc. we should also make sure that we don't require more data, than necessary, see https://bugs.php.net/bug.php?id=67453&edit=1) Another topic would be to that internal classes (not implementing Serializable) can still be instantiated without the constructor call through the unserialize method (using the "O:" format), and we can't prohibit that, as it is/can be perfectly normal to unserialize an internal class previously properly instantiated and serialize, but we don't have the knowledge in the class to tell what is a properly serialized string, and what isn't for that given class. (Which means that allowing the internal classes to be instantiated via ReflectionClass::newInstanceWithoutConstructor() doesn't really introduces a new set of problems, it would just allow to shut yourself with reflection instead of an obscure unserialize trick. (Not saying we should do this, just mentioning that the problem already exists). I think that it would be a nice if we could add more validation for the internal classes __wakeup()/unserialize() methods for data which presence is mandatory for the class to be able to work, which would ofc. bother the life of the phpunit/doctrine users/devs, but it would only prevent the mocking those classes which would be unstable/dangerous previously when instantiated without the constructor call. -- Ferenc Kovács @Tyr43l - http://tyrael.hu