From 45ca6cf6d5bd5c0ef8eda6eed245d7f4fc345478 Mon Sep 17 00:00:00 2001 From: Arnaud Le Blanc Date: Mon, 29 Jul 2024 17:49:16 +0200 Subject: [PATCH 1/2] Fix destruction of suspended generators in suspended fibers during shutdown The generator destructor is a no-op when the generator is running in a fiber, because unwinding the fiber will resume the generator. Normally the destructor is never called in this case, but this can happen during shutdown. We detect that a generator is running in a fiber with the ZEND_GENERATOR_IN_FIBER flag. This change fixes two cases in which this mechanism was broken: - The flag was not added when resuming a 'yield from $nonGenerator', as this is handled in a separate code path - When a generator that is running in a fiber has multiple children (aka multiple generators yielding from it), all of them could be considered to also run in a fiber (only one actually is), and could leak if not destroyed before shutdown. --- Zend/tests/gh15108-001.phpt | 36 +++++++++++++++++ Zend/tests/gh15108-002.phpt | 40 +++++++++++++++++++ Zend/tests/gh15108-003.phpt | 38 ++++++++++++++++++ Zend/tests/gh15108-004.phpt | 39 ++++++++++++++++++ Zend/tests/gh15108-005.phpt | 45 +++++++++++++++++++++ Zend/tests/gh15108-006.phpt | 49 +++++++++++++++++++++++ Zend/tests/gh15108-007.phpt | 51 ++++++++++++++++++++++++ Zend/zend_generators.c | 79 ++++++++++++++++++++++++++++++++----- Zend/zend_generators.h | 1 + 9 files changed, 368 insertions(+), 10 deletions(-) create mode 100644 Zend/tests/gh15108-001.phpt create mode 100644 Zend/tests/gh15108-002.phpt create mode 100644 Zend/tests/gh15108-003.phpt create mode 100644 Zend/tests/gh15108-004.phpt create mode 100644 Zend/tests/gh15108-005.phpt create mode 100644 Zend/tests/gh15108-006.phpt create mode 100644 Zend/tests/gh15108-007.phpt diff --git a/Zend/tests/gh15108-001.phpt b/Zend/tests/gh15108-001.phpt new file mode 100644 index 0000000000000..9971ada5b37fb --- /dev/null +++ b/Zend/tests/gh15108-001.phpt @@ -0,0 +1,36 @@ +--TEST-- +GH-15108 001: Segfault with delegated generator in suspended fiber +--FILE-- +current()); + $iterable->next(); + var_dump("not executed"); +}); + +$ref = $fiber; + +$fiber->start(); + +?> +==DONE== +--EXPECT-- +string(3) "foo" +==DONE== diff --git a/Zend/tests/gh15108-002.phpt b/Zend/tests/gh15108-002.phpt new file mode 100644 index 0000000000000..64bdafaebf5ed --- /dev/null +++ b/Zend/tests/gh15108-002.phpt @@ -0,0 +1,40 @@ +--TEST-- +GH-15108 002: Segfault with delegated generator in suspended fiber +--FILE-- +current()); + $iterable->next(); + var_dump("not executed"); +}); + +$ref = $fiber; + +$fiber->start(); + +?> +==DONE== +--EXPECT-- +string(3) "foo" +==DONE== diff --git a/Zend/tests/gh15108-003.phpt b/Zend/tests/gh15108-003.phpt new file mode 100644 index 0000000000000..c32e7b8f0c8d7 --- /dev/null +++ b/Zend/tests/gh15108-003.phpt @@ -0,0 +1,38 @@ +--TEST-- +GH-15108 003: Segfault with delegated generator in suspended fiber +--FILE-- +current()); + $b->next(); + var_dump("not executed"); +}); + +$ref = $fiber; + +$fiber->start(); + +?> +==DONE== +--EXPECT-- +string(3) "foo" +==DONE== diff --git a/Zend/tests/gh15108-004.phpt b/Zend/tests/gh15108-004.phpt new file mode 100644 index 0000000000000..caa548e515d3a --- /dev/null +++ b/Zend/tests/gh15108-004.phpt @@ -0,0 +1,39 @@ +--TEST-- +GH-15108 004: Segfault with delegated generator in suspended fiber +--FILE-- +current()); + var_dump($c->current()); + $b->next(); + var_dump("not executed"); +}); + +$ref = $fiber; + +$fiber->start(); + +?> +==DONE== +--EXPECT-- +string(3) "foo" +string(3) "foo" +==DONE== diff --git a/Zend/tests/gh15108-005.phpt b/Zend/tests/gh15108-005.phpt new file mode 100644 index 0000000000000..76457e62c3f12 --- /dev/null +++ b/Zend/tests/gh15108-005.phpt @@ -0,0 +1,45 @@ +--TEST-- +GH-15108 005: Segfault with delegated generator in suspended fiber +--FILE-- +current()); + +$fiber = new Fiber(function () use ($iterable) { + $iterable->next(); + var_dump("not executed"); +}); + +$ref = $fiber; + +$fiber->start(); + +?> +==DONE== +--EXPECT-- +string(3) "foo" +==DONE== diff --git a/Zend/tests/gh15108-006.phpt b/Zend/tests/gh15108-006.phpt new file mode 100644 index 0000000000000..5fb830d97cb1d --- /dev/null +++ b/Zend/tests/gh15108-006.phpt @@ -0,0 +1,49 @@ +--TEST-- +GH-15108 006: Segfault with delegated generator in suspended fiber +--FILE-- +current()); +var_dump($b->current()); + +$fiber = new Fiber(function () use ($a, $b, $g) { + $a->next(); + var_dump("not executed"); +}); + +$ref = $fiber; + +$fiber->start(); + +?> +==DONE== +--EXPECT-- +string(3) "foo" +string(3) "foo" +==DONE== diff --git a/Zend/tests/gh15108-007.phpt b/Zend/tests/gh15108-007.phpt new file mode 100644 index 0000000000000..d66a8010a83f4 --- /dev/null +++ b/Zend/tests/gh15108-007.phpt @@ -0,0 +1,51 @@ +--TEST-- +GH-15108 007: Segfault with delegated generator in suspended fiber +--FILE-- +current()); +var_dump($b->current()); + +$fiber = new Fiber(function () use ($a, $b, $c, $d, $g) { + $b->next(); + var_dump("not executed"); +}); + +$ref = $fiber; + +$fiber->start(); + +?> +==DONE== +--EXPECT-- +string(3) "foo" +string(3) "foo" +==DONE== diff --git a/Zend/zend_generators.c b/Zend/zend_generators.c index 1d785377e82e7..e4a6b6de2f38a 100644 --- a/Zend/zend_generators.c +++ b/Zend/zend_generators.c @@ -19,6 +19,7 @@ #include "zend.h" #include "zend_API.h" +#include "zend_hash.h" #include "zend_interfaces.h" #include "zend_exceptions.h" #include "zend_generators.h" @@ -216,19 +217,68 @@ static zend_always_inline void clear_link_to_root(zend_generator *generator) { } } +/* In the context of zend_generator_dtor_storage during shutdown, check if + * the intermediate node 'generator' is running in a fiber */ +static inline bool check_node_running_in_fiber(zend_generator *generator) { + ZEND_ASSERT(EG(flags) & EG_FLAGS_IN_SHUTDOWN); + ZEND_ASSERT(generator->execute_data); + + if (generator->flags & ZEND_GENERATOR_IN_FIBER) { + return true; + } + + if (generator->node.children == 0) { + return false; + } + + if (generator->flags & ZEND_GENERATOR_VISITED) { + return false; + } + generator->flags |= ZEND_GENERATOR_VISITED; + + if (generator->node.children == 1) { + if (check_node_running_in_fiber(generator->node.child.single)) { + goto in_fiber; + } + return false; + } + + zend_generator *child; + ZEND_HASH_FOREACH_PTR(generator->node.child.ht, child) { + if (check_node_running_in_fiber(child)) { + goto in_fiber; + } + } ZEND_HASH_FOREACH_END(); + return false; + +in_fiber: + generator->flags |= ZEND_GENERATOR_IN_FIBER; + return true; +} + static void zend_generator_dtor_storage(zend_object *object) /* {{{ */ { zend_generator *generator = (zend_generator*) object; + zend_generator *current_generator = zend_generator_get_current(generator); zend_execute_data *ex = generator->execute_data; uint32_t op_num, try_catch_offset; int i; - /* Generator is running in a suspended fiber. - * Will be dtor during fiber dtor */ - if (zend_generator_get_current(generator)->flags & ZEND_GENERATOR_IN_FIBER) { - /* Prevent finally blocks from yielding */ - generator->flags |= ZEND_GENERATOR_FORCED_CLOSE; - return; + /* If current_generator is running in a fiber, there are 2 cases to consider: + * - If generator is also marked with ZEND_GENERATOR_IN_FIBER, then the + * entire path from current_generator to generator is executing in a + * fiber. Do not dtor now: These will be dtor when terminating the fiber. + * - If generator is not marked with ZEND_GENERATOR_IN_FIBER, and has a + * child marked with ZEND_GENERATOR_IN_FIBER, then this an intermediate + * node of case 1. Otherwise generator is not executing in a fiber and we + * can dtor. + */ + if (current_generator->flags & ZEND_GENERATOR_IN_FIBER) { + if (check_node_running_in_fiber(generator)) { + /* Prevent finally blocks from yielding */ + generator->flags |= ZEND_GENERATOR_FORCED_CLOSE; + return; + } } /* leave yield from mode to properly allow finally execution */ @@ -722,6 +772,13 @@ ZEND_API void zend_generator_resume(zend_generator *orig_generator) /* {{{ */ return; } + if (EG(active_fiber)) { + orig_generator->flags |= ZEND_GENERATOR_IN_FIBER; + if (generator != orig_generator) { + generator->flags |= ZEND_GENERATOR_IN_FIBER; + } + } + /* Drop the AT_FIRST_YIELD flag */ orig_generator->flags &= ~ZEND_GENERATOR_AT_FIRST_YIELD; @@ -752,7 +809,10 @@ ZEND_API void zend_generator_resume(zend_generator *orig_generator) /* {{{ */ EG(current_execute_data) = original_execute_data; EG(jit_trace_num) = original_jit_trace_num; - orig_generator->flags &= ~ZEND_GENERATOR_DO_INIT; + orig_generator->flags &= ~(ZEND_GENERATOR_DO_INIT | ZEND_GENERATOR_IN_FIBER); + if (generator != orig_generator) { + generator->flags &= ~ZEND_GENERATOR_IN_FIBER; + } return; } /* If there are no more delegated values, resume the generator @@ -765,8 +825,7 @@ ZEND_API void zend_generator_resume(zend_generator *orig_generator) /* {{{ */ } /* Resume execution */ - generator->flags |= ZEND_GENERATOR_CURRENTLY_RUNNING - | (EG(active_fiber) ? ZEND_GENERATOR_IN_FIBER : 0); + generator->flags |= ZEND_GENERATOR_CURRENTLY_RUNNING; if (!ZEND_OBSERVER_ENABLED) { zend_execute_ex(generator->execute_data); } else { @@ -817,7 +876,7 @@ ZEND_API void zend_generator_resume(zend_generator *orig_generator) /* {{{ */ goto try_again; } - orig_generator->flags &= ~ZEND_GENERATOR_DO_INIT; + orig_generator->flags &= ~(ZEND_GENERATOR_DO_INIT | ZEND_GENERATOR_IN_FIBER); } /* }}} */ diff --git a/Zend/zend_generators.h b/Zend/zend_generators.h index 4ccc42b92f668..c2d524c6a57f5 100644 --- a/Zend/zend_generators.h +++ b/Zend/zend_generators.h @@ -93,6 +93,7 @@ static const zend_uchar ZEND_GENERATOR_FORCED_CLOSE = 0x2; static const zend_uchar ZEND_GENERATOR_AT_FIRST_YIELD = 0x4; static const zend_uchar ZEND_GENERATOR_DO_INIT = 0x8; static const zend_uchar ZEND_GENERATOR_IN_FIBER = 0x10; +static const zend_uchar ZEND_GENERATOR_VISITED = 0x20; void zend_register_generator_ce(void); ZEND_API void zend_generator_close(zend_generator *generator, bool finished_execution); From 46f785ae6f95e65145bbdebf34a5053da04bb4fb Mon Sep 17 00:00:00 2001 From: Arnaud Le Blanc Date: Tue, 30 Jul 2024 14:16:39 +0200 Subject: [PATCH 2/2] Review --- Zend/zend_generators.c | 12 ++++-------- Zend/zend_generators.h | 2 +- 2 files changed, 5 insertions(+), 9 deletions(-) diff --git a/Zend/zend_generators.c b/Zend/zend_generators.c index e4a6b6de2f38a..f7475ad6fbb77 100644 --- a/Zend/zend_generators.c +++ b/Zend/zend_generators.c @@ -231,10 +231,10 @@ static inline bool check_node_running_in_fiber(zend_generator *generator) { return false; } - if (generator->flags & ZEND_GENERATOR_VISITED) { + if (generator->flags & ZEND_GENERATOR_DTOR_VISITED) { return false; } - generator->flags |= ZEND_GENERATOR_VISITED; + generator->flags |= ZEND_GENERATOR_DTOR_VISITED; if (generator->node.children == 1) { if (check_node_running_in_fiber(generator->node.child.single)) { @@ -774,9 +774,7 @@ ZEND_API void zend_generator_resume(zend_generator *orig_generator) /* {{{ */ if (EG(active_fiber)) { orig_generator->flags |= ZEND_GENERATOR_IN_FIBER; - if (generator != orig_generator) { - generator->flags |= ZEND_GENERATOR_IN_FIBER; - } + generator->flags |= ZEND_GENERATOR_IN_FIBER; } /* Drop the AT_FIRST_YIELD flag */ @@ -810,9 +808,7 @@ ZEND_API void zend_generator_resume(zend_generator *orig_generator) /* {{{ */ EG(jit_trace_num) = original_jit_trace_num; orig_generator->flags &= ~(ZEND_GENERATOR_DO_INIT | ZEND_GENERATOR_IN_FIBER); - if (generator != orig_generator) { - generator->flags &= ~ZEND_GENERATOR_IN_FIBER; - } + generator->flags &= ~ZEND_GENERATOR_IN_FIBER; return; } /* If there are no more delegated values, resume the generator diff --git a/Zend/zend_generators.h b/Zend/zend_generators.h index c2d524c6a57f5..a3daba3a839bb 100644 --- a/Zend/zend_generators.h +++ b/Zend/zend_generators.h @@ -93,7 +93,7 @@ static const zend_uchar ZEND_GENERATOR_FORCED_CLOSE = 0x2; static const zend_uchar ZEND_GENERATOR_AT_FIRST_YIELD = 0x4; static const zend_uchar ZEND_GENERATOR_DO_INIT = 0x8; static const zend_uchar ZEND_GENERATOR_IN_FIBER = 0x10; -static const zend_uchar ZEND_GENERATOR_VISITED = 0x20; +static const zend_uchar ZEND_GENERATOR_DTOR_VISITED = 0x20; void zend_register_generator_ce(void); ZEND_API void zend_generator_close(zend_generator *generator, bool finished_execution);