Skip to content

Fix static variable behavior with inheritance #6705

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 11 commits into from
Closed

Fix static variable behavior with inheritance #6705

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
db545b3
Fix static variable behavior in methods
nikic Feb 17, 2021
cbf9712
Add assertion that refcount is 1
nikic Feb 17, 2021
a0ab4ad
Fix Closure::bindTo()
nikic Feb 17, 2021
ac54035
Add UPGRADING note
nikic Feb 17, 2021
04c4d5a
Use zend_array_destroy() instead of zend_array_release()
nikic Feb 17, 2021
14fd6f8
Don't set static_variables_ptr if no static_variables
nikic Feb 17, 2021
394337c
Run extra test configurations
nikic Feb 17, 2021
574c79c
Remove unnecessary addref during preloading
nikic Feb 18, 2021
45ea188
Only allocate space for static_variables map ptr if used
nikic Feb 18, 2021
87158f1
Remove changes to zend_closures.c
nikic Feb 18, 2021
8e62b53
Use zend_array_destroy instead of zend_array_release in one more place
nikic Feb 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 1 addition & 4 deletions Zend/tests/anon/015.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,7 @@ var_dump($d->foo(24));
var_dump($c->foo());
?>
--EXPECT--
array(1) {
[0]=>
int(42)
}
NULL
array(1) {
[0]=>
int(24)
Expand Down
6 changes: 1 addition & 5 deletions Zend/tests/anon/016.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,7 @@ var_dump($d->foo(24));
var_dump($c->foo());
?>
--EXPECT--
array(1) {
[0]=>
object(stdClass)#2 (0) {
}
}
NULL
array(1) {
[0]=>
int(24)
Expand Down
4 changes: 1 addition & 3 deletions Zend/tests/method_static_var.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,6 @@ Initial value of static var in method depends on the include time of the class d
--FILE--
<?php

/* The current behavior is probably a bug, but we should still test how it currently works. */

class Foo {
public static function test() {
static $i = 0;
Expand All @@ -22,5 +20,5 @@ Bar::test();
--EXPECT--
int(1)
int(2)
int(1)
int(2)
int(3)
27 changes: 14 additions & 13 deletions Zend/zend_closures.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ typedef struct _zend_closure {
zend_function func;
zval this_ptr;
zend_class_entry *called_scope;
HashTable *static_variables;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand way we need this new "static_variables" and can't reuse "func.static_variables" and "static_variables_ptr".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the changes in this file turned out to be unnecessary. I've reverted them.

zif_handler orig_internal_handler;
} zend_closure;

Expand Down Expand Up @@ -516,6 +517,11 @@ static zend_object *zend_closure_clone(zend_object *zobject) /* {{{ */

zend_create_closure(&result, &closure->func,
closure->func.common.scope, closure->called_scope, &closure->this_ptr);
if (closure->static_variables) {
zend_closure *new_closure = (zend_closure *) Z_OBJ(result);
zend_array_destroy(new_closure->static_variables);
new_closure->static_variables = zend_array_dup(closure->static_variables);
}
return Z_OBJ(result);
}
/* }}} */
Expand Down Expand Up @@ -549,11 +555,9 @@ static HashTable *zend_closure_get_debug_info(zend_object *object, int *is_temp)

debug_info = zend_new_array(8);

if (closure->func.type == ZEND_USER_FUNCTION && closure->func.op_array.static_variables) {
if (closure->func.type == ZEND_USER_FUNCTION && closure->static_variables) {
zval *var;
HashTable *static_variables =
ZEND_MAP_PTR_GET(closure->func.op_array.static_variables_ptr);
ZVAL_ARR(&val, zend_array_dup(static_variables));
ZVAL_ARR(&val, zend_array_dup(closure->static_variables));
zend_hash_update(debug_info, ZSTR_KNOWN(ZEND_STR_STATIC), &val);
ZEND_HASH_FOREACH_VAL(Z_ARRVAL(val), var) {
if (Z_TYPE_P(var) == IS_CONSTANT_AST) {
Expand Down Expand Up @@ -615,8 +619,7 @@ static HashTable *zend_closure_get_gc(zend_object *obj, zval **table, int *n) /*

*table = Z_TYPE(closure->this_ptr) != IS_NULL ? &closure->this_ptr : NULL;
*n = Z_TYPE(closure->this_ptr) != IS_NULL ? 1 : 0;
return (closure->func.type == ZEND_USER_FUNCTION) ?
ZEND_MAP_PTR_GET(closure->func.op_array.static_variables_ptr) : NULL;
return closure->static_variables;
}
/* }}} */

Expand Down Expand Up @@ -680,11 +683,10 @@ ZEND_API void zend_create_closure(zval *res, zend_function *func, zend_class_ent
closure->func.common.fn_flags &= ~ZEND_ACC_IMMUTABLE;

if (closure->func.op_array.static_variables) {
closure->func.op_array.static_variables =
zend_array_dup(closure->func.op_array.static_variables);
ZEND_MAP_PTR_INIT(closure->func.op_array.static_variables_ptr,
&closure->static_variables);
closure->static_variables = zend_array_dup(closure->func.op_array.static_variables);
}
ZEND_MAP_PTR_INIT(closure->func.op_array.static_variables_ptr,
&closure->func.op_array.static_variables);

/* Runtime cache is scope-dependent, so we cannot reuse it if the scope changed */
if (!ZEND_MAP_PTR_GET(closure->func.op_array.run_time_cache)
Expand Down Expand Up @@ -768,15 +770,14 @@ ZEND_API void zend_create_fake_closure(zval *res, zend_function *func, zend_clas
void zend_closure_bind_var(zval *closure_zv, zend_string *var_name, zval *var) /* {{{ */
{
zend_closure *closure = (zend_closure *) Z_OBJ_P(closure_zv);
HashTable *static_variables = ZEND_MAP_PTR_GET(closure->func.op_array.static_variables_ptr);
zend_hash_update(static_variables, var_name, var);
zend_hash_update(closure->static_variables, var_name, var);
}
/* }}} */

void zend_closure_bind_var_ex(zval *closure_zv, uint32_t offset, zval *val) /* {{{ */
{
zend_closure *closure = (zend_closure *) Z_OBJ_P(closure_zv);
HashTable *static_variables = ZEND_MAP_PTR_GET(closure->func.op_array.static_variables_ptr);
HashTable *static_variables = closure->static_variables;
zval *var = (zval*)((char*)static_variables->arData + offset);
zval_ptr_dtor(var);
ZVAL_COPY_VALUE(var, val);
Expand Down
24 changes: 14 additions & 10 deletions Zend/zend_compile.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1029,28 +1029,32 @@ static uint32_t zend_add_try_element(uint32_t try_op) /* {{{ */
}
/* }}} */

void zend_init_static_variables_map_ptr(zend_op_array *op_array)
{
if (op_array->static_variables) {
ZEND_MAP_PTR_INIT(op_array->static_variables_ptr,
zend_arena_alloc(&CG(arena), sizeof(HashTable *)));
ZEND_MAP_PTR_SET(op_array->static_variables_ptr, NULL);
}
}

ZEND_API void function_add_ref(zend_function *function) /* {{{ */
{
if (function->type == ZEND_USER_FUNCTION) {
zend_op_array *op_array = &function->op_array;

if (op_array->refcount) {
(*op_array->refcount)++;
}
if (op_array->static_variables
&& !(GC_FLAGS(op_array->static_variables) & IS_ARRAY_IMMUTABLE)) {
GC_ADDREF(op_array->static_variables);
}

if (CG(compiler_options) & ZEND_COMPILE_PRELOAD) {
ZEND_ASSERT(op_array->fn_flags & ZEND_ACC_PRELOADED);
ZEND_MAP_PTR_NEW(op_array->run_time_cache);
ZEND_MAP_PTR_NEW(op_array->static_variables_ptr);
} else {
ZEND_MAP_PTR_INIT(op_array->static_variables_ptr, &op_array->static_variables);
ZEND_MAP_PTR_INIT(op_array->run_time_cache, zend_arena_alloc(&CG(arena), sizeof(void*)));
ZEND_MAP_PTR_INIT(op_array->run_time_cache, zend_arena_alloc(&CG(arena), sizeof(void *)));
ZEND_MAP_PTR_SET(op_array->run_time_cache, NULL);
}

zend_init_static_variables_map_ptr(op_array);
}

if (function->common.function_name) {
Expand Down Expand Up @@ -7021,9 +7025,8 @@ void zend_compile_func_decl(znode *result, zend_ast *ast, bool toplevel) /* {{{
if (CG(compiler_options) & ZEND_COMPILE_PRELOAD) {
op_array->fn_flags |= ZEND_ACC_PRELOADED;
ZEND_MAP_PTR_NEW(op_array->run_time_cache);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably, won't need special map_ptr handling for preloading now. I'll check this, after you merge the PR.

ZEND_MAP_PTR_NEW(op_array->static_variables_ptr);
} else {
ZEND_MAP_PTR_INIT(op_array->run_time_cache, zend_arena_alloc(&CG(arena), sizeof(void*)));
ZEND_MAP_PTR_INIT(op_array->run_time_cache, zend_arena_alloc(&CG(arena), sizeof(void *)));
ZEND_MAP_PTR_SET(op_array->run_time_cache, NULL);
}

Expand Down Expand Up @@ -7112,6 +7115,7 @@ void zend_compile_func_decl(znode *result, zend_ast *ast, bool toplevel) /* {{{
zend_do_extended_stmt();
zend_emit_final_return(0);

zend_init_static_variables_map_ptr(op_array);
pass_two(CG(active_op_array));
zend_oparray_context_end(&orig_oparray_context);

Expand Down
1 change: 1 addition & 0 deletions Zend/zend_compile.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -795,6 +795,7 @@ void zend_verify_namespace(void);
void zend_resolve_goto_label(zend_op_array *op_array, zend_op *opline);

ZEND_API void function_add_ref(zend_function *function);
void zend_init_static_variables_map_ptr(zend_op_array *op_array);

#define INITIAL_OP_ARRAY_SIZE 64

Expand Down
4 changes: 2 additions & 2 deletions Zend/zend_execute_API.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,7 +285,7 @@ void shutdown_executor(void) /* {{{ */
if (op_array->type == ZEND_INTERNAL_FUNCTION) {
break;
}
if (op_array->static_variables) {
if (ZEND_MAP_PTR(op_array->static_variables_ptr)) {
HashTable *ht = ZEND_MAP_PTR_GET(op_array->static_variables_ptr);
if (ht) {
zend_array_release(ht);
Expand All @@ -308,7 +308,7 @@ void shutdown_executor(void) /* {{{ */
zend_op_array *op_array;
ZEND_HASH_FOREACH_PTR(&ce->function_table, op_array) {
if (op_array->type == ZEND_USER_FUNCTION) {
if (op_array->static_variables) {
if (ZEND_MAP_PTR(op_array->static_variables_ptr)) {
HashTable *ht = ZEND_MAP_PTR_GET(op_array->static_variables_ptr);
if (ht) {
zend_array_release(ht);
Expand Down
33 changes: 9 additions & 24 deletions Zend/zend_inheritance.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,28 +89,10 @@ static zend_function *zend_duplicate_internal_function(zend_function *func, zend

static zend_function *zend_duplicate_user_function(zend_function *func) /* {{{ */
{
zend_function *new_function;

new_function = zend_arena_alloc(&CG(arena), sizeof(zend_op_array));
memcpy(new_function, func, sizeof(zend_op_array));

if (CG(compiler_options) & ZEND_COMPILE_PRELOAD) {
ZEND_ASSERT(new_function->op_array.fn_flags & ZEND_ACC_PRELOADED);
ZEND_MAP_PTR_NEW(new_function->op_array.static_variables_ptr);
} else {
ZEND_MAP_PTR_INIT(new_function->op_array.static_variables_ptr, &new_function->op_array.static_variables);
}

HashTable *static_properties_ptr = ZEND_MAP_PTR_GET(func->op_array.static_variables_ptr);
if (static_properties_ptr) {
/* See: Zend/tests/method_static_var.phpt */
ZEND_MAP_PTR_SET(new_function->op_array.static_variables_ptr, static_properties_ptr);
GC_TRY_ADDREF(static_properties_ptr);
} else {
GC_TRY_ADDREF(new_function->op_array.static_variables);
}

return new_function;
zend_op_array *new_op_array = zend_arena_alloc(&CG(arena), sizeof(zend_op_array));
memcpy(new_op_array, func, sizeof(zend_op_array));
zend_init_static_variables_map_ptr(new_op_array);
return (zend_function *) new_op_array;
}
/* }}} */

Expand Down Expand Up @@ -2504,20 +2486,23 @@ static zend_class_entry *zend_lazy_class_load(zend_class_entry *pce)
for (; p != end; p++) {
zend_op_array *op_array, *new_op_array;
void ***run_time_cache_ptr;
HashTable **static_variables_ptr;

op_array = Z_PTR(p->val);
ZEND_ASSERT(op_array->type == ZEND_USER_FUNCTION);
ZEND_ASSERT(op_array->scope == pce);
ZEND_ASSERT(op_array->prototype == NULL);
new_op_array = zend_arena_alloc(&CG(arena), sizeof(zend_op_array) + sizeof(void*));
new_op_array = zend_arena_alloc(&CG(arena), sizeof(zend_op_array) + 2 * sizeof(void*));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need to allocate additional word if op_array->static_variables == NULL.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Z_PTR(p->val) = new_op_array;
memcpy(new_op_array, op_array, sizeof(zend_op_array));
run_time_cache_ptr = (void***)(new_op_array + 1);
*run_time_cache_ptr = NULL;
static_variables_ptr = (HashTable **) (run_time_cache_ptr + 1);
*static_variables_ptr = NULL;
new_op_array->fn_flags &= ~ZEND_ACC_IMMUTABLE;
new_op_array->scope = ce;
ZEND_MAP_PTR_INIT(new_op_array->run_time_cache, run_time_cache_ptr);
ZEND_MAP_PTR_INIT(new_op_array->static_variables_ptr, &new_op_array->static_variables);
ZEND_MAP_PTR_INIT(new_op_array->static_variables_ptr, static_variables_ptr);

zend_update_inherited_handler(constructor);
zend_update_inherited_handler(destructor);
Expand Down
1 change: 1 addition & 0 deletions Zend/zend_language_scanner.l
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -636,6 +636,7 @@ static zend_op_array *zend_compile(int type)
zend_emit_final_return(type == ZEND_USER_FUNCTION);
op_array->line_start = 1;
op_array->line_end = last_lineno;
zend_init_static_variables_map_ptr(op_array);
pass_two(op_array);
zend_oparray_context_end(&original_oparray_context);
zend_file_context_end(&original_file_context);
Expand Down
13 changes: 7 additions & 6 deletions Zend/zend_opcode.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ void init_op_array(zend_op_array *op_array, zend_uchar type, int initial_ops_siz
op_array->last_live_range = 0;

op_array->static_variables = NULL;
ZEND_MAP_PTR_INIT(op_array->static_variables_ptr, &op_array->static_variables);
ZEND_MAP_PTR_INIT(op_array->static_variables_ptr, NULL);
op_array->last_try_catch = 0;

op_array->fn_flags = 0;
Expand Down Expand Up @@ -515,12 +515,10 @@ ZEND_API void destroy_op_array(zend_op_array *op_array)
{
uint32_t i;

if (op_array->static_variables) {
if (ZEND_MAP_PTR(op_array->static_variables_ptr)) {
HashTable *ht = ZEND_MAP_PTR_GET(op_array->static_variables_ptr);
if (ht && !(GC_FLAGS(ht) & IS_ARRAY_IMMUTABLE)) {
if (GC_DELREF(ht) == 0) {
zend_array_destroy(ht);
}
if (ht) {
zend_array_release(ht);
}
}

Expand Down Expand Up @@ -599,6 +597,9 @@ ZEND_API void destroy_op_array(zend_op_array *op_array)
}
efree(arg_info);
}
if (op_array->static_variables) {
zend_array_release(op_array->static_variables);
}
}

static void zend_update_extended_stmts(zend_op_array *op_array)
Expand Down
7 changes: 0 additions & 7 deletions Zend/zend_vm_def.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8659,15 +8659,8 @@ ZEND_VM_HANDLER(183, ZEND_BIND_STATIC, CV, UNUSED, REF)

ht = ZEND_MAP_PTR_GET(EX(func)->op_array.static_variables_ptr);
if (!ht) {
ZEND_ASSERT(EX(func)->op_array.fn_flags & (ZEND_ACC_IMMUTABLE|ZEND_ACC_PRELOADED));
ht = zend_array_dup(EX(func)->op_array.static_variables);
ZEND_MAP_PTR_SET(EX(func)->op_array.static_variables_ptr, ht);
} else if (GC_REFCOUNT(ht) > 1) {
if (!(GC_FLAGS(ht) & IS_ARRAY_IMMUTABLE)) {
GC_DELREF(ht);
}
ht = zend_array_dup(ht);
ZEND_MAP_PTR_SET(EX(func)->op_array.static_variables_ptr, ht);
}

value = (zval*)((char*)ht->arData + (opline->extended_value & ~(ZEND_BIND_REF|ZEND_BIND_IMPLICIT)));
Expand Down
7 changes: 0 additions & 7 deletions Zend/zend_vm_execute.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47468,15 +47468,8 @@ static ZEND_OPCODE_HANDLER_RET ZEND_FASTCALL ZEND_BIND_STATIC_SPEC_CV_UNUSED_HAN

ht = ZEND_MAP_PTR_GET(EX(func)->op_array.static_variables_ptr);
if (!ht) {
ZEND_ASSERT(EX(func)->op_array.fn_flags & (ZEND_ACC_IMMUTABLE|ZEND_ACC_PRELOADED));
ht = zend_array_dup(EX(func)->op_array.static_variables);
ZEND_MAP_PTR_SET(EX(func)->op_array.static_variables_ptr, ht);
} else if (GC_REFCOUNT(ht) > 1) {
if (!(GC_FLAGS(ht) & IS_ARRAY_IMMUTABLE)) {
GC_DELREF(ht);
}
ht = zend_array_dup(ht);
ZEND_MAP_PTR_SET(EX(func)->op_array.static_variables_ptr, ht);
}

value = (zval*)((char*)ht->arData + (opline->extended_value & ~(ZEND_BIND_REF|ZEND_BIND_IMPLICIT)));
Expand Down
Loading