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 use-of-uninitialized-value of EG(last_fatal_error_backtrace) with ZTS #17639

Merged
merged 1 commit into from
Jan 31, 2025

Conversation

iluuu1994
Copy link
Member

Static variables are zeroed, but ts memory is not. Hence, we need to do it ourselves.

See https://github.com/php/php-src/actions/runs/13043905843/job/36391169998.

Copy link
Member

@TimWolla TimWolla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Static variables are zeroed, but ts memory is not.

Makes me wonder if the first thing happening in GINIT should be a memset() to avoid this kind of bug.

Patch LGTM. /cc @ericnorris

@iluuu1994
Copy link
Member Author

Hmm... unfortunately executor_globals_ctor() is called twice, once by tsrm_update_active_threads() and once by zend_post_startup(). The ini initialization happens in-between, so fatal_error_backtrace_on is initialized to false again. We could just initialize it to true, but this might lead to slight differences between zts/nts before ini's are loaded.

… ZTS

Static variables are zeroed, but ts memory is not. Hence, we need to do
it ourselves.
@iluuu1994 iluuu1994 force-pushed the fix-uouv-last_fatal_error_backtrace branch from 9931ba7 to c7bff1e Compare January 31, 2025 11:26
@iluuu1994
Copy link
Member Author

Honestly, I have no idea why executor_globals_ctor() is being called twice, but I now moved the initialization directly after ts_allocate_fast_id(), which seems to work fine.

@iluuu1994 iluuu1994 merged commit 16c9652 into php:master Jan 31, 2025
9 checks passed
@ericnorris
Copy link
Contributor

Sorry, I ended up being a bit busier than usual at the end of last week. Thanks @iluuu1994!

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.

3 participants