can induce segmentation fault with basic php code

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 (profile)
Status:	Closed	Package:	Reproducible crash
PHP Version:	5.5.25	OS:	OSX / Ubuntu Trusty
Private report:	No	CVE-ID:	None

[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

Pull Requests

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

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

-Status: Assigned +Status: Closed