Memory leak when class and trait declare same static property with doc block in 8.3

[rioderelfte]

Description

When building the current PHP-8.3 branch (tested with commit 99cd81c) in debug mode, PHP reports a memory leak when executing the following code:

<?php
trait MyTrait {
    /**
     * trait comment
     */
    static $property;
}

class MyClass {
    use MyTrait;

    /**
     * class comment
     */
    static $property;
}

Resulted in this output:

$ sapi/cli/php test.php
[Wed Sep 13 23:02:06 2023]  Script:  '/Users/florian/projects/php/php-src/test.php'
Zend/zend_string.h(174) :  Freeing 0x0000000107c4d540 (64 bytes), script=/Users/florian/projects/php/php-src/test.php
=== Total 1 memory leaks detected ===

The doc block from the class is leaked. I had the same behavior with the 8.3 beta 3. The issue did not occur in 8.2 for me.

The following change fixes the issue for me:

diff --git a/Zend/zend_API.c b/Zend/zend_API.c
index d629b7166a..22ed008004 100644
--- a/Zend/zend_API.c
+++ b/Zend/zend_API.c
@@ -4332,6 +4332,9 @@ ZEND_API zend_property_info *zend_declare_typed_property(zend_class_entry *ce, z
                    (property_info_ptr->flags & ZEND_ACC_STATIC) != 0) {
                        property_info->offset = property_info_ptr->offset;
                        zval_ptr_dtor(&ce->default_static_members_table[property_info->offset]);
+                       if (property_info_ptr->doc_comment) {
+                               zend_string_release_ex(property_info_ptr->doc_comment, 0);
+                       }
                        zend_hash_del(&ce->properties_info, name);
                } else {
                        property_info->offset = ce->default_static_members_count++;

But I don't think thats a proper fix for the issue.

PHP Version

PHP 8.3

Operating System

Mac OS X

[devnexen]

Not sure it's wrong but it might need more completion perhaps but I will defer to @Girgias @nielsdos .

[ndossche]

I think the suggested fix is almost right, except that I believe doc_comment may also be persistent. So please use zend_string_release instead of zend_string_release_ex.
Please create a pull request :)

[devnexen]

I think the suggested fix is almost right, except that I believe doc_comment may also be persistent. So please use zend_string_release instead of zend_string_release_ex. Please create a pull request :)

quick question, would we need to do the same here ?

[ndossche]

quick question, would we need to do the same here ?

That code is only for internal classes. I don't know if we actively support doc comments for internal classes (e.g. via stub generation). Probably, for completeness we should do the same fix there too as it's possible to call the API manually anyway. For that case, you may use zend_string_release_ex(property_info->doc_comment, 1); as internal classes are always persistent.

[rioderelfte]

Thanks for the input!

I found this zend_string_release_ex here:

php-src/Zend/zend_opcode.c

Line 388 in 3e0e7e3

zend_string_release_ex(prop_info->doc_comment, 0);

Thats why I used it in my patch as well. But I will create a PR with zend_string_release and also will add the release to the else.