Do not delete main chunk in zend_gc

[danog]

Fixes an issue where invoking zend_mm_gc twice with no allocations (and only one chunk, and no cached chunks) causes a segfault, because the first zend_mm_gc assigns NULL to the next property when copying the chunk to the (initially empty) cached chunks list, which breaks the main chunk list which expects a circular linked list instead of a null-terminated linked list.

This matches the behavior of the other invocation of zend_mm_gc, which is also skipped when acting upon the main chunk.

[danog]

Ping @bukka / @nielsdos :)

[nielsdos]

On first sight it seems right but I would need to make myself more familiar with zend_alloc first to say for sure. Do you also have a test for this?

[danog]

Calling zend_mm_gc twice in a row during the shutdown process (before the heap is freed obviously), or super early after it's created so it has no usage reproduces the issue.

Currently using https://github.com/danog/php-src/tree/heap_validation to try and catch a heap corruption bug (#17974) that won't popup with ASAN (w/ the zend allocator enabled), this is were I found the issue.

(Side note, the original #17974 issue seems to trigger when cleaning up the objects store, specifically when cleaning up a weakmap, currently debugging more to better reproduce that issue)

[bukka]

I'm not really zend_alloc expert - should be more a ping for @arnaud-lb I guess :)

[arnaud-lb]

I confirm the issue, and the fix looks good to me.

Here is a reproducer:

--- a/ext/zend_test/test.c
+++ b/ext/zend_test/test.c
@@ -1613,3 +1613,10 @@ static PHP_FUNCTION(zend_test_compile_to_ast)
 
        RETVAL_STR(result);
 }
+
+PHP_FUNCTION(zend_test_mm)
+{
+       zend_mm_heap *heap = zend_mm_startup();
+       zend_mm_gc(heap);
+       zend_mm_gc(heap);
+}
diff --git a/ext/zend_test/test.stub.php b/ext/zend_test/test.stub.php
index 9923da6dcee..48be105d219 100644
--- a/ext/zend_test/test.stub.php
+++ b/ext/zend_test/test.stub.php
@@ -202,6 +202,8 @@ enum ZendTestIntEnum: int {
         case Baz = -1;
     }
 
+    function zend_test_mm(): void {}
+
     function zend_test_array_return(): array {}
 
     /** @genstubs-expose-comment-block
@@ -399,3 +401,4 @@ function namespaced_aliased_func(): void {}
      */
     function namespaced_deprecated_aliased_func(): void {}
 }

The first zend_mm_gc() call deletes the main chunk, which shouldn't happen. The second call crashes because of that.

[TimWolla]

Should be in ext/zend_test and a --EXTENSION-- section for zend_test.

[arnaud-lb]

--EXTENSION-- section for zend_test.

Indeed, my bad.

Should be in ext/zend_test

Are you sure? I see ext/zend_test as a set of APIs for testing underlying mechanisms that are outside of ext/zend_test. Tests in ext/zend_test/tests would be for testing zend_test itself.

[TimWolla]

Are you sure?

No. What you say makes sense, boundaries are probably a little muddy 😃

[nielsdos]

Looks good but needs a small fix in the test code (besides Tim's EXTENSIONS section remark)

[nielsdos]

You need ZEND_PARSE_PARAMETERS_NONE(); for the arginfo/zpp check.

[arnaud-lb]

Indeed (that's my mistake, not @danog's, I did push this test)