php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Bug #69732 can induce segmentation fault with basic php code
Submitted: 2015-05-29 16:16 UTC Modified: 2015-05-31 11:10 UTC
From: will dot o dot west at gmail dot com Assigned: dmitry
Status: Closed Package: Reproducible crash
PHP Version: 5.5.25 OS: OSX / Ubuntu Trusty
Private report: No CVE-ID:
 [2015-05-29 16:16 UTC] will dot o dot west at gmail dot com
Description:
------------
The following snippet is a minimal reproduction of an segv found in a WordPress site, wherein a theme was stashing data in an undeclared field of the WP_Query object, which has a __get but no __set. Its not clear whether this worked as intended in any prior versions of php.

Test script:
---------------
<?php
 
class wpq {
    private $unreferenced;
 
    public function __get($name) {
        return $this->$name;
    }
}
 
function ret_assoc() {
    return array('foo' => 'bar');
}
 
$wpq = new wpq;
$wpq->interesting =& ret_assoc();
$x = $wpq->interesting;
printf("%s\n", $x);


Expected result:
----------------
likely printing an empty string

Actual result:
--------------
Segmentation fault: 11

Patches

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2015-05-30 01:27 UTC] requinix@php.net
The segfault is because of infinite recursion: if ->foo does not exist then __get(foo) will be called, which tries to do ->foo, which does not exist so __get(foo) will be called... until PHP dies.
__get() should never blindly do a ->$name for this reason. If anything it should manually test for the presence of the property, like with get_object_vars(), before attempting to return it.

If there is a bug here it's because $wpq->interesting =& ret_assoc() does not behave like a regular assignment (given that ret_assoc() isn't returning by-reference), creating the property on $wpq. If you dump $wpq after this line you'll see it doesn't have a $interesting property - just $unreferenced.

Making ret_assoc() return by-ref results in a fatal error "Cannot assign by reference to overloaded object". I too don't think that should be a problem if __set is not implemented and assignment should behave normally (ie, creating the property).
 [2015-05-31 11:10 UTC] laruence@php.net
-Assigned To: +Assigned To: dmitry
 [2015-05-31 11:10 UTC] laruence@php.net
hmm, it's not a stack overflow segfault..

it's a double free, freeing the op1 of ZEND_ASSIGN.

a simple fix could be(but it break some tests, since they are not sharing one zval anymore):

$ git diff
diff --git a/Zend/zend_execute.c b/Zend/zend_execute.c
index ac00c28..0eff680 100644
--- a/Zend/zend_execute.c
+++ b/Zend/zend_execute.c
@@ -896,13 +896,11 @@ static inline zval* zend_assign_to_variable(zval **variable_ptr_ptr, zval *value
 			if (UNEXPECTED(variable_ptr == value)) {
 				return variable_ptr;
 			} else if (EXPECTED(!PZVAL_IS_REF(value))) {
-				Z_ADDREF_P(value);
-				*variable_ptr_ptr = value;
-				ZEND_ASSERT(variable_ptr != &EG(uninitialized_zval));
-				GC_REMOVE_ZVAL_FROM_BUFFER(variable_ptr);
-				zval_dtor(variable_ptr);
-				efree(variable_ptr);
-				return value;
+				ZVAL_COPY_VALUE(&garbage, variable_ptr);
+				ZVAL_COPY_VALUE(variable_ptr, value);
+				zendi_zval_copy_ctor(*variable_ptr);
+				_zval_dtor_func(&garbage ZEND_FILE_LINE_CC);
+				return variable_ptr;
 			} else {
 				goto copy_value;
 			}
 [2015-06-01 09:22 UTC] dmitry@php.net
Automatic comment on behalf of dmitry@zend.com
Revision: http://git.php.net/?p=php-src.git;a=commit;h=9031a902e3393ff7dc8a02615430a7d894c740fa
Log: Fixed bug #69732 (can induce segmentation fault with basic php code).
 [2015-06-01 09:22 UTC] dmitry@php.net
-Status: Assigned +Status: Closed
 
PHP Copyright © 2001-2017 The PHP Group
All rights reserved.
Last updated: Thu Jun 22 22:01:38 2017 UTC