-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Add ASAN support to the zend allocator #18858
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
base: master
Are you sure you want to change the base?
Conversation
@dstogov / @nielsdos / @iluuu1994 This is now ready for review! The implementation is finalized, and has actually revealed a shutdown memory leak in php-fpm when terminated via a SIGTERM, due to the fact that alloc_globals_dtor is not getting invoked (the leak is surprisingly revealed exclusively by the usage of the poison functions, in fact, altering the ZEND_MM_POISON/ZEND_MM_UNPOISON macros to be no-ops re-hides the issue again, i.e. it's not caused by the other code changes/reorganization). This PR also includes the changes from #18834, as the bug is detectable by ASAN now. Side note, tracked_malloc also poisons/unpoisons the heap to avoid issues with the observe proxies. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! This adds a bit of complexity but this seems worth it. I would suggest we run this in nightly. We could add a step to the existing ASAN jobs that runs the test suite without --asan
(which sets USE_ZEND_ALLOC=0
).
This looks good to me apart from a few comments.
Co-authored-by: Arnaud Le Blanc <[email protected]>
Co-authored-by: Arnaud Le Blanc <[email protected]>
Co-authored-by: Arnaud Le Blanc <[email protected]>
Co-authored-by: Arnaud Le Blanc <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks interesting and can allow finding other memory related bugs.
This shouldn't affect performance of the release build, but this should be re-checked before merge.
The amount of changes is big, but almost all of them are trivial.
+1 from me
@iluuu1994 @nielsdos please also take a look
I'll make a deep review later this week. +1 on the idea. |
Ping @nielsdos? :) |
I apologize, this went out of my mind and apparently I didn't track this properly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When trying to run ./run-tests.php
with this I get an immediate ASAN crash that should be fixed:
==104448==ERROR: AddressSanitizer: use-after-poison on address 0x7b96b5c01080 at pc 0x560e145d67d4 bp 0x7ffd7eb60800 sp 0x7ffd7eb607f8
WRITE of size 8 at 0x7b96b5c01080 thread T0
#0 0x560e145d67d3 in zend_mm_alloc_small_slow /run/media/niels/MoreData/php-src/Zend/zend_alloc.c:1507:14
#1 0x560e145d4f8e in zend_mm_alloc_small /run/media/niels/MoreData/php-src/Zend/zend_alloc.c:1546:10
#2 0x560e145c629b in zend_mm_alloc_heap /run/media/niels/MoreData/php-src/Zend/zend_alloc.c:1625:9
#3 0x560e145cb2a4 in _emalloc /run/media/niels/MoreData/php-src/Zend/zend_alloc.c:3034:14
#4 0x560e14c45e82 in cwd_globals_ctor /run/media/niels/MoreData/php-src/Zend/zend_virtual_cwd.c:145:2
#5 0x560e14c45dce in virtual_cwd_startup /run/media/niels/MoreData/php-src/Zend/zend_virtual_cwd.c:211:2
#6 0x560e14c5ec09 in zend_startup /run/media/niels/MoreData/php-src/Zend/zend.c:942:2
#7 0x560e14320b8d in php_module_startup /run/media/niels/MoreData/php-src/main/main.c:2200:2
#8 0x560e14c733b4 in php_cli_startup /run/media/niels/MoreData/php-src/sapi/cli/php_cli.c:398:9
#9 0x560e14c6e243 in main /run/media/niels/MoreData/php-src/sapi/cli/php_cli.c:1330:6
#10 0x7f96bb8906b4 (/usr/lib/libc.so.6+0x276b4) (BuildId: 468e3585c794491a48ea75fceb9e4d6b1464fc35)
#11 0x7f96bb890768 in __libc_start_main (/usr/lib/libc.so.6+0x27768) (BuildId: 468e3585c794491a48ea75fceb9e4d6b1464fc35)
#12 0x560e13003584 in _start (/run/media/niels/MoreData/php-src/sapi/cli/php+0xa03584) (BuildId: 04a8f40ab178fb1528246ccec118a5d2e939fd1b)
@nielsdos what compiler, platform, configure flags and memory manager envvars are you using? |
It seems I was testing on an older checkout, I tested it again with a reset to your branch and it works, sorry for the confusion... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besides the remaining cs nit I don't see anything obviously wrong. Thanks for the clarifications. I think this patch is worth it. I'll leave this open for some time to see if others want to review this too.
This pull request adds ASAN support to the zend allocator, by automatically poisoning all unused pages, chunks and heap management structures before exiting the alloc, free, etc (all ZEND_API) functions.
Internally, the implementation uses the following rules:
This is what allowed me to find #18833, before I found the fast shutdown workaround.