-
Notifications
You must be signed in to change notification settings - Fork 7.8k
Set CLOEXEC on the listen socket when forking FPM children #11708
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
Conversation
4633f53
to
360b165
Compare
Basically, that was a real issue in our project. Not sure if introducing the new function is a good idea - I will be happy to move it somewhere. It is probably worth adding a test. Let me know if you have any idea how |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a known issue and there are some related reports / discussion about this:
- https://bugs.php.net/bug.php?id=67383 - exec() leaks file and socket descriptors to called program (general for all parts of PHP - not good idea)
- https://bugs.php.net/bug.php?id=76067 - system() function call leaks php-fpm listening sockets
- https://bugs.php.net/bug.php?id=80992 - fork() don't close fpm_globals.listening_socket
From re-reading the discussions I was a bit worried about stdin but not really sure why. It would be definitely good to test if it is possible to currently read from stdin in some useful way in the execed script.
Some of the reports also contained some script that could be used for the test so take a look. It would be good to have some sort of test even if it was just Linux specific.
sapi/fpm/fpm/fpm_children.c
Outdated
@@ -167,6 +167,26 @@ struct fpm_child_s *fpm_child_find(pid_t pid) /* {{{ */ | |||
} | |||
/* }}} */ | |||
|
|||
static int fpm_child_cloexec(void) /* {{{ */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: we no longer use {{{
and }}}
for new functions so please remove it.
360b165
to
ad4639b
Compare
Sure, I removed |
It actually looks like the stdin is already set to I feel like it happens in the fpm_stdio_init_main() Master process:
Worker:
|
aa0f8bd
to
4aba136
Compare
ae2c2ac
to
90ac9e0
Compare
Hi @bukka, I just checked the issues you mentioned and it looks like the main concern (listening socket = I added a test and also set CLOEXEC on the accepted socket in the FPM worker (which also leaked). |
Co-authored-by: Jakub Zelenka <[email protected]>
90ac9e0
to
06af583
Compare
@mikhainin Finally got time to properly test it. I have done some modifications in couple of places but in general it was good. All merged now. |
Filed follow-up as the test is borked for Alpinelinux #12077 |
It doesn't look like a good idea to invoke fork() from the FPM worker but sometimes people may do this. If this happens, the socket may leak into the new process and break something.
The socket still can be accessed via
fopren('php://fd/<FD>')
but not sure if we can/should do anything about that