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

use local variable for retval to avoid clobbering #640

Merged
merged 1 commit into from
Mar 14, 2023

Conversation

zsistla
Copy link
Contributor

@zsistla zsistla commented Mar 14, 2023

@lavarou 's great research and findings:
While I was looking for some information about -Wclobber and learning about zend_try/zend_catch I found these two interesting resources:
php/php-src#5151 - this basically shows that PHP team is aware of issues caused by -Wclobber and basically turn these warnings off (I don’t know if it’s good or bad that PHP is ignoring compiler warnings though; maybe they know what they’re doing)
https://github.com/php/php-src/blob/master/main/streams/userspace.c#L335-L340 - this is an example in PHP source code how zend_call_method_if_exists is called. It is a nice little trick of using an automatic variable to store user function call result. Based on this I came up with this idea: https://github.com/newrelic/newrelic-php-agent/compare/compiler_error...lavarou/fix/php-call-try-catch?expand=1. It seems to resolve the issue - I haven’t tested it past agent’s unit tests (make agent-valgrind) for PHP 8.2. Using automatic/local variable has this advantage that memory allocation is delayed until it is certain retval is good for use - i.e. zend_call_method_if_exists succeeded. Clobbering (longjmp called from within zend_call_method_if_exists which will cause this line to jump here) may invalidate the value of retval pointer and at the time it is freed here it may point to invalid memory and cause segfault. I don’t know this for certain. It would be awesome if there was a way to unit tests this code somehow and emulate happy path as well as exception path.

@zsistla zsistla merged commit 8803b1a into compiler_error Mar 14, 2023
@lavarou lavarou deleted the lavarou/fix/php-call-try-catch branch March 14, 2023 16:58
@lavarou lavarou restored the lavarou/fix/php-call-try-catch branch August 9, 2023 13:49
@lavarou lavarou deleted the lavarou/fix/php-call-try-catch branch August 9, 2023 13:59
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.

2 participants