Skip to content

Commit

Permalink
Don't overload get_properties for ArrayObject
Browse files Browse the repository at this point in the history
Instead overload get_properties_for for a few specific cases such
as array casts. This resolves the issue where ArrayObject
get_properties may violate engine invariants in some cases.
  • Loading branch information
nikic committed Oct 10, 2018
1 parent 1270e50 commit a5fa51a
Show file tree
Hide file tree
Showing 6 changed files with 105 additions and 56 deletions.
6 changes: 0 additions & 6 deletions ext/reflection/tests/bug61388.phpt
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,6 @@ print_r($reflObj->getProperties(ReflectionProperty::IS_PUBLIC));
--EXPECT--
Array
(
[0] => ReflectionProperty Object
(
[name] => test
[class] => ArrayObject
)

)
Array
(
Expand Down
36 changes: 29 additions & 7 deletions ext/spl/spl_array.c
Original file line number Diff line number Diff line change
Expand Up @@ -807,18 +807,40 @@ SPL_METHOD(Array, getArrayCopy)
RETURN_ARR(zend_array_dup(spl_array_get_hash_table(intern)));
} /* }}} */

static HashTable *spl_array_get_properties(zval *object) /* {{{ */
static HashTable *spl_array_get_properties_for(zval *object, zend_prop_purpose purpose) /* {{{ */
{
spl_array_object *intern = Z_SPLARRAY_P(object);
HashTable *ht;
zend_bool dup;

if (intern->ar_flags & SPL_ARRAY_STD_PROP_LIST) {
if (!intern->std.properties) {
rebuild_object_properties(&intern->std);
}
return intern->std.properties;
return zend_std_get_properties_for(object, purpose);
}

/* We are supposed to be the only owner of the internal hashtable.
* The "dup" flag decides whether this is a "long-term" use where
* we need to duplicate, or a "temporary" one, where we can expect
* that no operations on the ArrayObject will be performed in the
* meantime. */
switch (purpose) {
case ZEND_PROP_PURPOSE_ARRAY_CAST:
dup = 1;
break;
case ZEND_PROP_PURPOSE_VAR_EXPORT:
case ZEND_PROP_PURPOSE_JSON:
dup = 0;
break;
default:
return zend_std_get_properties_for(object, purpose);
}

return spl_array_get_hash_table(intern);
ht = spl_array_get_hash_table(intern);
if (dup) {
ht = zend_array_dup(ht);
} else {
GC_ADDREF(ht);
}
return ht;
} /* }}} */

static HashTable* spl_array_get_debug_info(zval *obj, int *is_temp) /* {{{ */
Expand Down Expand Up @@ -2013,7 +2035,7 @@ PHP_MINIT_FUNCTION(spl_array)
spl_handler_ArrayObject.has_dimension = spl_array_has_dimension;
spl_handler_ArrayObject.count_elements = spl_array_object_count_elements;

spl_handler_ArrayObject.get_properties = spl_array_get_properties;
spl_handler_ArrayObject.get_properties_for = spl_array_get_properties_for;
spl_handler_ArrayObject.get_debug_info = spl_array_get_debug_info;
spl_handler_ArrayObject.get_gc = spl_array_get_gc;
spl_handler_ArrayObject.read_property = spl_array_read_property;
Expand Down
30 changes: 30 additions & 0 deletions ext/spl/tests/ArrayObject_get_object_vars.phpt
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
--TEST--
get_object_vars() on ArrayObject works on the properties of the ArrayObject itself
--FILE--
<?php

class Test {
public $test;
public $test2;
}

class AO extends ArrayObject {
private $test;

public function getObjectVars() {
return get_object_vars($this);
}
}

$ao = new AO(new Test);
var_dump(get_object_vars($ao));
var_dump($ao->getObjectVars());

?>
--EXPECT--
array(0) {
}
array(1) {
["test"]=>
NULL
}
72 changes: 44 additions & 28 deletions ext/spl/tests/array_017.phpt
Original file line number Diff line number Diff line change
Expand Up @@ -139,13 +139,17 @@ array(3) {
["Flags"]=>
int(0)
["OVars"]=>
array(3) {
[0]=>
int(1)
["a"]=>
int(25)
array(5) {
["pub1"]=>
int(42)
int(1)
["pro1"]=>
int(2)
["pri1"]=>
int(3)
["imp1"]=>
int(4)
["dyn1"]=>
int(5)
}
["$this"]=>
object(ArrayObjectEx)#%d (6) {
Expand Down Expand Up @@ -178,13 +182,17 @@ array(3) {
["Flags"]=>
int(0)
["OVars"]=>
array(3) {
[0]=>
array(5) {
["pub2"]=>
int(1)
["a"]=>
int(25)
["pub1"]=>
int(42)
["pro2"]=>
int(2)
["pri2"]=>
int(3)
["imp2"]=>
int(4)
["dyn2"]=>
int(5)
}
["$this"]=>
object(ArrayIteratorEx)#%d (6) {
Expand Down Expand Up @@ -242,13 +250,17 @@ array(3) {
["Flags"]=>
int(0)
["OVars"]=>
array(3) {
[0]=>
array(5) {
["pub2"]=>
int(1)
["a"]=>
int(25)
["pub1"]=>
int(42)
["pro2"]=>
int(2)
["pri2"]=>
int(3)
["imp2"]=>
int(4)
["dyn2"]=>
int(5)
}
["$this"]=>
object(ArrayIteratorEx)#%d (6) {
Expand Down Expand Up @@ -541,14 +553,16 @@ array(3) {
["Flags"]=>
int(0)
["OVars"]=>
array(4) {
["pub1"]=>
array(5) {
["pub2"]=>
int(1)
["pro1"]=>
["pro2"]=>
int(2)
["imp1"]=>
["pri2"]=>
int(3)
["imp2"]=>
int(4)
["dyn1"]=>
["dyn2"]=>
int(5)
}
["$this"]=>
Expand Down Expand Up @@ -598,14 +612,16 @@ array(3) {
["Flags"]=>
int(0)
["OVars"]=>
array(4) {
["pub1"]=>
array(5) {
["pub2"]=>
int(1)
["pro1"]=>
["pro2"]=>
int(2)
["imp1"]=>
["pri2"]=>
int(3)
["imp2"]=>
int(4)
["dyn1"]=>
["dyn2"]=>
int(5)
}
["$this"]=>
Expand Down
4 changes: 2 additions & 2 deletions ext/spl/tests/bug61347.phpt
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ var_dump(isset($b['no_exists'])); //false
var_dump(empty($b['b'])); //true
var_dump(empty($b[37])); //true

var_dump(array_key_exists('b', $b)); //true
var_dump(array_key_exists('b', $b)); //false
var_dump($b['b']);

$a = array('b' => '', 37 => false);
Expand All @@ -31,7 +31,7 @@ bool(false)
bool(false)
bool(true)
bool(true)
bool(true)
bool(false)
NULL
bool(true)
bool(true)
Expand Down
13 changes: 0 additions & 13 deletions tests/classes/arrayobject_001.phpt

This file was deleted.

0 comments on commit a5fa51a

Please sign in to comment.