Skip to content

Make guards context-dependent #14994

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Make guards context-dependent #14994

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
95d28fd
Make guards context-dependent
iluuu1994 Jul 17, 2024
1bc65f2
Make sure the guard actually keeps working in a different context
iluuu1994 Jul 17, 2024
ade1e85
Test fibers, move bailout context bump, remove hash guard
iluuu1994 Jul 17, 2024
f5adc6c
Fix bailout context number
iluuu1994 Jul 17, 2024
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
42 changes: 42 additions & 0 deletions Zend/tests/gh14983_1.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
--TEST--
GH-14983: Shutdown disregards guards active on bailout
--SKIPIF--
<?php if (getenv("SKIP_SLOW_TESTS")) die('skip slow test'); ?>
--FILE--
<?php

ini_set('max_execution_time', 1);
$loop = true;

class A {
function __get($name) {
global $loop;
if ($loop) {
while (true) {}
}
echo __METHOD__, "\n";
return $this->{$name};
}
}

global $a;
$a = new A;

function shutdown() {
global $a;
global $loop;
$loop = false;
var_dump($a->foo);
}

register_shutdown_function('shutdown');

$a->foo;

?>
--EXPECTF--
Fatal error: Maximum execution time of 1 second exceeded in %s on line %d
A::__get

Warning: Undefined property: A::$foo in %s on line %d
NULL
32 changes: 32 additions & 0 deletions Zend/tests/gh14983_2.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
--TEST--
GH-14983: Guards from fibers are separated from guards from main context
--FILE--
<?php

class Foo {
public function __get($prop) {
echo "__set($prop)\n";
if (Fiber::getCurrent()) {
Fiber::suspend();
echo "Resuming\n";
}
}
}

$foo = new Foo();

$fiber = new Fiber(function () use ($foo) {
$foo->bar;
});

$value = $fiber->start();
echo "Suspended\n";
$foo->bar;
$fiber->resume('test');

?>
--EXPECT--
__set(bar)
Suspended
__set(bar)
Resuming
1 change: 1 addition & 0 deletions Zend/zend.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1240,6 +1240,7 @@ ZEND_API ZEND_COLD ZEND_NORETURN void _zend_bailout(const char *filename, uint32
CG(in_compilation) = 0;
CG(memoize_mode) = 0;
EG(current_execute_data) = NULL;
EG(guard_context) = ++EG(guard_context_counter);
LONGJMP(*EG(bailout), FAILURE);
}
/* }}} */
Expand Down
3 changes: 3 additions & 0 deletions Zend/zend_execute_API.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,9 @@ void init_executor(void) /* {{{ */
EG(filename_override) = NULL;
EG(lineno_override) = -1;

EG(guard_context) = 0;
EG(guard_context_counter) = 0;

zend_max_execution_timer_init();
zend_fiber_init();
zend_weakrefs_init();
Expand Down
5 changes: 5 additions & 0 deletions Zend/zend_fibers.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@ typedef struct _zend_fiber_vm_state {
void *stack_base;
void *stack_limit;
#endif
uint32_t guard_context;
} zend_fiber_vm_state;

static zend_always_inline void zend_fiber_capture_vm_state(zend_fiber_vm_state *state)
Expand All @@ -133,6 +134,7 @@ static zend_always_inline void zend_fiber_capture_vm_state(zend_fiber_vm_state *
state->stack_base = EG(stack_base);
state->stack_limit = EG(stack_limit);
#endif
state->guard_context = EG(guard_context);
}

static zend_always_inline void zend_fiber_restore_vm_state(zend_fiber_vm_state *state)
Expand All @@ -150,6 +152,7 @@ static zend_always_inline void zend_fiber_restore_vm_state(zend_fiber_vm_state *
EG(stack_base) = state->stack_base;
EG(stack_limit) = state->stack_limit;
#endif
EG(guard_context) = state->guard_context;
}

