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.

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

Release memory in Imagick::getImageArtifacts() #602

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mikhainin
Copy link
Contributor

It seems pretty similar to #600 but it's another method and I decided to split it into separated PR, just in case.
Will be happy to merge them, if you prefer

$ ZEND_DONT_UNLOAD_MODULES=1 ./tests/315_Imagick_getImageArtifacts.sh
Ok
=================================================================
==2891804==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 8192 byte(s) in 1 object(s) allocated from:
    #0 0x7f7607a64bb8 in __interceptor_malloc (/lib64/libasan.so.5+0xefbb8)
    #1 0x7f76027b1b0d in MagickGetImageArtifacts MagickWand/magick-property.c:622
    #2 0x7f7602b52613 in zim_Imagick_getImageArtifacts /home/mgalanin/php-build/github.com-mkoppanen-imagick/imagick_class.c:14197
    #3 0xd3438d in ZEND_DO_FCALL_SPEC_RETVAL_USED_HANDLER /home/mgalanin/php-build/php-sources/Zend/zend_vm_execute.h:1863
    #4 0xe7687e in execute_ex /home/mgalanin/php-build/php-sources/Zend/zend_vm_execute.h:55195
    #5 0xe83e40 in zend_execute /home/mgalanin/php-build/php-sources/Zend/zend_vm_execute.h:59523
    #6 0xca0a4d in zend_execute_scripts /home/mgalanin/php-build/php-sources/Zend/zend.c:1826
    #7 0xb309dc in php_execute_script /home/mgalanin/php-build/php-sources/main/main.c:2568
    #8 0xf1b6ff in do_cli /home/mgalanin/php-build/php-sources/sapi/cli/php_cli.c:949
    #9 0xf1d9dc in main /home/mgalanin/php-build/php-sources/sapi/cli/php_cli.c:1341
    #10 0x7f76064afd84 in __libc_start_main (/lib64/libc.so.6+0x3ad84)

Indirect leak of 878 byte(s) in 48 object(s) allocated from:
    #0 0x7f7607a64bb8 in __interceptor_malloc (/lib64/libasan.so.5+0xefbb8)
    #1 0x7f760223a823 in ConstantString MagickCore/string.c:691

SUMMARY: AddressSanitizer: 9070 byte(s) leaked in 49 allocation(s).

@Danack
Copy link
Collaborator

Danack commented Mar 18, 2023

Will be happy to merge them, if you prefer

Nope......having small commits on code is almost always preferable.

I think about the only time that merging commits is appropriate is when it's documentation cleanup, and the changes are a bazillion trivial things.

I hadn't actually seen ZEND_DONT_UNLOAD_MODULES before. I think it's actually meant to be used when running the tests through valgrind aka php run-tests.php -m. However it actually looks like it's been broken for 'a while'.

@mikhainin
Copy link
Contributor Author

mikhainin commented Mar 18, 2023

I hadn't actually seen ZEND_DONT_UNLOAD_MODULES before. I think it's actually meant to be used when running the tests through valgrind

Right, I just prefer using ASAN (which is pretty similar in this case), it checks leaks on the program termination, by this time PHP usually unloads all the modules and we cannot see the proper backtrace

@mikhainin
Copy link
Contributor Author

However it actually looks like it's been broken for 'a while'.

Sorry, what do you mean?

@mikhainin
Copy link
Contributor Author

Hey, @Danack. I can see that this PR is already in master.

Is there anything left before the PR becomes closed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants