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

Fix destruction of dl'ed module classes #17961

Open
wants to merge 1 commit into
base: PHP-8.3
Choose a base branch
from

Conversation

arnaud-lb
Copy link
Member

@arnaud-lb arnaud-lb commented Mar 3, 2025

During shutdown, we destroy classes of dl()'ed modules in clean_module_classes():

php-src/Zend/zend_API.c

Lines 3278 to 3281 in ff88701

static void clean_module_classes(int module_number) /* {{{ */
{
zend_hash_apply_with_argument(EG(class_table), clean_module_class, (void *) &module_number);
}

Child classes of a module use structures of the parent class (such as inherited properties), which is destroyed earlier, so we have a use-after-free when destroying a child class:

==477311==ERROR: AddressSanitizer: heap-use-after-free on address 0x50600001b4c0 at pc 0x561078b05961 bp 0x7ffd20dfac20 sp 0x7ffd20dfac18
READ of size 8 at 0x50600001b4c0 thread T0
    #0 0x561078b05960 in destroy_zend_class Zend/zend_opcode.c:447:20
    #1 0x561078c4d44f in _zend_hash_del_el_ex Zend/zend_hash.c:1488:3
    #2 0x561078c4aae1 in _zend_hash_del_el Zend/zend_hash.c:1515:2
    #3 0x561078c5fcea in zend_hash_apply_with_argument Zend/zend_hash.c:2128:5
    #4 0x561078bd75c2 in clean_module_classes Zend/zend_API.c:3128:2
    #5 0x561078bd67cb in module_destructor Zend/zend_API.c:3165:3
    #6 0x561078bd9f91 in zend_post_deactivate_modules Zend/zend_API.c:3291:4
    #7 0x56107876a286 in php_request_shutdown main/main.c:1917:3
    #8 0x56107993f19a in do_cli sapi/cli/php_cli.c:1137:3
    #9 0x56107993a31a in main sapi/cli/php_cli.c:1341:18
    #10 0x7f75b4e2d247 in __libc_start_call_main (/lib64/libc.so.6+0x3247) (BuildId: 515c33a35f41020661fea8ac4eb995e26ccd6b00)
    #11 0x7f75b4e2d30a in __libc_start_main@GLIBC_2.2.5 (/lib64/libc.so.6+0x330a) (BuildId: 515c33a35f41020661fea8ac4eb995e26ccd6b00)
    #12 0x561077601fc4 in _start (sapi/cli/php+0x201fc4) (BuildId: 06db8fea59ff1f83339934018c55f48156e833d6)

0x50600001b4c0 is located 32 bytes inside of 56-byte region [0x50600001b4a0,0x50600001b4d8)
freed by thread T0 here:
    #0 0x5610776a1b5a in free (sapi/cli/php+0x2a1b5a) (BuildId: 06db8fea59ff1f83339934018c55f48156e833d6)
    #1 0x561078b05c35 in destroy_zend_class Zend/zend_opcode.c:453:6
    #2 0x561078c4d44f in _zend_hash_del_el_ex Zend/zend_hash.c:1488:3
    #3 0x561078c4aae1 in _zend_hash_del_el Zend/zend_hash.c:1515:2
    #4 0x561078c5fcea in zend_hash_apply_with_argument Zend/zend_hash.c:2128:5
    #5 0x561078bd75c2 in clean_module_classes Zend/zend_API.c:3128:2
    #6 0x561078bd67cb in module_destructor Zend/zend_API.c:3165:3
    #7 0x561078bd9f91 in zend_post_deactivate_modules Zend/zend_API.c:3291:4
    #8 0x56107876a286 in php_request_shutdown main/main.c:1917:3
    #9 0x56107993f19a in do_cli sapi/cli/php_cli.c:1137:3
    #10 0x56107993a31a in main sapi/cli/php_cli.c:1341:18
    #11 0x7f75b4e2d247 in __libc_start_call_main (/lib64/libc.so.6+0x3247) (BuildId: 515c33a35f41020661fea8ac4eb995e26ccd6b00)
    #12 0x7f75b4e2d30a in __libc_start_main@GLIBC_2.2.5 (/lib64/libc.so.6+0x330a) (BuildId: 515c33a35f41020661fea8ac4eb995e26ccd6b00)
    #13 0x561077601fc4 in _start (sapi/cli/php+0x201fc4) (BuildId: 06db8fea59ff1f83339934018c55f48156e833d6)

This was found by @TimWolla.

In this PR, I destroy classes in reverse order, as it is done in zend_shutdown():

php-src/Zend/zend.c

Lines 1175 to 1176 in ff88701

/* Child classes may reuse structures from parent classes, so destroy in reverse order. */
zend_hash_graceful_reverse_destroy(GLOBAL_CLASS_TABLE);

I don't use zend_hash_reverse_apply() because I need to pass an argument to the callback, and because of this comment.

@arnaud-lb arnaud-lb marked this pull request as ready for review March 3, 2025 13:05
@bwoebi
Copy link
Member

bwoebi commented Mar 4, 2025

Do I see it right that this fixes #15367?

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

Successfully merging this pull request may close these issues.

4 participants