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). Closes GH-16346.
php · Oct 12, 2024 · a56ff4f · a56ff4f
1 parent 7cdd130
commit a56ff4f
Show file tree

Hide file tree

Showing 3 changed files with 89 additions and 18 deletions.
diff --git a/NEWS b/NEWS
@@ -32,6 +32,9 @@ PHP                                                                        NEWS
   . Fixed bug GH-16385 (Unexpected null returned by session_set_cookie_params).
     (nielsdos)
 
+- SPL:
+  . Fixed bug GH-16337 (Use-after-free in SplHeap). (nielsdos)
+
 - XMLReader:
   . Fixed bug GH-16292 (Segmentation fault in ext/xmlreader/php_xmlreader.c).
     (nielsdos)

diff --git 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
@@ -0,0 +1,49 @@
+--TEST--
+GH-16337 (Use-after-free in SplHeap)
+--FILE--
+<?php
+
+class C {
+    function __toString() {
+        global $heap;
+        try {
+            $heap->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