Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:75013
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain sugarcrm.com designates 108.166.43.67 as permitted sender)
Message-ID: <53A386E1.8040303@sugarcrm.com>
Date: Thu, 19 Jun 2014 17:57:05 -0700
Organization: SugarCRM
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:24.0) Gecko/20100101 Thunderbird/24.6.0
MIME-Version: 1.0
To: Ferenc Kovacs <tyra3l@gmail.com>
CC: Julien Pauli <jpauli@php.net>, Remi Collet <remi@fedoraproject.org>, 
 PHP Internals <internals@lists.php.net>
References: <CAH-PCH6xEXspisLOHtiTbViA4vDTk7q6LM3t_Bh9Dzr1cj_Nag@mail.gmail.com>	<53A1C722.9060501@fedoraproject.org>	<53A21137.6010705@sugarcrm.com>	<CAMUwpuT_S2EE6cdJM95RzY4vmORGx3vJ9+gau8G3EZCMsNSydA@mail.gmail.com>	<53A2A9BD.1070603@sugarcrm.com> <CAH-PCH5-vtCVvGvceMqt6rDXq4zo0Vb34cyDK3KdBUNfg_oLyg@mail.gmail.com>
In-Reply-To: <CAH-PCH5-vtCVvGvceMqt6rDXq4zo0Vb34cyDK3KdBUNfg_oLyg@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Subject: Re: [PHP-DEV] Re: Problems with the fix for the BC break introduced
 in 5.4.29 and 5.5.13
From: smalyshev@sugarcrm.com (Stas Malyshev)

Hi!

> 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.

Ah, I was missing this part. I thought those objects go through C, but
now I see they can go through O also. This means we can't really ban
internal classes in O:. But can we see if the serializer is
zend_user_serialize (in which case it's probably OK to allow internal
class or user class) or some other function (in which case allowing
internal class is probably not a good idea)?
Do we still have problems or segfaults in this case?

> 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.

Some classes just don't allow being serialized/unserialized at all, and
use serialize handlers to do that. Trying to run them through
unserialize in a roundabout way is very dangerous since they would
assume nobody does that.

> 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.

I guess this is where I'm trying to get - banning unserializing of the
classes which are unsafe (i.e. O: in internal class while having
serialize handler) while not banning cases that we can allow. So I
wonder here if additional check for zend_user_serialize would help?
-- 
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227