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 segfault by checking observer_class_lookup for NULL in observer callback #102

Merged
merged 2 commits into from
Oct 18, 2023

Conversation

agoallikmaa
Copy link
Contributor

Resolves open-telemetry/opentelemetry-php#1168 and most likely also open-telemetry/opentelemetry-php#1167

I have managed to reproduce the crash in open-telemetry/opentelemetry-php#1168 locally on PHP 8.2.10 with a minimal sample (running via Apache).

<?php
function test() {}
header_register_callback('test');

This happens because if nothing has been written to output, php_output_deactivate causes sapi_send_headers to be called, which in turn runs the header callback. Since header callback is a PHP function, the observer callback is invoked for that. The problem is that php_output_deactivate was called by php_request_shutdown immediately after module RSHUTDOWN callbacks were called, in which observer_class_lookup used by our observer callback is freed and set to NULL.

I have confirmed that the crash in the above sample stops reproducing after this change.

Additionally, the issue in open-telemetry/opentelemetry-php#1167 seems to be caused by another extension invoking PHP code in their RINIT. Since it is possible that the other extension is earlier in the load list, thus observer_class_lookup is not constructed yet (it is guaranteed to not be garbage because GINIT sets it to NULL, so checking for NULL should be safe) as that is done in RINIT. This new check should fix that as well.

@pdelewski
Copy link
Member

@agoallikmaa Is it possible to test it via unit test? Could you add it?

@agoallikmaa
Copy link
Contributor Author

It does not seem possible to test in the CLI mode that unit tests are run in, as header callback is not called there. Setting up a test environment that uses FPM or apache2handler does not seem trivial.

@pdelewski
Copy link
Member

@agoallikmaa I agree that test env with FPM or apache2handler is not the option. I was thinking about something that will trigger callback, however if that's not possible I'm ok with it.

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.

[opentelemetry-php-instrumentation] - BUG: Laravel Octane + Roadrunner + Opentelemetry Segmentation fault
3 participants