[go: up one dir, main page]
Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
rioderelfte opened this issue Sep 13, 2023 · 5 comments
Closed

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

rioderelfte opened this issue Sep 13, 2023 · 5 comments
Labels
Bug Status: Verified

Comments

@rioderelfte
Copy link
Contributor

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
Copy link
Member

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

@nielsdos
Copy link
Member

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
Copy link
Member

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 ?

@nielsdos
Copy link
Member

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
Copy link
Contributor Author

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.

@rioderelfte rioderelfte mentioned this issue Sep 18, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug Status: Verified
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@rioderelfte @devnexen @nielsdos