php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Bug #64196 __clone recursion stack overflow
Submitted: 2013-02-12 14:27 UTC Modified: 2013-02-12 21:39 UTC
Votes:3
Avg. Score:4.7 ± 0.5
Reproduced:2 of 3 (66.7%)
Same Version:1 (50.0%)
Same OS:1 (50.0%)
From: krakjoe@php.net Assigned:
Status: Open Package: Reproducible crash
PHP Version: Irrelevant OS: Any
Private report: No CVE-ID: None
Have you experienced this issue?
Rate the importance of this bug to you:

 [2013-02-12 14:27 UTC] krakjoe@php.net
Description:
------------
This patch avoids stack overflows where recursion is present in __clone.

Test script:
---------------
<?php
class Cloneable {
	public function __clone(){
		return clone $this;
	}
}

$c = new Cloneable();
$a = clone $c;
?>

Expected result:
----------------
Stack overflow

Actual result:
--------------
E_ERROR

Patches

__clone-2.patch (last revision 2013-02-12 23:12 UTC) by krakjoe@php.net)
__clone.patch (last revision 2013-02-12 14:29 UTC) by pthreads at pthreads dot org)

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2013-02-12 14:45 UTC] nikic@php.net
zend_object.guards is for property guards. Wouldn't you be clashing with the guard for $__clone here?

Also, I'm not convinced we need to add this check at all. Recursion is a valid means of programming and as long as there is some termination condition everything's okay. Arguably with "clone" recursion makes rather little sense, but as it stands now we are open to recursion everywhere and I don't think we should go down the patch of saying "recursion is okay here, but it's not okay here". I mean, "include" for example can also be used recursively, even though you might argue that that's nearly as useless. Should we be adding checks everywhere, where we think recursion makes too little sense? I don't think so.

The only (calls) that currently use recursion guarding are __get/__set and friends. For those it makes sense because the recursion guarding gives access to the underlying property, so it has some actual function (rather than just forbidding a programming pattern).
 [2013-02-12 20:03 UTC] krakjoe@php.net
The following patch has been added/updated:

Patch Name: __clone-2.patch
Revision:   1360699422
URL:        https://bugs.php.net/patch-display.php?bug=64196&patch=__clone-2.patch&revision=1360699422
 [2013-02-12 20:05 UTC] krakjoe@php.net
__clone-2.patch addresses the clash ... and ensures proper functionality in all 
situations, not just basic examples.

we don't seem to be able to agree on whether such checks should be made, but at 
least now there are no clashes and the patch is correct ...
 [2013-02-12 21:39 UTC] nikic@php.net
Not sure in what way the new patch resolves the clash. Doesn't it just move it from "$foo->__clone" towards "$foo->{'$__clone'}"?
 [2013-02-12 23:12 UTC] krakjoe@php.net
The following patch has been added/updated:

Patch Name: __clone-2.patch
Revision:   1360710727
URL:        https://bugs.php.net/patch-display.php?bug=64196&patch=__clone-2.patch&revision=1360710727
 
PHP Copyright © 2001-2017 The PHP Group
All rights reserved.
Last updated: Sun Nov 19 01:31:42 2017 UTC