Newsgroups: php.internals Path: news.php.net Xref: news.php.net php.internals:75837 Return-Path: Mailing-List: contact internals-help@lists.php.net; run by ezmlm Delivered-To: mailing list internals@lists.php.net Received: (qmail 71611 invoked from network); 22 Jul 2014 12:04:56 -0000 Received: from unknown (HELO lists.php.net) (127.0.0.1) by localhost with SMTP; 22 Jul 2014 12:04:56 -0000 Authentication-Results: pb1.pair.com smtp.mail=tyra3l@gmail.com; spf=pass; sender-id=pass Authentication-Results: pb1.pair.com header.from=tyra3l@gmail.com; sender-id=pass Received-SPF: pass (pb1.pair.com: domain gmail.com designates 209.85.216.45 as permitted sender) X-PHP-List-Original-Sender: tyra3l@gmail.com X-Host-Fingerprint: 209.85.216.45 mail-qa0-f45.google.com Received: from [209.85.216.45] ([209.85.216.45:35056] helo=mail-qa0-f45.google.com) by pb1.pair.com (ecelerity 2.1.1.9-wez r(12769M)) with ESMTP id 02/BE-14611-7635EC35 for ; Tue, 22 Jul 2014 08:04:55 -0400 Received: by mail-qa0-f45.google.com with SMTP id cm18so6294423qab.18 for ; Tue, 22 Jul 2014 05:04:52 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type; bh=V01LJ/LDtEas+FlOdRVl5+N+d5H5EkIaSEU00lrSZnA=; b=gVRoAU/ymHD4FP2lSUNKXBoFGVG0NYEe8AV4dUEfcufTaFVrrG6Y7gAELLzgCNyHqh s3Rtz9P33jkg23A+twOQjvdt3KrWb9Z0VyZ8T2ggzZQG6bremPeeEevC6yb1Um73iuhe Xs47r2wcC5iqrtodQjaXKrPmQn13WnI77dwZDyoWqNHX0yStWOhw8C00ubrcfmoDPaPX Bwl4NpFkSt1J9HOnoYGx0PvjBAiOw6Zv8jKluCZaZxchtXnly9QtDADkNd8aOl9sHXIa xl4GZPuTcgiBh458hMgQhVJZYdWhpkHURWV03GouQcWp7uPkgAUb971wM9CEMYFUm9zU Dv+Q== MIME-Version: 1.0 X-Received: by 10.224.96.137 with SMTP id h9mr54679184qan.96.1406030692709; Tue, 22 Jul 2014 05:04:52 -0700 (PDT) Received: by 10.140.102.111 with HTTP; Tue, 22 Jul 2014 05:04:52 -0700 (PDT) In-Reply-To: References: <53A1C722.9060501@fedoraproject.org> <53A21137.6010705@sugarcrm.com> <53A2A9BD.1070603@sugarcrm.com> <53A3874E.20704@sugarcrm.com> <53A62AFF.4080302@sugarcrm.com> <53B10D59.4060206@sugarcrm.com> Date: Tue, 22 Jul 2014 14:04:52 +0200 Message-ID: To: Nikita Popov Cc: Alexey Zakhlestin , Stas Malyshev , Marco Pivetta , Sebastian Bergmann , Julien Pauli , Remi Collet , PHP Internals Content-Type: multipart/alternative; boundary=001a11c2e0a8fb7cd504fec7070d Subject: Re: [PHP-DEV] Problems with the fix for the BC break introduced in 5.4.29 and 5.5.13 From: tyra3l@gmail.com (Ferenc Kovacs) --001a11c2e0a8fb7cd504fec7070d Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On Tue, Jul 22, 2014 at 1:48 PM, Nikita Popov wrote: > On Tue, Jul 22, 2014 at 12:27 PM, Ferenc Kovacs 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 th= e >> 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 ca= n >> 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 co= de >> accepts arbitrary serialized data from the client) I see no reason to ke= ep >> the limitation in the ReflectionClass::newInstanceWithoutConstructor() a= nd >> 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. --=20 Ferenc Kov=C3=A1cs @Tyr43l - http://tyrael.hu --001a11c2e0a8fb7cd504fec7070d--