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 sockproc possibly inheriting nginx's sockets/file descriptors #75

Merged
merged 2 commits into from
Jun 18, 2017

Conversation

GUI
Copy link
Collaborator

@GUI GUI commented Jun 18, 2017

When we launch the sockproc process from inside nginx, we explicitly try to clear all extra file descriptors, so nginx doesn't inherit the nginx processes sockets. If sockproc does inherit these sockets, then if nginx gets stopped or restarted, sockproc takes over listening on those sockets, which means nginx can't be started again.

However, the existing implementation of clearing these file descriptors could fail under certain circumstances, leading to nginx being unable to restart. It depends on how nginx is started, but this could reliably fail under this scenario:

  • nginx was started under an init script.
  • nginx had worker processes running as a less privileged user.
  • The user that originally ran the init start command was "cd"ed into a directory that the less privileged nginx worker user didn't have permissions to be in.

This was failing due to the find command being combined with -exec basename to find the file descriptors to clear. basename would fail if the current user didn't have permissions to be in the current directory (even if it was only finding the basename of paths in /proc which it did have permissions to). This led to no file descriptors being cleared. The find command would also generate a warning message after finishing about not being able to restore the initial directory.

To fix this:

  • Our script to start sockproc now explicitly sets the current directory to /, so any commands that require permissions to be in the current directory should work.
  • Extract the basename of the /proc/ paths using bash, instead of shelling out to the basename command.
  • If for any reason, clearing the file descriptors based on /proc/ paths fails, also fallback to the more naive OS x approach in Linux too.

A test has been added to the test suite to test this behavior. Since the Makefile was getting a little unwieldy (particularly with another test that needed all the "worker perms" environment variables), shift the commands used to run the tests into a wrapper shell script to make things a bit easier to manage.

GUI added 2 commits June 17, 2017 19:58
This cleans things up a bit, and takes care of some theoretical issues
if paths had spaces in them.
When we launch the sockproc process from inside nginx, we explicitly try
to clear all extra file descriptors, so nginx doesn't inherit the nginx
processes sockets. If sockproc does inherit these sockets, then if nginx
gets stopped or restarted, sockproc takes over listening on those
sockets, which means nginx can't be started again.

However, the existing implementation of clearing these file descriptors
could fail under certain circumstances, leading to nginx being unable to
restart. It depends on how nginx is started, but this could reliably
fail under this scenario:

- nginx was started under an init script.
- nginx had worker processes running as a less privileged user.
- The user that originally ran the init start command was "cd"ed into a
  directory that the less privileged nginx worker user didn't have
  permissions to be in.

This was failing due to the `find` command being combined with `-exec
basename` to find the file descriptors to clear. `basename` would fail
if the current user didn't have permissions to be in the current
directory (even if it was only finding the basename of paths in /proc
which it did have permissions to). This led to no file descriptors being
cleared. The `find` command would also generate a warning message after
finishing about not being able to restore the initial directory.

To fix this:

- Our script to start sockproc now explicitly sets the current directory
  to `/`, so any commands that require permissions to be in the current
  directory should work.
- Extract the basename of the /proc/ paths using bash, instead of
  shelling out to the `basename` command.
- If for any reason, clearing the file descriptors based on /proc/ paths
  fails, also fallback to the more naive OS x approach in Linux too.

A test has been added to the test suite to test this behavior. Since the
Makefile was getting a little unwieldy (particularly with another test
that needed all the "worker perms" environment variables), shift the
commands used to run the tests into a wrapper shell script to make
things a bit easier to manage.
@GUI GUI merged commit ad1c3df into master Jun 18, 2017
@GUI GUI deleted the sockproc-fds branch June 18, 2017 14:20
@GUI GUI added this to the v0.11.0 milestone Jun 19, 2017
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.

None yet

1 participant