Re: Bug 67072 resolution for 5.4/5.5

On Mon, Jun 23, 2014 at 10:07 AM, Ferenc Kovacs <tyra3l@gmail.com> wrote: > > > > On Mon, Jun 23, 2014 at 9:54 AM, Julien Pauli <jpauli@php.net> wrote: >> >> On Mon, Jun 23, 2014 at 2:20 AM, Stas Malyshev <smalyshev@sugarcrm.com> >> wrote: >> > Hi! >> > >> >> for the issue to materialize you need to feed hand-crafted input to >> >> unserialize, >> > >> > True. >> > >> >> anybody doing that with user controlled data already asking >> >> for problems, >> > >> > True in theory, in practice this is widely and commonly done. >> > >> >> I prefer this over what we have in 5.4/5.5 and given how few classes >> >> does 1, actually mean, I think it would be an acceptable compromise, >> >> but >> >> let's hear what others think. >> > >> > Cool, waiting for others to chime in. >> > >> >> ps: I've seen that you created a pull request with the patch, if >> >> somebody don't wanna copypaste the patch from the mail, here it is: >> >> https://github.com/php/php-src/pull/701 >> > >> > Yes, thanks for quoting it, it seems to be green on Travis and phpunit >> > also seems to work fine with it. I also added a unit tests with a couple >> > of cases to see how it's supposed to work. >> > >> > -- >> > Stanislav Malyshev, Software Architect >> > SugarCRM: http://www.sugarcrm.com/ >> > (408)454-6900 ext. 227 >> >> >> Hello, >> >> I find the compromise nice. >> The goal is to have something barely working in most use cases for 5.4 >> and 5.5, and prepare something nicer and stronger for 5.6. >> >> So, the proposed patch ( Stas' ) is nice for this, as comon tools still >> work. >> >> I'm also ok for the 5.6 statements : >> - Disalow O: for classes with custom serializer >> - Unlock newInstanceArgWithoutConstructor() for internal classes >> >> Note that unlocking newInstanceArgWithoutConstructor() for internal >> classes may require lot of work. >> Remi already tried to patch some extensions for them to work AFAIR. > > > and maybe not even possible to fix all those cases, yet we already have the > same problem with: > MyClass extends InternalClassDependingOnConstructor { > public function __construct(){ > //not calling parent::__construct > } > } > > so that shouldn't be a blocker for enabling internal classes for > newInstanceWithoutConstructor > but I would discuss this separately/later, as the 5.4/5.5 decision/fix is a > bit more urgent. Yes, for 5.4 and 5.5 , Stas' patch looks right to me. Julien