From cff6df2c2dcaef15e3310af7a9e2525a9df10fd4 Mon Sep 17 00:00:00 2001 From: HypeMC Date: Tue, 13 Oct 2026 05:46:53 +0200 Subject: [PATCH] Fix Heap comparator interference --- README.md | 2 + php_ds.c | 2 +- src/ds/ds_heap.c | 25 +++--- src/ds/ds_heap.h | 12 +-- src/php/classes/php_heap_ce.c | 14 ++-- src/php/handlers/php_heap_handlers.c | 2 +- src/php/iterators/php_heap_iterator.c | 3 +- src/php/objects/php_heap.c | 20 ++--- src/php/objects/php_heap.h | 7 +- tests/Heap/comparator_nested.phpt | 78 +++++++++++++++++++ tests/Heap/comparator_traversable.phpt | 20 +++++ tests/Heap/iterator_comparator_isolation.phpt | 43 ++++++++++ 12 files changed, 186 insertions(+), 42 deletions(-) create mode 100644 tests/Heap/comparator_nested.phpt create mode 100644 tests/Heap/comparator_traversable.phpt create mode 100644 tests/Heap/iterator_comparator_isolation.phpt diff --git a/README.md b/README.md index 00a7ccb..824ba68 100644 --- a/README.md +++ b/README.md @@ -24,6 +24,8 @@ Documentation is available on [php.net](https://www.php.net/manual/en/book.ds.ph Heap iterators retain a snapshot of the values present when created; `rewind()` restarts that snapshot even if the original heap has changed. Calling `next()` or `rewind()` on the same iterator from its comparator throws a `RuntimeException`. +Each `Heap` uses its own comparator, including during nested heap operations and interleaved iteration. + You should also include the [polyfill](https://github.com/php-ds/polyfill) in your project for IDE integration. ## Installation diff --git a/php_ds.c b/php_ds.c index 8ee471a..9a0c06e 100644 --- a/php_ds.c +++ b/php_ds.c @@ -132,7 +132,7 @@ PHP_FUNCTION(heap) if (values && Z_TYPE_P(values) != IS_NULL) { ds_heap_compare_func_t compare = php_ds_heap_get_compare(obj); - ds_heap_push_all(heap, values, compare); + ds_heap_push_all(heap, values, compare, obj); } ZVAL_OBJ(return_value, obj_zend); diff --git a/src/ds/ds_heap.c b/src/ds/ds_heap.c index 23237f5..06aef5d 100644 --- a/src/ds/ds_heap.c +++ b/src/ds/ds_heap.c @@ -28,7 +28,7 @@ static inline void ds_heap_compact(ds_heap_t *heap) } } -int ds_heap_default_compare(zval *a, zval *b) +int ds_heap_default_compare(zval *a, zval *b, void *context) { zval retval; if (compare_function(&retval, a, b) == SUCCESS) { @@ -104,7 +104,7 @@ zval *ds_heap_peek(ds_heap_t *heap) return &heap->nodes[0]; } -void ds_heap_push(ds_heap_t *heap, zval *value, ds_heap_compare_func_t compare) +void ds_heap_push(ds_heap_t *heap, zval *value, ds_heap_compare_func_t compare, void *context) { uint32_t index; uint32_t parent; @@ -117,7 +117,7 @@ void ds_heap_push(ds_heap_t *heap, zval *value, ds_heap_compare_func_t compare) for (index = heap->size; index > 0; index = parent) { parent = PARENT(index); - if (compare(value, &heap->nodes[parent]) <= 0) { + if (compare(value, &heap->nodes[parent], context) <= 0) { break; } @@ -129,7 +129,7 @@ void ds_heap_push(ds_heap_t *heap, zval *value, ds_heap_compare_func_t compare) heap->size++; } -void ds_heap_pop(ds_heap_t *heap, zval *return_value, ds_heap_compare_func_t compare) +void ds_heap_pop(ds_heap_t *heap, zval *return_value, ds_heap_compare_func_t compare, void *context) { uint32_t index; uint32_t swap; @@ -165,12 +165,12 @@ void ds_heap_pop(ds_heap_t *heap, zval *return_value, ds_heap_compare_func_t com swap = LEFT(index); // Pick the larger child. - if (swap + 1 < heap->size && compare(&heap->nodes[swap], &heap->nodes[swap + 1]) < 0) { + if (swap + 1 < heap->size && compare(&heap->nodes[swap], &heap->nodes[swap + 1], context) < 0) { swap++; } // If the bottom value is >= the larger child, we're done. - if (compare(&bottom, &heap->nodes[swap]) >= 0) { + if (compare(&bottom, &heap->nodes[swap], context) >= 0) { break; } @@ -184,16 +184,17 @@ void ds_heap_pop(ds_heap_t *heap, zval *return_value, ds_heap_compare_func_t com typedef struct _ds_heap_push_all_data { ds_heap_t *heap; ds_heap_compare_func_t compare; + void *context; } ds_heap_push_all_data; static int ds_heap_iterator_add(zend_object_iterator *iterator, void *puser) { ds_heap_push_all_data *data = (ds_heap_push_all_data *) puser; - ds_heap_push(data->heap, iterator->funcs->get_current_data(iterator), data->compare); + ds_heap_push(data->heap, iterator->funcs->get_current_data(iterator), data->compare, data->context); return ZEND_HASH_APPLY_KEEP; } -void ds_heap_push_all(ds_heap_t *heap, zval *values, ds_heap_compare_func_t compare) +void ds_heap_push_all(ds_heap_t *heap, zval *values, ds_heap_compare_func_t compare, void *context) { if (!values) { return; @@ -202,14 +203,14 @@ void ds_heap_push_all(ds_heap_t *heap, zval *values, ds_heap_compare_func_t comp if (ds_is_array(values)) { zval *value; ZEND_HASH_FOREACH_VAL(Z_ARRVAL_P(values), value) { - ds_heap_push(heap, value, compare); + ds_heap_push(heap, value, compare, context); } ZEND_HASH_FOREACH_END(); return; } if (ds_is_traversable(values)) { - ds_heap_push_all_data data = { heap, compare }; + ds_heap_push_all_data data = { heap, compare, context }; spl_iterator_apply(values, ds_heap_iterator_add, &data); return; } @@ -217,7 +218,7 @@ void ds_heap_push_all(ds_heap_t *heap, zval *values, ds_heap_compare_func_t comp ARRAY_OR_TRAVERSABLE_REQUIRED(); } -void ds_heap_to_array(ds_heap_t *heap, zval *array, ds_heap_compare_func_t compare) +void ds_heap_to_array(ds_heap_t *heap, zval *array, ds_heap_compare_func_t compare, void *context) { if (heap->size == 0) { array_init(array); @@ -231,7 +232,7 @@ void ds_heap_to_array(ds_heap_t *heap, zval *array, ds_heap_compare_func_t compa while (clone->size > 0) { zval value; - ds_heap_pop(clone, &value, compare); + ds_heap_pop(clone, &value, compare, context); add_next_index_zval(array, &value); } diff --git a/src/ds/ds_heap.h b/src/ds/ds_heap.h index 4f78d40..2688c29 100644 --- a/src/ds/ds_heap.h +++ b/src/ds/ds_heap.h @@ -8,7 +8,7 @@ #define DS_HEAP_SIZE(h) ((h)->size) #define DS_HEAP_IS_EMPTY(h) ((h)->size == 0) -typedef int (*ds_heap_compare_func_t)(zval *a, zval *b); +typedef int (*ds_heap_compare_func_t)(zval *a, zval *b, void *context); typedef struct _ds_heap_t { zval *nodes; @@ -23,19 +23,19 @@ ds_heap_t *ds_heap_clone(ds_heap_t *heap); void ds_heap_separate(ds_heap_t **heap); void ds_heap_release(ds_heap_t *heap); -void ds_heap_push(ds_heap_t *heap, zval *value, ds_heap_compare_func_t compare); -void ds_heap_push_all(ds_heap_t *heap, zval *values, ds_heap_compare_func_t compare); -void ds_heap_pop(ds_heap_t *heap, zval *return_value, ds_heap_compare_func_t compare); +void ds_heap_push(ds_heap_t *heap, zval *value, ds_heap_compare_func_t compare, void *context); +void ds_heap_push_all(ds_heap_t *heap, zval *values, ds_heap_compare_func_t compare, void *context); +void ds_heap_pop(ds_heap_t *heap, zval *return_value, ds_heap_compare_func_t compare, void *context); zval *ds_heap_peek(ds_heap_t *heap); void ds_heap_allocate(ds_heap_t *heap, uint32_t capacity); uint32_t ds_heap_capacity(ds_heap_t *heap); -void ds_heap_to_array(ds_heap_t *heap, zval *array, ds_heap_compare_func_t compare); +void ds_heap_to_array(ds_heap_t *heap, zval *array, ds_heap_compare_func_t compare, void *context); void ds_heap_clear(ds_heap_t *heap); void ds_heap_free(ds_heap_t *heap); -int ds_heap_default_compare(zval *a, zval *b); +int ds_heap_default_compare(zval *a, zval *b, void *context); #endif diff --git a/src/php/classes/php_heap_ce.c b/src/php/classes/php_heap_ce.c index 1c6a819..c216f96 100644 --- a/src/php/classes/php_heap_ce.c +++ b/src/php/classes/php_heap_ce.c @@ -40,7 +40,7 @@ METHOD(__construct) if (values && Z_TYPE_P(values) != IS_NULL) { SEPARATE(); ds_heap_compare_func_t compare = php_ds_heap_get_compare(obj); - ds_heap_push_all(obj->heap, values, compare); + ds_heap_push_all(obj->heap, values, compare, obj); } } @@ -70,7 +70,7 @@ METHOD(push) SEPARATE(); for (int i = 0; i < argc; i++) { - ds_heap_push(obj->heap, &argv[i], compare); + ds_heap_push(obj->heap, &argv[i], compare, obj); } } @@ -79,7 +79,7 @@ METHOD(pop) php_ds_heap_t *obj = THIS_DS_HEAP(); PARSE_NONE; SEPARATE(); - ds_heap_pop(obj->heap, return_value, php_ds_heap_get_compare(obj)); + ds_heap_pop(obj->heap, return_value, php_ds_heap_get_compare(obj), obj); } METHOD(peek) @@ -117,14 +117,14 @@ METHOD(toArray) { php_ds_heap_t *obj = THIS_DS_HEAP(); PARSE_NONE; - ds_heap_to_array(obj->heap, return_value, php_ds_heap_get_compare(obj)); + ds_heap_to_array(obj->heap, return_value, php_ds_heap_get_compare(obj), obj); } METHOD(jsonSerialize) { php_ds_heap_t *obj = THIS_DS_HEAP(); PARSE_NONE; - ds_heap_to_array(obj->heap, return_value, php_ds_heap_get_compare(obj)); + ds_heap_to_array(obj->heap, return_value, php_ds_heap_get_compare(obj), obj); } METHOD(__serialize) @@ -141,7 +141,7 @@ METHOD(__serialize) while (clone->size > 0) { zval value; - ds_heap_pop(clone, &value, compare); + ds_heap_pop(clone, &value, compare, obj); add_next_index_zval(return_value, &value); } @@ -157,7 +157,7 @@ METHOD(__unserialize) zval *value; ZEND_HASH_FOREACH_VAL(Z_ARRVAL_P(data), value) { - ds_heap_push(obj->heap, value, compare); + ds_heap_push(obj->heap, value, compare, obj); } ZEND_HASH_FOREACH_END(); } diff --git a/src/php/handlers/php_heap_handlers.c b/src/php/handlers/php_heap_handlers.c index b7e0d87..25e96bc 100644 --- a/src/php/handlers/php_heap_handlers.c +++ b/src/php/handlers/php_heap_handlers.c @@ -35,7 +35,7 @@ static HashTable *php_ds_heap_get_debug_info(zend_object *obj, int *is_temp) ds_heap_t *heap = heap_obj->heap; zval arr; *is_temp = 1; - ds_heap_to_array(heap, &arr, php_ds_heap_get_compare(heap_obj)); + ds_heap_to_array(heap, &arr, php_ds_heap_get_compare(heap_obj), heap_obj); return Z_ARRVAL(arr); } diff --git a/src/php/iterators/php_heap_iterator.c b/src/php/iterators/php_heap_iterator.c index 01d4cae..24306da 100644 --- a/src/php/iterators/php_heap_iterator.c +++ b/src/php/iterators/php_heap_iterator.c @@ -40,7 +40,8 @@ static void php_ds_heap_iterator_set_current(php_ds_heap_iterator_t *iterator) DTOR_AND_UNDEF(&iterator->intern.data); if (!DS_HEAP_IS_EMPTY(iterator->heap)) { - ds_heap_pop(iterator->heap, &iterator->intern.data, iterator->compare); + ds_heap_pop(iterator->heap, &iterator->intern.data, iterator->compare, + php_ds_heap_fetch_object(iterator->object)); } } diff --git a/src/php/objects/php_heap.c b/src/php/objects/php_heap.c index 459cd5c..2b57abe 100644 --- a/src/php/objects/php_heap.c +++ b/src/php/objects/php_heap.c @@ -3,21 +3,23 @@ #include "php_heap.h" -int ds_heap_user_compare(zval *a, zval *b) +int ds_heap_user_compare(zval *a, zval *b, void *context) { + php_ds_heap_t *obj = context; + // Keep callback state local so nested heap operations cannot overwrite it. + zend_fcall_info fci = obj->fci; + zend_fcall_info_cache fci_cache = obj->fci_cache; zval params[2]; zval retval; ZVAL_COPY_VALUE(¶ms[0], a); ZVAL_COPY_VALUE(¶ms[1], b); - DSG(user_compare_fci).param_count = 2; - DSG(user_compare_fci).params = params; - DSG(user_compare_fci).retval = &retval; + fci.param_count = 2; + fci.params = params; + fci.retval = &retval; - if (zend_call_function( - &DSG(user_compare_fci), - &DSG(user_compare_fci_cache)) == SUCCESS) { + if (zend_call_function(&fci, &fci_cache) == SUCCESS) { int result = (int) zval_get_long(&retval); zval_ptr_dtor(&retval); return result; @@ -29,8 +31,6 @@ int ds_heap_user_compare(zval *a, zval *b) ds_heap_compare_func_t php_ds_heap_get_compare(php_ds_heap_t *obj) { if (obj->has_comparator) { - DSG(user_compare_fci) = obj->fci; - DSG(user_compare_fci_cache) = obj->fci_cache; return ds_heap_user_compare; } return ds_heap_default_compare; @@ -119,7 +119,7 @@ int php_ds_heap_unserialize(zval *object, zend_class_entry *ce, const unsigned c goto error; } - ds_heap_push(heap, value, ds_heap_default_compare); + ds_heap_push(heap, value, ds_heap_default_compare, NULL); } ZVAL_OBJ(object, php_ds_heap_create_object_ex(heap)); diff --git a/src/php/objects/php_heap.h b/src/php/objects/php_heap.h index 02cb2c4..9eca48a 100644 --- a/src/php/objects/php_heap.h +++ b/src/php/objects/php_heap.h @@ -32,15 +32,14 @@ do { \ /** * Returns the compare function for this heap. - * If the heap has a custom comparator, sets up the module globals - * and returns the user compare function. Otherwise returns the default. + * The heap object must be passed as context when invoking the comparator. */ ds_heap_compare_func_t php_ds_heap_get_compare(php_ds_heap_t *obj); /** - * User comparator function that reads from module globals. + * User comparator function that reads the callable from the heap object. */ -int ds_heap_user_compare(zval *a, zval *b); +int ds_heap_user_compare(zval *a, zval *b, void *context); zend_object *php_ds_heap_create_object_ex(ds_heap_t *heap); zend_object *php_ds_heap_create_object(zend_class_entry *ce); diff --git a/tests/Heap/comparator_nested.phpt b/tests/Heap/comparator_nested.phpt new file mode 100644 index 0000000..2de731e --- /dev/null +++ b/tests/Heap/comparator_nested.phpt @@ -0,0 +1,78 @@ +--TEST-- +Heap: nested heap operations preserve each caller's comparator +--FILE-- + $a <=> $b); +$compare = function ($a, $b) use ($inner) { + $inner->toArray(); + return $b <=> $a; +}; +$heap = new Ds\Heap([3, 5, 1, 4, 2], $compare); +echo json_encode($heap->toArray()), "\n"; + +// A temporary heap's callback must not be retained by the surrounding operation. +$heap = new Ds\Heap([3, 5, 1, 4, 2], function ($a, $b) { + $temporary = new Ds\Heap([10, 20], fn($x, $y) => $x <=> $y); + $temporary->toArray(); + return $b <=> $a; +}); +echo json_encode($heap->toArray()), "\n"; + +// An exception caught inside the comparator must not replace its callback state. +$throw = false; +$throwing = new Ds\Heap([1, 2, 3], function ($a, $b) use (&$throw) { + if ($throw) { + throw new RuntimeException('nested comparison'); + } + return $a <=> $b; +}); +$throw = true; +$heap = new Ds\Heap([3, 5, 1, 4, 2], function ($a, $b) use ($throwing) { + try { + $throwing->toArray(); + } catch (RuntimeException $e) { + } + return $b <=> $a; +}); +echo json_encode($heap->toArray()), "\n"; +$heap->push(7, 0, 6); +echo json_encode($heap), "\n"; +echo json_encode($heap->__serialize()), "\n"; +$result = []; +while (!$heap->isEmpty()) { + $result[] = $heap->pop(); +} +echo json_encode($result), "\n"; + +$heap = new Ds\Heap([], $compare); +$heap->__unserialize([3, 5, 1, 4, 2]); +echo json_encode($heap->toArray()), "\n"; +echo json_encode(Ds\heap([3, 5, 1, 4, 2], $compare)->toArray()), "\n"; + +// Heap operations must also leave the comparator of a surrounding sort intact. +$sequence = new Ds\Seq([3, 5, 1, 4, 2]); +$sequence->sort(function ($a, $b) use ($inner) { + $inner->toArray(); + return $b <=> $a; +}); +echo json_encode($sequence->toArray()), "\n"; + +// Sorts inside a heap comparator must not affect the heap's ordering either. +$heap = new Ds\Heap([3, 5, 1, 4, 2], function ($a, $b) { + $sequence = new Ds\Seq([3, 1, 2]); + $sequence->sort(fn($x, $y) => $x <=> $y); + return $b <=> $a; +}); +echo json_encode($heap->toArray()), "\n"; +?> +--EXPECT-- +[1,2,3,4,5] +[1,2,3,4,5] +[1,2,3,4,5] +[0,1,2,3,4,5,6,7] +[0,1,2,3,4,5,6,7] +[0,1,2,3,4,5,6,7] +[1,2,3,4,5] +[1,2,3,4,5] +[5,4,3,2,1] +[1,2,3,4,5] diff --git a/tests/Heap/comparator_traversable.phpt b/tests/Heap/comparator_traversable.phpt new file mode 100644 index 0000000..95c49d2 --- /dev/null +++ b/tests/Heap/comparator_traversable.phpt @@ -0,0 +1,20 @@ +--TEST-- +Heap: traversable input may operate on heaps with different comparators +--FILE-- + $a <=> $b); + foreach ([3, 5, 1, 4, 2] as $value) { + $other->toArray(); + yield $value; + } +} + +$min = fn($a, $b) => $b <=> $a; +echo json_encode((new Ds\Heap(values(), $min))->toArray()), "\n"; +echo json_encode(Ds\heap(values(), $min)->toArray()), "\n"; +?> +--EXPECT-- +[1,2,3,4,5] +[1,2,3,4,5] diff --git a/tests/Heap/iterator_comparator_isolation.phpt b/tests/Heap/iterator_comparator_isolation.phpt new file mode 100644 index 0000000..6d4ce39 --- /dev/null +++ b/tests/Heap/iterator_comparator_isolation.phpt @@ -0,0 +1,43 @@ +--TEST-- +Heap: nested and interleaved iterators use their own comparators +--FILE-- + $b <=> $a); +$max = new Ds\Heap([1, 2, 3, 4, 5], fn($a, $b) => $a <=> $b); + +$outer = []; +$inner = []; +foreach ($min as $value) { + $outer[] = $value; + $row = []; + foreach ($max as $other) { + $row[] = $other; + } + $inner[] = $row; +} +echo json_encode($outer), "\n"; +echo json_encode($inner), "\n"; + +$a = $min->getIterator(); +$b = $max->getIterator(); +$result = []; +while ($a->valid() && $b->valid()) { + $result[] = [$a->current(), $b->current()]; + $a->next(); + $b->next(); +} +echo json_encode($result), "\n"; + +// Creating an iterator inside another collection's comparator must not replace it. +$sequence = new Ds\Seq([3, 5, 1, 4, 2]); +$sequence->sort(function ($a, $b) use ($max) { + $iterator = $max->getIterator(); + return $b <=> $a; +}); +echo json_encode($sequence->toArray()), "\n"; +?> +--EXPECT-- +[1,2,3,4,5] +[[5,4,3,2,1],[5,4,3,2,1],[5,4,3,2,1],[5,4,3,2,1],[5,4,3,2,1]] +[[1,5],[2,4],[3,3],[4,2],[5,1]] +[5,4,3,2,1]