From 2543e61aed67add7522e0b4cdf9a13cf3e441f6f Mon Sep 17 00:00:00 2001 From: Nikita Popov Date: Fri, 22 Jun 2018 12:58:48 +0200 Subject: [PATCH] Fixed bug #76509 In PHP static properties are shared between inheriting classes, unless they are explicitly overwritten. However, because this functionality was implemented using reference, it was possible to break the implementation by reassigning the static property reference. This is fixed by switching the implementation from using references to using INDIRECTs, which cannot be affected by userland code. --- NEWS | 2 ++ UPGRADING | 17 +++++++++++ Zend/zend_API.c | 17 +++-------- Zend/zend_builtin_functions.c | 1 + Zend/zend_inheritance.c | 29 +++++++++--------- Zend/zend_object_handlers.c | 5 ++-- Zend/zend_types.h | 6 ++++ ext/opcache/zend_accelerator_util_funcs.c | 36 ++++++++++++++++------- ext/opcache/zend_file_cache.c | 23 ++++++++++----- ext/opcache/zend_persist.c | 9 ++++-- ext/opcache/zend_persist_calc.c | 4 ++- ext/reflection/php_reflection.c | 3 ++ tests/classes/static_properties_004.phpt | 10 +++---- 13 files changed, 105 insertions(+), 57 deletions(-) diff --git a/NEWS b/NEWS index 0232c33b271..13407f953d7 100644 --- a/NEWS +++ b/NEWS @@ -7,6 +7,8 @@ PHP NEWS (Nikita) . Fixed bug #76502 (Chain of mixed exceptions and errors does not serialize properly). (Nikita) + . Fixed bug #76509 (Inherited static properties can be desynchronized from + their parent by ref). (Nikita) - FPM: . Fixed bug #73342 (Vulnerability in php-fpm by changing stdin to diff --git a/UPGRADING b/UPGRADING index 6b5b267a07e..2607cc29185 100644 --- a/UPGRADING +++ b/UPGRADING @@ -44,6 +44,23 @@ Core: the following "FOO;" will cause a syntax error. This issue can always be resolved by choosing an ending label that does not occur within the contents of the string. + . In PHP, static properties are shared between inheriting classes, unless the + static property is explicitly overridden in a child class. However, due to + an implementation artifact it was possible to separate the static properties + by assigning a reference. This loophole has been fixed. + + class Test { + public static $x = 0; + } + class Test2 extends Test { } + + Test2::$x = &$x; + $x = 1; + + var_dump(Test::$x, Test2::$x); + // Previously: int(0), int(1) + // Now: int(1), int(1) + . References returned by array and property accesses are now unwrapped as part of the access. This means that it is no longer possible to modify the reference between the access and the use of the accessed value: diff --git a/Zend/zend_API.c b/Zend/zend_API.c index ab12fed9ad3..a4e501217e7 100644 --- a/Zend/zend_API.c +++ b/Zend/zend_API.c @@ -1164,19 +1164,10 @@ ZEND_API int zend_update_class_constants(zend_class_entry *class_type) /* {{{ */ #endif for (i = 0; i < class_type->default_static_members_count; i++) { p = &class_type->default_static_members_table[i]; - if (Z_ISREF_P(p)) { - if (class_type->parent && - i < class_type->parent->default_static_members_count && - p == &class_type->parent->default_static_members_table[i] && - Z_TYPE(CE_STATIC_MEMBERS(class_type->parent)[i]) != IS_UNDEF - ) { - zval *q = &CE_STATIC_MEMBERS(class_type->parent)[i]; - - ZVAL_NEW_REF(q, q); - ZVAL_COPY(&CE_STATIC_MEMBERS(class_type)[i], q); - } else { - ZVAL_COPY_OR_DUP(&CE_STATIC_MEMBERS(class_type)[i], Z_REFVAL_P(p)); - } + if (Z_TYPE_P(p) == IS_INDIRECT) { + zval *q = &CE_STATIC_MEMBERS(class_type->parent)[i]; + ZVAL_DEINDIRECT(q); + ZVAL_INDIRECT(&CE_STATIC_MEMBERS(class_type)[i], q); } else { ZVAL_COPY_OR_DUP(&CE_STATIC_MEMBERS(class_type)[i], p); } diff --git a/Zend/zend_builtin_functions.c b/Zend/zend_builtin_functions.c index e4d3b2df5c7..591c8af17f6 100644 --- a/Zend/zend_builtin_functions.c +++ b/Zend/zend_builtin_functions.c @@ -1071,6 +1071,7 @@ static void add_class_vars(zend_class_entry *scope, zend_class_entry *ce, int st prop = NULL; if (statics && (prop_info->flags & ZEND_ACC_STATIC) != 0) { prop = &ce->default_static_members_table[prop_info->offset]; + ZVAL_DEINDIRECT(prop); } else if (!statics && (prop_info->flags & ZEND_ACC_STATIC) == 0) { prop = &ce->default_properties_table[OBJ_PROP_TO_NUM(prop_info->offset)]; } diff --git a/Zend/zend_inheritance.c b/Zend/zend_inheritance.c index 95746e68de6..67e2b97d3eb 100644 --- a/Zend/zend_inheritance.c +++ b/Zend/zend_inheritance.c @@ -890,25 +890,23 @@ ZEND_API void zend_do_inheritance(zend_class_entry *ce, zend_class_entry *parent do { dst--; src--; - if (Z_ISREF_P(src)) { - Z_ADDREF_P(src); + if (Z_TYPE_P(src) == IS_INDIRECT) { + ZVAL_INDIRECT(dst, Z_INDIRECT_P(src)); } else { - ZVAL_MAKE_REF_EX(src, 2); + ZVAL_INDIRECT(dst, src); } - ZVAL_REF(dst, Z_REF_P(src)); } while (dst != end); } else if (ce->type == ZEND_USER_CLASS) { src = parent_ce->default_static_members_table + parent_ce->default_static_members_count; do { dst--; src--; - if (Z_ISREF_P(src)) { - Z_ADDREF_P(src); + if (Z_TYPE_P(src) == IS_INDIRECT) { + ZVAL_INDIRECT(dst, Z_INDIRECT_P(src)); } else { - ZVAL_MAKE_REF_EX(src, 2); + ZVAL_INDIRECT(dst, src); } - ZVAL_REF(dst, Z_REF_P(src)); - if (Z_TYPE_P(Z_REFVAL_P(dst)) == IS_CONSTANT_AST) { + if (Z_TYPE_P(Z_INDIRECT_P(dst)) == IS_CONSTANT_AST) { ce->ce_flags &= ~ZEND_ACC_CONSTANTS_UPDATED; } } while (dst != end); @@ -917,11 +915,11 @@ ZEND_API void zend_do_inheritance(zend_class_entry *ce, zend_class_entry *parent do { dst--; src--; - if (!Z_ISREF_P(src)) { - ZVAL_NEW_PERSISTENT_REF(src, src); + if (Z_TYPE_P(src) == IS_INDIRECT) { + ZVAL_INDIRECT(dst, Z_INDIRECT_P(src)); + } else { + ZVAL_INDIRECT(dst, src); } - ZVAL_COPY_VALUE(dst, src); - Z_ADDREF_P(dst); } while (dst != end); } ce->default_static_members_count += parent_ce->default_static_members_count; @@ -1605,8 +1603,8 @@ static void zend_do_traits_property_binding(zend_class_entry *ce) /* {{{ */ if (flags & ZEND_ACC_STATIC) { op1 = &ce->default_static_members_table[coliding_prop->offset]; op2 = &ce->traits[i]->default_static_members_table[property_info->offset]; - ZVAL_DEREF(op1); - ZVAL_DEREF(op2); + ZVAL_DEINDIRECT(op1); + ZVAL_DEINDIRECT(op2); } else { op1 = &ce->default_properties_table[OBJ_PROP_TO_NUM(coliding_prop->offset)]; op2 = &ce->traits[i]->default_properties_table[OBJ_PROP_TO_NUM(property_info->offset)]; @@ -1651,6 +1649,7 @@ static void zend_do_traits_property_binding(zend_class_entry *ce) /* {{{ */ /* property not found, so lets add it */ if (flags & ZEND_ACC_STATIC) { prop_value = &ce->traits[i]->default_static_members_table[property_info->offset]; + ZEND_ASSERT(Z_TYPE_P(prop_value) != IS_INDIRECT); } else { prop_value = &ce->traits[i]->default_properties_table[OBJ_PROP_TO_NUM(property_info->offset)]; } diff --git a/Zend/zend_object_handlers.c b/Zend/zend_object_handlers.c index 1148944a189..0af48993e96 100644 --- a/Zend/zend_object_handlers.c +++ b/Zend/zend_object_handlers.c @@ -1445,7 +1445,6 @@ ZEND_API zval *zend_std_get_static_property(zend_class_entry *ce, zend_string *p return NULL; } } - ret = CE_STATIC_MEMBERS(ce) + property_info->offset; /* check if static properties were destoyed */ if (UNEXPECTED(CE_STATIC_MEMBERS(ce) == NULL)) { @@ -1453,9 +1452,11 @@ undeclared_property: if (!silent) { zend_throw_error(NULL, "Access to undeclared static property: %s::$%s", ZSTR_VAL(ce->name), ZSTR_VAL(property_name)); } - ret = NULL; + return NULL; } + ret = CE_STATIC_MEMBERS(ce) + property_info->offset; + ZVAL_DEINDIRECT(ret); return ret; } /* }}} */ diff --git a/Zend/zend_types.h b/Zend/zend_types.h index 8e6a21d052b..59b0af1a4a8 100644 --- a/Zend/zend_types.h +++ b/Zend/zend_types.h @@ -1101,6 +1101,12 @@ static zend_always_inline uint32_t zval_delref_p(zval* pz) { } \ } while (0) +#define ZVAL_DEINDIRECT(z) do { \ + if (Z_TYPE_P(z) == IS_INDIRECT) { \ + (z) = Z_INDIRECT_P(z); \ + } \ + } while (0) + #define ZVAL_OPT_DEREF(z) do { \ if (UNEXPECTED(Z_OPT_ISREF_P(z))) { \ (z) = Z_REFVAL_P(z); \ diff --git a/ext/opcache/zend_accelerator_util_funcs.c b/ext/opcache/zend_accelerator_util_funcs.c index 1fc8c5a8b96..a91a2e80e7f 100644 --- a/ext/opcache/zend_accelerator_util_funcs.c +++ b/ext/opcache/zend_accelerator_util_funcs.c @@ -346,6 +346,10 @@ static void zend_class_copy_ctor(zend_class_entry **pce) *pce = ce = ARENA_REALLOC(old_ce); ce->refcount = 1; + if (ce->parent) { + ce->parent = ARENA_REALLOC(ce->parent); + } + if (old_ce->default_properties_table) { ce->default_properties_table = emalloc(sizeof(zval) * old_ce->default_properties_count); src = old_ce->default_properties_table; @@ -361,13 +365,29 @@ static void zend_class_copy_ctor(zend_class_entry **pce) /* static members */ if (old_ce->default_static_members_table) { + int i, end; + zend_class_entry *parent = ce->parent; + ce->default_static_members_table = emalloc(sizeof(zval) * old_ce->default_static_members_count); - src = old_ce->default_static_members_table; - end = src + old_ce->default_static_members_count; - dst = ce->default_static_members_table; - for (; src != end; src++, dst++) { - ZVAL_COPY_VALUE(dst, src); - zend_clone_zval(dst); + i = ce->default_static_members_count; + + /* Copy static properties in this class */ + end = parent ? parent->default_static_members_count : 0; + for (; i >= end; i--) { + zval *p = &ce->default_static_members_table[i]; + ZVAL_COPY_VALUE(p, &old_ce->default_static_members_table[i]); + zend_clone_zval(p); + } + + /* Create indirections to static properties from parent classes */ + while (parent && parent->default_static_members_table) { + end = parent->parent ? parent->parent->default_static_members_count : 0; + for (; i >= end; i--) { + zval *p = &ce->default_static_members_table[i]; + ZVAL_INDIRECT(p, &parent->default_static_members_table[i]); + } + + parent = parent->parent; } } ce->static_members_table = ce->default_static_members_table; @@ -386,10 +406,6 @@ static void zend_class_copy_ctor(zend_class_entry **pce) ce->interfaces = NULL; } - if (ce->parent) { - ce->parent = ARENA_REALLOC(ce->parent); - } - zend_update_inherited_handler(constructor); zend_update_inherited_handler(destructor); zend_update_inherited_handler(clone); diff --git a/ext/opcache/zend_file_cache.c b/ext/opcache/zend_file_cache.c index 05ec90ad2f5..36576d892ed 100644 --- a/ext/opcache/zend_file_cache.c +++ b/ext/opcache/zend_file_cache.c @@ -612,12 +612,16 @@ static void zend_file_cache_serialize_class(zval *zv, } } if (ce->default_static_members_table) { - zval *p, *end; + zval *table, *p, *end; SERIALIZE_PTR(ce->default_static_members_table); - p = ce->default_static_members_table; - UNSERIALIZE_PTR(p); - end = p + ce->default_static_members_count; + table = ce->default_static_members_table; + UNSERIALIZE_PTR(table); + + /* Serialize only static properties in this class. + * Static properties from parent classes will be handled in class_copy_ctor */ + p = table + (ce->parent ? ce->parent->default_static_members_count : 0); + end = table + ce->default_static_members_count; while (p < end) { zend_file_cache_serialize_zval(p, script, info, buf); p++; @@ -1225,6 +1229,7 @@ static void zend_file_cache_unserialize_class(zval *zv, ce = Z_PTR_P(zv); UNSERIALIZE_STR(ce->name); + UNSERIALIZE_PTR(ce->parent); zend_file_cache_unserialize_hash(&ce->function_table, script, buf, zend_file_cache_unserialize_func, ZEND_FUNCTION_DTOR); if (ce->default_properties_table) { @@ -1239,11 +1244,14 @@ static void zend_file_cache_unserialize_class(zval *zv, } } if (ce->default_static_members_table) { - zval *p, *end; + zval *table, *p, *end; + /* Unserialize only static properties in this class. + * Static properties from parent classes will be handled in class_copy_ctor */ UNSERIALIZE_PTR(ce->default_static_members_table); - p = ce->default_static_members_table; - end = p + ce->default_static_members_count; + table = ce->default_static_members_table; + p = table + (ce->parent ? ce->parent->default_static_members_count : 0); + end = table + ce->default_static_members_count; while (p < end) { zend_file_cache_unserialize_zval(p, script, buf); p++; @@ -1326,7 +1334,6 @@ static void zend_file_cache_unserialize_class(zval *zv, } } - UNSERIALIZE_PTR(ce->parent); UNSERIALIZE_PTR(ce->constructor); UNSERIALIZE_PTR(ce->destructor); UNSERIALIZE_PTR(ce->clone); diff --git a/ext/opcache/zend_persist.c b/ext/opcache/zend_persist.c index fd5defdf234..d26286c33d0 100644 --- a/ext/opcache/zend_persist.c +++ b/ext/opcache/zend_persist.c @@ -720,10 +720,13 @@ static void zend_persist_class_entry(zval *zv) } } if (ce->default_static_members_table) { - int i; - + int i; zend_accel_store(ce->default_static_members_table, sizeof(zval) * ce->default_static_members_count); - for (i = 0; i < ce->default_static_members_count; i++) { + + /* Persist only static properties in this class. + * Static properties from parent classes will be handled in class_copy_ctor */ + i = ce->parent ? ce->parent->default_static_members_count : 0; + for (; i < ce->default_static_members_count; i++) { zend_persist_zval(&ce->default_static_members_table[i]); } } diff --git a/ext/opcache/zend_persist_calc.c b/ext/opcache/zend_persist_calc.c index 6feedc6e6f1..bc8955e6e32 100644 --- a/ext/opcache/zend_persist_calc.c +++ b/ext/opcache/zend_persist_calc.c @@ -326,7 +326,9 @@ static void zend_persist_class_entry_calc(zval *zv) ADD_SIZE(sizeof(zval) * ce->default_static_members_count); for (i = 0; i < ce->default_static_members_count; i++) { - zend_persist_zval_calc(&ce->default_static_members_table[i]); + if (Z_TYPE(ce->default_static_members_table[i]) != IS_INDIRECT) { + zend_persist_zval_calc(&ce->default_static_members_table[i]); + } } } zend_hash_persist_calc(&ce->constants_table, zend_persist_class_constant_calc); diff --git a/ext/reflection/php_reflection.c b/ext/reflection/php_reflection.c index 99fd4a7959c..b816106e255 100644 --- a/ext/reflection/php_reflection.c +++ b/ext/reflection/php_reflection.c @@ -3795,6 +3795,7 @@ static void add_class_vars(zend_class_entry *ce, int statics, zval *return_value prop = NULL; if (statics && (prop_info->flags & ZEND_ACC_STATIC) != 0) { prop = &ce->default_static_members_table[prop_info->offset]; + ZVAL_DEINDIRECT(prop); } else if (!statics && (prop_info->flags & ZEND_ACC_STATIC) == 0) { prop = &ce->default_properties_table[OBJ_PROP_TO_NUM(prop_info->offset)]; } @@ -5503,6 +5504,7 @@ ZEND_METHOD(reflection_property, getValue) return; } member_p = &CE_STATIC_MEMBERS(intern->ce)[ref->prop.offset]; + ZVAL_DEINDIRECT(member_p); ZVAL_DEREF(member_p); ZVAL_COPY(return_value, member_p); } else { @@ -5570,6 +5572,7 @@ ZEND_METHOD(reflection_property, setValue) return; } variable_ptr = &CE_STATIC_MEMBERS(intern->ce)[ref->prop.offset]; + ZVAL_DEINDIRECT(variable_ptr); if (variable_ptr != value) { zval garbage; diff --git a/tests/classes/static_properties_004.phpt b/tests/classes/static_properties_004.phpt index 7d5363f9fc4..79bbc6380ba 100644 --- a/tests/classes/static_properties_004.phpt +++ b/tests/classes/static_properties_004.phpt @@ -1,5 +1,5 @@ --TEST-- -Inherited static properties can be separated from their reference set. +Inherited static properties cannot be separated from their reference set. --FILE--