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 GH-9139: Allow FFI in opcache.preload when opcache.preload_user=root #9473

Merged
merged 1 commit into from
Sep 9, 2022

Conversation

arnaud-lb
Copy link
Member

@arnaud-lb arnaud-lb commented Sep 2, 2022

opcache.preload can not be used for FFI preloading when running as root, because of a chain of causes:

  • opcache.preload requires opcache.preload_user when running as root
  • FFI does not work when opcache.preload_user is defined, because the preloading happens in a sub-process in this case

It is not strictly necessary to preload in a sub-process when opcache.preload_user is the current user, because we are not going to change the uid of the process.

This PR skips the fork() when opcache.preload_user is the current user, thus allowing FFI in opcache.preload when running as root, as long as opcache.preload_user=root.

Although running as root is not recommended, it is common practice with docker containers. This change also allows to have at least one CI pipeline running as root.

Fixes #9139

Most changes are whitespaces.

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.

The implementation looks good, although I don't know enough about this code to understand if there are any implications here. Note that the documentation will need to be adjusted.

https://www.php.net/manual/en/opcache.preloading.php

-Running preloading as root is not allowed.
+Running preloading as root is disallowed by default. Set `opcache.preload_user=root` to explicitly allow it.

Or something along those lines.

@cmb69 cmb69 requested a review from dstogov September 5, 2022 13:43
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.

ext/ffi/tests/300.phpt test is failing on Alpine linux
3 participants