Skip to content

Initialize ping_auto_globals_mask to prevent undefined behaviour #10121

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

Closed
wants to merge 2 commits into from

Conversation

nielsdos
Copy link
Member

ping_auto_globals_mask was not initialized on all paths towards script->ping_auto_globals_mask = ping_auto_globals_mask;, which is undefined behaviour.

Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

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

Nice catch! We don't currently run preloading jobs with MSAN and only have a single test for auto_globals_jit (sapi/phpdbg/tests/info_001.phpt) which is why I guess this was never caught. A test for this could be added though (forcing both preloading and auto_globals_jit through the --INI-- section).

@nielsdos
Copy link
Member Author

Thanks for the review.
I've added a test to test the codepath which previously had UB.

Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

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

Thank you! That looks perfect! Just a tiny nit.

@nielsdos nielsdos force-pushed the fix-ub-ping_auto_globals_mask branch from 17540e2 to f5d1b7c Compare December 18, 2022 18:08
@nielsdos
Copy link
Member Author

Thanks. I updated the test. :)
Failure in Travis "No output has been received in the last 10m0s" seems unrelated.

Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

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

Looks good, thank you! I'll merge this tonight when I'm off work (this requires manual merging as it's targeting an older branch).

@iluuu1994 iluuu1994 closed this in c4487b7 Dec 22, 2022
@nielsdos
Copy link
Member Author

Thanks for the merge.
I think you forgot to commit the regression test though, I can't seem to find it in the master branch.

@iluuu1994
Copy link
Member

@nielsdos Oh, sorry! I'll fix that in a second.

@iluuu1994
Copy link
Member

I picked the wrong commit to reset to. Should be good now. Thanks for noticing.

@nielsdos nielsdos deleted the fix-ub-ping_auto_globals_mask branch March 19, 2023 15:23
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