From c13794cdcb1ddcd9ddcd5a14da99a904b8c91aac Mon Sep 17 00:00:00 2001 From: Arnaud Le Blanc Date: Mon, 25 Mar 2024 16:17:54 +0100 Subject: [PATCH] Adjust GC threshold if num_roots is higher than gc_threshold after collection (#13758) This fixes an edge case causing the GC to be triggered repeatedly. Destructors might add potential garbage to the buffer, so it may happen that num_root it higher than gc_threshold after collection, thus triggering a GC run almost immediately. This can happen by touching enough objects in a destructor, e.g. by iterating over an array. If this happens again in the new run, and the threshold is not updated, the GC may be triggered again. The edge case requires specific conditions to be triggered and it must happen rarely in practice: * At least GC_THRESHOLD_TRIGGER (100) objects must be collected during each run for the threshold to not be updated * At least GC_G(gc_threshold) (initially 10k) objects must be touched (decref'ed to n>0) by any destructor during each run to fill the buffer The fix is to increase the threshold if GC_G(num_roots) >= GC_G(gc_threshold) after GC. The threshold eventually reaches a point at which the second condition is not met anymore. The included tests trigger more than 200 GC runs before the fix, and 2 after the fix (dtors always trigger a second run). A related issue is that zend_gc_check_root_tmpvars() may add potential garbage before the threshold is adjusted, which may trigger GC and exhaust the stack. This is fixed by setting GC_G(active)=1 around zend_gc_check_root_tmpvars(). --- Zend/tests/gh13670_001.phpt | 43 +++++++++++++++++++++++ Zend/tests/gh13670_002.phpt | 66 +++++++++++++++++++++++++++++++++++ Zend/tests/gh13670_003.phpt | 68 +++++++++++++++++++++++++++++++++++++ Zend/zend_gc.c | 8 ++++- 4 files changed, 184 insertions(+), 1 deletion(-) create mode 100644 Zend/tests/gh13670_001.phpt create mode 100644 Zend/tests/gh13670_002.phpt create mode 100644 Zend/tests/gh13670_003.phpt diff --git a/Zend/tests/gh13670_001.phpt b/Zend/tests/gh13670_001.phpt new file mode 100644 index 00000000000..def8fa3f5f2 --- /dev/null +++ b/Zend/tests/gh13670_001.phpt @@ -0,0 +1,43 @@ +--TEST-- +GH-13670 001 +--FILE-- +self = $this; + } + public function __destruct() { + global $shutdown; + if (!$shutdown) { + new Cycle(); + } + } +} + +$defaultThreshold = gc_status()['threshold']; +for ($i = 0; $i < $defaultThreshold+1; $i++) { + new Cycle(); +} + +$objs = []; +for ($i = 0; $i < 100; $i++) { + $obj = new stdClass; + $objs[] = $obj; +} + +$st = gc_status(); + +if ($st['runs'] > 10) { + var_dump($st); +} +?> +==DONE== +--EXPECT-- +==DONE== diff --git a/Zend/tests/gh13670_002.phpt b/Zend/tests/gh13670_002.phpt new file mode 100644 index 00000000000..c822585e363 --- /dev/null +++ b/Zend/tests/gh13670_002.phpt @@ -0,0 +1,66 @@ +--TEST-- +GH-13670 002 +--FILE-- +self = $this; + } +} + +class Canary { + public $self; + public function __construct() { + $this->self = $this; + } + public function __destruct() { + global $shutdown; + if (!$shutdown) { + work(); + } + } +} + +function work() { + global $objs, $defaultThreshold; + new Canary(); + // Create some collectable garbage so the next run will not adjust + // threshold + for ($i = 0; $i < 100; $i++) { + new Cycle(); + } + // Add potential garbage to buffer + foreach (array_slice($objs, 0, $defaultThreshold) as $obj) { + $o = $obj; + } +} + +$defaultThreshold = gc_status()['threshold']; +$objs = []; +for ($i = 0; $i < $defaultThreshold*2; $i++) { + $obj = new stdClass; + $objs[] = $obj; +} + +work(); + +foreach ($objs as $obj) { + $o = $obj; +} + +$st = gc_status(); + +if ($st['runs'] > 10) { + var_dump($st); +} +?> +==DONE== +--EXPECT-- +==DONE== diff --git a/Zend/tests/gh13670_003.phpt b/Zend/tests/gh13670_003.phpt new file mode 100644 index 00000000000..defd3da4386 --- /dev/null +++ b/Zend/tests/gh13670_003.phpt @@ -0,0 +1,68 @@ +--TEST-- +GH-13670 003 +--FILE-- +self = $this; + } +} + +class Canary { + public $self; + public function __construct() { + $this->self = $this; + } + public function __destruct() { + global $shutdown; + if (!$shutdown) { + work(); + } + } +} + +function work() { + global $objs, $defaultThreshold; + new Canary(); + // Create some collectable garbage so the next run will not adjust + // threshold + for ($i = 0; $i < 100; $i++) { + new Cycle(); + } + // Add potential garbage to buffer + foreach (array_slice($objs, 0, $defaultThreshold) as $obj) { + $o = $obj; + } +} + +$defaultThreshold = gc_status()['threshold']; +$objs = []; +for ($i = 0; $i < $defaultThreshold*2; $i++) { + $obj = new stdClass; + $objs[] = $obj; +} + +work(); + +// Result of array_slice() is a tmpvar that will be checked by +// zend_gc_check_root_tmpvars() +foreach (array_slice($objs, -10) as $obj) { + $o = $obj; +} + +$st = gc_status(); + +if ($st['runs'] > 10) { + var_dump($st); +} +?> +==DONE== +--EXPECT-- +==DONE== diff --git a/Zend/zend_gc.c b/Zend/zend_gc.c index e7d9c8ef292..4417f674cbb 100644 --- a/Zend/zend_gc.c +++ b/Zend/zend_gc.c @@ -557,7 +557,7 @@ static void gc_adjust_threshold(int count) /* TODO Very simple heuristic for dynamic GC buffer resizing: * If there are "too few" collections, increase the collection threshold * by a fixed step */ - if (count < GC_THRESHOLD_TRIGGER) { + if (count < GC_THRESHOLD_TRIGGER || GC_G(num_roots) >= GC_G(gc_threshold)) { /* increase */ if (GC_G(gc_threshold) < GC_THRESHOLD_MAX) { new_threshold = GC_G(gc_threshold) + GC_THRESHOLD_STEP; @@ -1674,7 +1674,13 @@ rerun_gc: finish: zend_get_gc_buffer_release(); + + /* Prevent GC from running during zend_gc_check_root_tmpvars, before + * gc_threshold is adjusted, as this may result in unbounded recursion */ + GC_G(gc_active) = 1; zend_gc_check_root_tmpvars(); + GC_G(gc_active) = 0; + return total_count; }