#ifdef ZEND_FIBER_UCONTEXT
Expand Down Expand Up @@ -446,6 +449,7 @@ ZEND_API zend_result zend_fiber_init_context(zend_fiber_context *context, void *

context->kind = kind;
context->function = coroutine;
context->guard_context = ++EG(guard_context_counter);

// Set status in case memory has not been zeroed.
context->status = ZEND_FIBER_STATUS_INIT;
Expand Down Expand Up @@ -500,6 +504,7 @@ ZEND_API void zend_fiber_switch_context(zend_fiber_transfer *transfer)
transfer->context = from;

EG(current_fiber_context) = to;
EG(guard_context) = to->guard_context;

#ifdef __SANITIZE_ADDRESS__
void *fake_stack = NULL;
Expand Down
2 changes: 2 additions & 0 deletions Zend/zend_fibers.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,8 @@ struct _zend_fiber_context {
/* Observer state */
zend_execute_data *top_observed_frame;

uint32_t guard_context;

/* Reserved for extensions */
void *reserved[ZEND_MAX_RESERVED_RESOURCES];
};
Expand Down
7 changes: 7 additions & 0 deletions Zend/zend_globals.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -295,6 +295,13 @@ struct _zend_executor_globals {
zend_string *filename_override;
zend_long lineno_override;

/* Guards are context dependent. I.e. if __get() is being called for an object
* within a fiber, the guard will _not_ skip __get() in the main context. To
* achieve this, we offset the guard string hash by the guard context.
* Additionally, bailout will discard the current guards in the same way. */
uint32_t guard_context;
uint32_t guard_context_counter;
Copy link
Member

@bwoebi bwoebi Jul 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm quite afraid of 32 bit integer overflows for long running scripts here. It's a very good source of a heisenbug, especially if the counter overflows to 0 and a fiber and a main thread happen to suspend at the same place. With 64 bits I wouldn't worry.

After all the hash is also a zend_ulong, so, why not make this a zend_ulong too?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds reasonable 👍


#ifdef ZEND_CHECK_STACK_LIMIT
zend_call_stack call_stack;
zend_long max_allowed_stack_size;
Expand Down
19 changes: 16 additions & 3 deletions Zend/zend_object_handlers.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -565,18 +565,27 @@ ZEND_API uint32_t *zend_get_property_guard(zend_object *zobj, zend_string *membe
zval *zv;
uint32_t *ptr;

if (EXPECTED(EG(guard_context) == 0)) {
member = zend_string_copy(member);
} else {
member = zend_string_init(ZSTR_VAL(member), ZSTR_LEN(member), false);
ZSTR_H(member) = zend_string_hash_val(member) + EG(guard_context);
Comment on lines +571 to +572
Copy link
Member

@bwoebi bwoebi Jul 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I don't particularly like having the overhead of a string allocation for every single property hook access in a fiber. It will also fail the trivial str == member comparison and require a full comparison for any access in fiber context. It's probably okay for just magic __get/__set, but I expect property hooks to be quite common.

Can you just allocate 8 bytes more for zend_objects using guards? And store the guard_context there - at least for the case where no nested (or parallel in case of fibers) access happens?

The hack with the hash is fine in case it's actually going to the hashtable. But too much overhead for the common scenario.

Copy link
Member Author

Choose a reason for hiding this comment

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

but I expect property hooks to be quite common.

Hooks don't do guards anymore, so. :)

Copy link
Member

Choose a reason for hiding this comment

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

Oh, right. I still think allocating a few more bytes for the guard wouldn't do harm though, i.e. common case.

Copy link
Member

Choose a reason for hiding this comment

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

This not only requires a new string allocation for each __get/__set call in a fiber. These strings are also going to be kept in guards hash until the object destruction.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's true. I can try @bwoebi's approach, but that also means increasing the allocation size of all objects with guards, even when they don't use fibers. I would assume that applications using fibers are generally more modern, possibly also making less use of __get/__set. But of course we can't know for sure.

}

ZEND_ASSERT(zobj->ce->ce_flags & ZEND_ACC_USE_GUARDS);
zv = zend_get_guard_value(zobj);
if (EXPECTED(Z_TYPE_P(zv) == IS_STRING)) {
zend_string *str = Z_STR_P(zv);
if (EXPECTED(str == member) ||
/* str and member don't necessarily have a pre-calculated hash value here */
EXPECTED(zend_string_equal_content(str, member))) {
(EXPECTED(ZSTR_HASH(str) == ZSTR_HASH(member)
&& zend_string_equal_content(str, member)))) {
zend_string_release(member);
return &Z_GUARD_P(zv);
} else if (EXPECTED(Z_GUARD_P(zv) == 0)) {
zval_ptr_dtor_str(zv);
ZVAL_STR_COPY(zv, member);
/* Transfer ownership. */
ZVAL_STR(zv, member);
return &Z_GUARD_P(zv);
} else {
ALLOC_HASHTABLE(guards);
Expand All @@ -592,18 +601,22 @@ ZEND_API uint32_t *zend_get_property_guard(zend_object *zobj, zend_string *membe
ZEND_ASSERT(guards != NULL);
zv = zend_hash_find(guards, member);
if (zv != NULL) {
zend_string_release(member);
return (uint32_t*)(((uintptr_t)Z_PTR_P(zv)) & ~1);
}
} else {
ZEND_ASSERT(Z_TYPE_P(zv) == IS_UNDEF);
ZVAL_STR_COPY(zv, member);
Z_GUARD_P(zv) &= ~ZEND_GUARD_PROPERTY_MASK;
zend_string_release(member);
return &Z_GUARD_P(zv);
}
/* we have to allocate uint32_t separately because ht->arData may be reallocated */
ptr = (uint32_t*)emalloc(sizeof(uint32_t));
*ptr = 0;
return (uint32_t*)zend_hash_add_new_ptr(guards, member, ptr);
uint32_t *result = (uint32_t*)zend_hash_add_new_ptr(guards, member, ptr);
zend_string_release(member);
return result;
}
/* }}} */

Expand Down