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

Do not inherit open FDs to SSH child process #2

Merged
merged 1 commit into from
Dec 6, 2018
Merged

Conversation

clue
Copy link
Owner

@clue clue commented Dec 5, 2018

This somewhat obscure PR ensures that we do not inherit open file descriptors (FDs) to the SSH child process. This can cause all sorts of errors in long running applications and really is not desired here.

This is implemented by explicitly closing all superfluous FDs in the implicit sh child process before launching the actual ssh binary. PHP does not support FD_CLOEXEC, O_CLOEXEC or SOCK_CLOEXEC and this appears to be the best work around I could find (yes, I should probably write a lengthy, somewhat technical blog post about this). Additionally, this PR includes a test to verify this works on all supported platforms and this could perhaps be used as a starting point for other libraries (YMMV).

See also reactphp/child-process#51

@clue clue added the new feature New feature or request label Dec 5, 2018
@clue clue added this to the v1.0.0 milestone Dec 5, 2018
$command = $this->cmd . ' -W ' . \escapeshellarg($parts['host'] . ':' . $parts['port']);

// try to get list of all open FDs (Linux only) or simply assume range 3-1024 (FD_SETSIZE)
$fds = @scandir('/proc/self/fd');

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI working on a package to do this cross platform: https://github.com/WyriHaximus/php-file-descriptors

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@WyriHaximus Thank you, will definitely keep an eye on this! 👍

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@clue currently onto the daunting task of figuring it out for Windows...

@clue
Copy link
Owner Author

clue commented Dec 6, 2018

For the reference: This PR ensures that we immediately close all inherited file descriptors before executing the actual ssh child process. This means that there is still a (very short) period where these file descriptors are in fact inherited to the intermediary sh process and could potentially cause problems. While I have not seen any actual issues in my application because of this possible race condition, I still agree that this is something to address in a future version, most likely by taking advantage of reactphp/child-process#65.

@clue clue merged commit 72d70dc into clue:master Dec 6, 2018
@clue clue deleted the fds branch December 6, 2018 11:02
clue added a commit to clue-labs/reactphp-sqlite that referenced this pull request Jan 22, 2019
This somewhat obscure PR ensures that we do not inherit open file
descriptors (FDs) to the SQLite child worker process. This can cause all
sorts of errors in long running applications and really is not desired
here.

This is implemented by explicitly overwriting all superfluous FDs with
dummy file handles and then closing all of these in the implicit `sh`
child process before launching the actual php binary. PHP does not
support `FD_CLOEXEC`, `O_CLOEXEC` or `SOCK_CLOEXEC`  and this appears to
be the best work around I could find (yes, I should probably write a
lengthy, somewhat technical blog post about this). Additionally, this PR
includes a test to verify this works on all supported platforms and this
could perhaps be used as a starting point for other libraries (YMMV).

This builds on top of clue/reactphp-ssh-proxy#2
and clue/reactphp-ssh-proxy#10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants