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

ext/ffi/tests/300.phpt test is failing on Alpine linux #9139

Closed
mvorisek opened this issue Jul 25, 2022 · 15 comments · Fixed by #9473
Closed

ext/ffi/tests/300.phpt test is failing on Alpine linux #9139

mvorisek opened this issue Jul 25, 2022 · 15 comments · Fixed by #9473

Comments

@mvorisek
Copy link
Contributor

mvorisek commented Jul 25, 2022

Description

CI output:

 ========DIFF========
+001+ Mon Jul 25 18:27:07 2022 (140065768821768): Fatal Error "opcache.preload_user" has not been defined
-001- Hello World from PHP!
 ========DONE========
 FAIL FFI 300: FFI preloading [ext/ffi/tests/300.phpt] 

CI: https://github.com/php/php-src/runs/7506106345?check_suite_focus=true#step:10:739

how can be this test fixed?

PHP Version

master, all other FFI tests pass

Operating System

Alpine linux

@cmb69
Copy link
Member

cmb69 commented Jul 26, 2022

This test has a skipif clause which checks the opcache.preload_user is not empty. I have no idea why that doesn't skip the test.

@mvorisek
Copy link
Contributor Author

mvorisek commented Jul 26, 2022

Yes, but it skips the test if the opcache.preload_user is non-empty. @dstogov you are the author of the test, can you please have a look?

@arnaud-lb
Copy link
Member

Looking at the code, this error is triggered when running as root and opcache.preload_user was not defined. According to the doc, opcache.preload_user must be defined when running as root.

We could clarify the error message.

@cmb69
Copy link
Member

cmb69 commented Jul 28, 2022

Would it also be possible to run the tests on Alpine as regular user?

@mvorisek
Copy link
Contributor Author

I searched all occurences of opcache.preload_user in tests and ext/ffi/tests/bug78761.phpt test does not fail. It is because ffi.enable=1 is not defined there?

Would it also be possible to run the tests on Alpine as regular user?

We can. Why is opcache.preload_user actually so dangerous to require non root user? In Docker, it is very common to run everything as root and rely on Docker isolation. So maybe such arbitrary limitation should be removed complete.

@KapitanOczywisty
Copy link

We can. Why is opcache.preload_user actually so dangerous to require non root user?

Seems like opcache.preload_user do not require non-root user.

php-src/.cirrus.yml

Lines 23 to 24 in fdb9e3a

# Specify opcache.preload_user as we're running as root.
- echo opcache.preload_user=root >> /etc/php.d/opcache.ini

@arnaud-lb
Copy link
Member

Adding opcache.preload_user yields a new error:

Fatal error: Uncaught FFI\Exception: FFI::load() doesn't work in conjunction with "opcache.preload_user". Use "ffi.preload" instead.

We should skip this test when ran as root.

I also think that we should execute tests as a non root user.

I searched all occurences of opcache.preload_user in tests and ext/ffi/tests/bug78761.phpt test does not fail. It is because ffi.enable=1 is not defined there?

This test also fails here

@cmb69
Copy link
Member

cmb69 commented Aug 5, 2022

Isn't this resolved via 1c9a49e?

I also think that we should execute tests as a non root user.

ACK

@arnaud-lb
Copy link
Member

With 1c9a49e we get the new error:

Fatal error: Uncaught FFI\Exception: FFI::load() doesn't work in conjunction with "opcache.preload_user". Use "ffi.preload" instead.

That's because opcache.preload_user preloads in a separate process

@mvorisek
Copy link
Contributor Author

mvorisek commented Aug 5, 2022

With 1c9a49e we get the new error:

Fatal error: Uncaught FFI\Exception: FFI::load() doesn't work in conjunction with "opcache.preload_user". Use "ffi.preload" instead.

That's because opcache.preload_user preloads in a separate process

I confirm this issue: https://github.com/php/php-src/runs/7694180097?check_suite_focus=true#step:10:92

Is there anything againt allowing preload as a root (/wo opcache.preload_user)?

@KapitanOczywisty
Copy link

KapitanOczywisty commented Aug 5, 2022

pw = getpwnam(ZCG(accel_directives).preload_user);

Can fork() be skipped entirely when pw->pw_uid == 0? preload_user would be still required for root, just to make sure, but FFI::load() would work on root.

Crude patch:

diff --git a/ext/opcache/ZendAccelerator.c b/ext/opcache/ZendAccelerator.c
--- ext/opcache/ZendAccelerator.c
+++ ext/opcache/ZendAccelerator.c
@@ -4624,8 +4624,12 @@
 				zend_accel_error_noreturn(ACCEL_LOG_FATAL, "Preloading failed to getpwnam(\"%s\")", ZCG(accel_directives).preload_user);
 				return FAILURE;
 			}
 
+			if (pw->pw_uid == 0) {
+				goto skipfork;
+			}
+
 			pid = fork();
 			if (pid == -1) {
 				zend_shared_alloc_unlock();
 				zend_accel_error_noreturn(ACCEL_LOG_FATAL, "Preloading failed to fork()");
@@ -4670,8 +4674,9 @@
 				zend_accel_error(ACCEL_LOG_WARNING, "\"opcache.preload_user\" is ignored");
 			}
 		}
 
+skipfork:
 		sapi_module.activate = NULL;
 		sapi_module.deactivate = NULL;
 		sapi_module.register_server_variables = NULL;
 		sapi_module.header_handler = preload_header_handler;

@arnaud-lb
Copy link
Member

Is there anything againt allowing preload as a root (/wo opcache.preload_user)?

I've tried to look for particular reasons for this. What I've found is that when running Apache mod_php, the function accel_finish_startup is called before Apache switches to an unprivileged user. This is because extension startup happens in the SAPI's main process.

People wouldn't expect their PHP scripts to be loaded/executed as root when they have configured Apache to run as an unprivileged user.

So opcache.preload_user is a workaround for this. However it's easy to miss this fact or to forget about this, which would be a security issue. The fact that opcache.preload is forbidden when the current user is root and opcache.preload_user is unset prevents this: It is not possible to forget about this.

This also explains why the error message is worded this way: "opcache.preload_user" has not been defined.

It's possible that FPM and other SAPIs work in a similar way.

Can fork() be skipped entirely when pw->pw_uid == 0? preload_user would be still required for root, just to make sure, but FFI::load() would work on root.

Yes, I think so

@arnaud-lb
Copy link
Member

For the purpose of #8621, we could run the tests as a non-root user, or skip this test when ran as root.

We could then allow FFI preloading to work a root in a separate PR.

@bukka
Copy link
Member

bukka commented Aug 9, 2022

It would be actually good to have one of the pipeline run as root possibly as for example fpm often runs under root. Currently we have got just one test specific to root but there might be more in the future. Think the best solution is to skip it if run under root.

In terms of opcache preloading and fpm, you get the same issue so that preload_user is needed there to work correctly. However it might still not resolve all of the issues as there can be multiple pools to run under different users and then it might not really work because opcache is shared between pools which is not ideal. You can see some discussion on the related issue in #8072 .

@bwoebi
Copy link
Member

bwoebi commented Sep 6, 2022

People wouldn't expect their PHP scripts to be loaded/executed as root when they have configured Apache to run as an unprivileged user.

In that light, doesn't it make sense to have the check (requiring opcache.preload_user) only for web SAPIs, and not check it on CLI SAPIs - because there you'll anyway run the preloaded code as the executing user, entirely within peoples expectations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants