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

On Wed, Jun 18, 2014 at 12:53 AM, Stas Malyshev <smalyshev@sugarcrm.com> wrote: > Hi! > > > Could you check out my last mail about the unserialize stuff?: > > http://news.php.net/php.internals/74947 > > Reading this message, I gather the situation is as follows: > > 1. Original fix banned O: unserialization of classes that have custom > serializers. > Yeah, with https://github.com/php/php-src/commit/757da1eed5ffbc1c5a0ae07e82ae91f7e431f1a2 we introduced the Serializable interface, which provides a different method to serialize/unserialize an object. Since then, serialize/unserialize uses a different codepath for objects implementing this interface: __sleep()/__wakeup() is ignored, the unserialized format will use C: instead of O, and the format of the payload will be also a bit different: http://3v4l.org/B57Kk Even though we documented this from the userland point of view: http://www.php.net/Serializable Classes that implement this interface no longer support __sleep() and > __wakeup(). The method serialize is called whenever an instance needs to be > serialized. > ... > The above example will output something similar to: > string(38) "C:3:"obj":23:{s:15:"My private data";}" So while we never produced "O:" for objects implementing Serializable, we also never enforced/validated that, which made it possible to unserialize a Serializable via bypassing it's unserialize() method, and this also allowed to unserialize classes which explicitly prohibits that (zend_class_unserialize_deny). Userland devs could have used Reflection to check if the class they are trying to mock/whatever was implementing Serializable, but that would have required a Reflection call plus with the O: format you can always use the O:$classNameLength:"$className":0:{} for every class, while with the C: format, you have to make sure that your string passes the unserialize() call of that specific class, which makes it unfeasible for general purpose libraries like phpunit-mock-objects or doctrine2 entity manager. > 2. The following fix allowed this behavior for user classes. > yes > 3. The second fix means that if the user class descends from internal > class with serializer, we still have a problem. > correct > > My opinion is this: > > 1. Using unserialize() on anything that is not a result of serialize() > is a hack. As such, there are no support guarantees that it would work > in any particular manner, besides continuity of support for serialized > format itself. In particular, we have no guarantees that string that did > not come from serializer would behave in any particular way. > strongly agree > > 2. We should make reasonable effort to keep code that worked in version > x.y.z working in x.y.z+1. However, "reasonable" is important here, if > there's a behavior that is not documented and not wanted, we can break > it. We don't have to and will try not to, but we can. > +1, we don't really document the php serialize format (only show some examples), so I would consider it an internal implementation detail, and we indeed mentioned in the docs that classes implementing Serializable will have a different output, so I think that it isn't anything wrong with actually starting to enforce the documented behavior and validate and reject malformed serialized strings. > > 3. In light of this, I think we could do with current patch in 5.4 and > 5.5 (one that permits userland classes) but we should plug it completely > for 5.6. If somebody needs code that does whatever it did, it should be > in Reflection or someplace else, serializer should do serializing. But I > think for stable version it is a reasonable, if imperfect, compromise - > it would allow most common use cases (userland classes) to work and most > common crashes (internal classes) to not crash. Extending SplFileInfo > looks a bit more exotic so I think we can live with it not being fixed > till 5.6. > > I also agree that it is more important to make sure that internal objects are properly instantiated and not subject to random segfaults and probably other problems, than to allow an undocumented hack to be used for easier instantiation of objects. Back then when Sebastian proposed ReflectionClass::newInstanceWithoutConstructor() for 5.4, he himself introduced the only userland class restriction, because as he mentioned some internal classes would crash if instantiated without a constructor: http://marc.info/?l=php-internals&m=131426563418067&w=2 And it properly considers userland classes extending internal classes as internal, and prohibits the instantiation for such classes: http://3v4l.org/Zp4YS I agree that this would be big hit for some of those tools/libs, but other than fixing every internal class to be properly initialized without their constructors (which would be a nice thing anyways) I don't see any other way to fix the segfaults while keeping the ability to instantiate any class (be that an internal class, or a class implementing Serialize) with ease. -- Ferenc Kovács @Tyr43l - http://tyrael.hu