From c6eb017cf7a4720a2eeba175cdff8c6fa76f2a20 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Thu, 10 Oct 2024 21:28:45 +0200 Subject: [PATCH] Fix GH-16337: Use-after-free in SplHeap We introduce a new flag to indicate when a heap or priority queue is write-locked. In principle we could've used SPL_HEAP_CORRUPTED too, but that won't be descriptive to users (and it's a lie too). --- ext/spl/spl_heap.c | 55 +++++++++++++++++++++++++------------- ext/spl/tests/gh16337.phpt | 49 +++++++++++++++++++++++++++++++++ 2 files changed, 86 insertions(+), 18 deletions(-) create mode 100644 ext/spl/tests/gh16337.phpt diff --git a/ext/spl/spl_heap.c b/ext/spl/spl_heap.c index 736f151da5846..df67bcb9daf83 100644 --- a/ext/spl/spl_heap.c +++ b/ext/spl/spl_heap.c @@ -32,6 +32,7 @@ #define PTR_HEAP_BLOCK_SIZE 64 #define SPL_HEAP_CORRUPTED 0x00000001 +#define SPL_HEAP_WRITE_LOCKED 0x00000002 zend_object_handlers spl_handler_SplHeap; zend_object_handlers spl_handler_SplPriorityQueue; @@ -278,12 +279,16 @@ static void spl_ptr_heap_insert(spl_ptr_heap *heap, void *elem, void *cmp_userda heap->max_size *= 2; } + heap->flags |= SPL_HEAP_WRITE_LOCKED; + /* sifting up */ for (i = heap->count; i > 0 && heap->cmp(spl_heap_elem(heap, (i-1)/2), elem, cmp_userdata) < 0; i = (i-1)/2) { spl_heap_elem_copy(heap, spl_heap_elem(heap, i), spl_heap_elem(heap, (i-1)/2)); } heap->count++; + heap->flags &= ~SPL_HEAP_WRITE_LOCKED; + if (EG(exception)) { /* exception thrown during comparison */ heap->flags |= SPL_HEAP_CORRUPTED; @@ -311,6 +316,8 @@ static int spl_ptr_heap_delete_top(spl_ptr_heap *heap, void *elem, void *cmp_use return FAILURE; } + heap->flags |= SPL_HEAP_WRITE_LOCKED; + if (elem) { spl_heap_elem_copy(heap, elem, spl_heap_elem(heap, 0)); } else { @@ -334,6 +341,8 @@ static int spl_ptr_heap_delete_top(spl_ptr_heap *heap, void *elem, void *cmp_use } } + heap->flags &= ~SPL_HEAP_WRITE_LOCKED; + if (EG(exception)) { /* exception thrown during comparison */ heap->flags |= SPL_HEAP_CORRUPTED; @@ -374,10 +383,14 @@ static spl_ptr_heap *spl_ptr_heap_clone(spl_ptr_heap *from) { /* {{{ */ static void spl_ptr_heap_destroy(spl_ptr_heap *heap) { /* {{{ */ int i; + heap->flags |= SPL_HEAP_WRITE_LOCKED; + for (i = 0; i < heap->count; ++i) { heap->dtor(spl_heap_elem(heap, i)); } + heap->flags &= ~SPL_HEAP_WRITE_LOCKED; + efree(heap->elements); efree(heap); } @@ -597,6 +610,21 @@ PHP_METHOD(SplHeap, isEmpty) } /* }}} */ +static zend_result spl_heap_consistency_validations(const spl_heap_object *intern, bool write) +{ + if (intern->heap->flags & SPL_HEAP_CORRUPTED) { + zend_throw_exception(spl_ce_RuntimeException, "Heap is corrupted, heap properties are no longer ensured.", 0); + return FAILURE; + } + + if (write && (intern->heap->flags & SPL_HEAP_WRITE_LOCKED)) { + zend_throw_exception(spl_ce_RuntimeException, "Heap cannot be changed when it is already being modified.", 0); + return FAILURE; + } + + return SUCCESS; +} + /* {{{ Push $value on the heap */ PHP_METHOD(SplHeap, insert) { @@ -609,8 +637,7 @@ PHP_METHOD(SplHeap, insert) intern = Z_SPLHEAP_P(ZEND_THIS); - if (intern->heap->flags & SPL_HEAP_CORRUPTED) { - zend_throw_exception(spl_ce_RuntimeException, "Heap is corrupted, heap properties are no longer ensured.", 0); + if (UNEXPECTED(spl_heap_consistency_validations(intern, true) != SUCCESS)) { RETURN_THROWS(); } @@ -632,8 +659,7 @@ PHP_METHOD(SplHeap, extract) intern = Z_SPLHEAP_P(ZEND_THIS); - if (intern->heap->flags & SPL_HEAP_CORRUPTED) { - zend_throw_exception(spl_ce_RuntimeException, "Heap is corrupted, heap properties are no longer ensured.", 0); + if (UNEXPECTED(spl_heap_consistency_validations(intern, true) != SUCCESS)) { RETURN_THROWS(); } @@ -658,8 +684,7 @@ PHP_METHOD(SplPriorityQueue, insert) intern = Z_SPLHEAP_P(ZEND_THIS); - if (intern->heap->flags & SPL_HEAP_CORRUPTED) { - zend_throw_exception(spl_ce_RuntimeException, "Heap is corrupted, heap properties are no longer ensured.", 0); + if (UNEXPECTED(spl_heap_consistency_validations(intern, true) != SUCCESS)) { RETURN_THROWS(); } @@ -699,8 +724,7 @@ PHP_METHOD(SplPriorityQueue, extract) intern = Z_SPLHEAP_P(ZEND_THIS); - if (intern->heap->flags & SPL_HEAP_CORRUPTED) { - zend_throw_exception(spl_ce_RuntimeException, "Heap is corrupted, heap properties are no longer ensured.", 0); + if (UNEXPECTED(spl_heap_consistency_validations(intern, true) != SUCCESS)) { RETURN_THROWS(); } @@ -726,8 +750,7 @@ PHP_METHOD(SplPriorityQueue, top) intern = Z_SPLHEAP_P(ZEND_THIS); - if (intern->heap->flags & SPL_HEAP_CORRUPTED) { - zend_throw_exception(spl_ce_RuntimeException, "Heap is corrupted, heap properties are no longer ensured.", 0); + if (UNEXPECTED(spl_heap_consistency_validations(intern, false) != SUCCESS)) { RETURN_THROWS(); } @@ -837,8 +860,7 @@ PHP_METHOD(SplHeap, top) intern = Z_SPLHEAP_P(ZEND_THIS); - if (intern->heap->flags & SPL_HEAP_CORRUPTED) { - zend_throw_exception(spl_ce_RuntimeException, "Heap is corrupted, heap properties are no longer ensured.", 0); + if (UNEXPECTED(spl_heap_consistency_validations(intern, false) != SUCCESS)) { RETURN_THROWS(); } @@ -902,8 +924,7 @@ static zval *spl_heap_it_get_current_data(zend_object_iterator *iter) /* {{{ */ { spl_heap_object *object = Z_SPLHEAP_P(&iter->data); - if (object->heap->flags & SPL_HEAP_CORRUPTED) { - zend_throw_exception(spl_ce_RuntimeException, "Heap is corrupted, heap properties are no longer ensured.", 0); + if (UNEXPECTED(spl_heap_consistency_validations(object, false) != SUCCESS)) { return NULL; } @@ -920,8 +941,7 @@ static zval *spl_pqueue_it_get_current_data(zend_object_iterator *iter) /* {{{ * zend_user_iterator *user_it = (zend_user_iterator *) iter; spl_heap_object *object = Z_SPLHEAP_P(&iter->data); - if (object->heap->flags & SPL_HEAP_CORRUPTED) { - zend_throw_exception(spl_ce_RuntimeException, "Heap is corrupted, heap properties are no longer ensured.", 0); + if (UNEXPECTED(spl_heap_consistency_validations(object, false) != SUCCESS)) { return NULL; } @@ -949,8 +969,7 @@ static void spl_heap_it_move_forward(zend_object_iterator *iter) /* {{{ */ { spl_heap_object *object = Z_SPLHEAP_P(&iter->data); - if (object->heap->flags & SPL_HEAP_CORRUPTED) { - zend_throw_exception(spl_ce_RuntimeException, "Heap is corrupted, heap properties are no longer ensured.", 0); + if (UNEXPECTED(spl_heap_consistency_validations(object, false) != SUCCESS)) { return; } diff --git a/ext/spl/tests/gh16337.phpt b/ext/spl/tests/gh16337.phpt new file mode 100644 index 0000000000000..94cf9d90cb1a9 --- /dev/null +++ b/ext/spl/tests/gh16337.phpt @@ -0,0 +1,49 @@ +--TEST-- +GH-16337 (Use-after-free in SplHeap) +--FILE-- +extract(); + } catch (Throwable $e) { + echo $e->getMessage(), "\n"; + } + try { + $heap->insert(1); + } catch (Throwable $e) { + echo $e->getMessage(), "\n"; + } + echo $heap->top(), "\n"; + return "0"; + } +} + +$heap = new SplMinHeap; +for ($i = 0; $i < 100; $i++) { + $heap->insert((string) $i); +} +$heap->insert(new C); + +?> +--EXPECT-- +Heap cannot be changed when it is already being modified. +Heap cannot be changed when it is already being modified. +0 +Heap cannot be changed when it is already being modified. +Heap cannot be changed when it is already being modified. +0 +Heap cannot be changed when it is already being modified. +Heap cannot be changed when it is already being modified. +0 +Heap cannot be changed when it is already being modified. +Heap cannot be changed when it is already being modified. +0 +Heap cannot be changed when it is already being modified. +Heap cannot be changed when it is already being modified. +0 +Heap cannot be changed when it is already being modified. +Heap cannot be changed when it is already being modified. +0