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

Better handling of fds 0/1/2 #1233

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Better handling of fds 0/1/2 #1233

wants to merge 2 commits into from

Conversation

cgull
Copy link
Member

@cgull cgull commented Oct 27, 2022

I ran into this while working on what is now #1232. mosh-server does not adequately handle the problem of standard file descriptors being closed. In my development, er, over two years ago, I ran into a situation where mosh-server had started with fd 2 closed, it opened /dev/urandom on fd 2, and then later reopened fd 2 on /dev/null. Then, when it tried to actually read fd 2 to get entropy, it got 0 bytes instead, and got caught in a never-ending loop involving an exception being caught and the action being retried. I don't remember for sure, but I think that left the Mosh session hung in mosh.

This work does 3 things:

  • makes the CryptoException thrown on failure to read fatal-- there's no point in continuing if we can't get a random seed.
  • Makes sure fds 0/1/2 are open very early, to avoid problems with any file access
  • Improves the reopening of 0/1/2 when becoming a daemon-- if the previous code was run with one of 0/1/2 closed, it would temporarily open /dev/null on the closed fd, dup2() it to 0/1/2, and then close the temporary file. Oops.

I can't now reproduce the hang, unfortunately. Both Mosh and my OSes have changed since hten. I tried to write a test for this issue, but between not being able to reproduce the problem and mosh-server being difficult to keep track of because it double-forks, I've given up for now. Should any of you doubt there's a problem, strace -o strace.log -f mosh-server new 2>&- will convince you fairly quickly. And while I didn't experience any actual crypto failure, even the possibility of confused crypto code is a rather bad smell.

(Plus, we burned a couple of weeks chasing this problem down at $WORK once.)

cgull added 2 commits October 26, 2022 22:09
Discovered when mosh-server is started with a closed stderr,
/dev/urandom is opened on fd 2, and the daemon code reopens fd 2 on
/dev/null.  In that case, mosh-server loops forever throwing a
non-fatal exception.
Fixes unpleasant behavior in mosh-server when, say, stderr is closed.
@achernya
Copy link
Collaborator

@cgull would you mind rebasing this PR?

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.

2 participants