Bug 629322

Summary: ssh command closes seems-to-be-unused filedescriptors
Product: [Fedora] Fedora Reporter: Tomáš Bžatek <tbzatek>
Component: opensshAssignee: Jan F. Chadima <jchadima>
Status: CLOSED RAWHIDE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: high Docs Contact:
Priority: high    
Version: rawhideCC: jchadima, mgrepl, tmraz, tsmetana, yaneti
Target Milestone: ---   
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: gvfs-1.6.4-1.fc15 Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2010-09-30 06:04:26 EDT Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Attachments:
Description Flags
strace of the nested ssh command none

Description Tomáš Bžatek 2010-09-01 11:58:07 EDT
Created attachment 442444 [details]
strace of the nested ssh command

Description of problem:
First thing to say, I don't know exactly where the problem is. Experimentally I came to a conclusion that our code is fine and the new openssh does some things that break our attempts to catch all tty output. I've been trying to modify our code, adding workarounds and excessive prints but none of them really revealed the problem.

The ssh command is used in gvfsd-sftp (SFTP backend of GVfs which is basic component of Gnome desktop) to spawn sftp server on remote machine and then communicates with the server by grabbing output of tty, stdout etc. For this we create a pty instance, a pseudo terminal which the ssh command runs in. For some reason we started to get EIO errors when trying to read from pty (on our side).

Things worked perfectly before rebase to openssh-5.6p1, i.e. openssh-5.5p1-18.fc14 works fine. So I started comparing strace outputs and found this:

1283345551.881934 open("/dev/pts/41", O_RDWR) = 3
1283345551.881989 ioctl(3, TIOCSCTTY)   = 0
...
1283345551.883328 execve("/usr/bin/ssh", ["/usr/bin/ssh", "-oNoHostAuthenticationForLocalho"..., "-s", "localhost", "sftp"], [/* 73 vars */]) = 0
...
1283345551.893834 close(3)              = 0

Adding a bunch of useless fds only proved that ssh closes them all on startup -- see attached strace. Probably as a security measure. However it closes other important things.


Version-Release number of selected component (if applicable):
openssh-5.6p1-1.fc15

How reproducible:
always

Steps to Reproduce:
1. Install gvfs, run this in terminal under active Gnome session: 
2. GVFS_DEBUG=1 /usr/libexec/gvfsd-sftp host=localhost
3. Watch the "Mount failed: Error reading from unix: Input/output error" message

Additional info:
If only OpenBSD folks knew what ChangeLog is for...
Comment 1 Tomáš Bžatek 2010-09-01 12:06:07 EDT
(In reply to comment #0)
> Additional info:
> If only OpenBSD folks knew what ChangeLog is for...
Ooops, finally found it :-o)
Comment 2 Tomáš Bžatek 2010-09-01 12:07:39 EDT
> 20100816
>  - OpenBSD CVS Sync
>    - djm@cvs.openbsd.org 2010/08/12 21:49:44
>      [ssh.c]
>      close any extra file descriptors inherited from parent at start and
>      reopen stdin/stdout to /dev/null when forking for ControlPersist.
> 
>      prevents tools that fork and run a captive ssh for communication from
>      failing to exit when the ssh completes while they wait for these fds to
>      close. The inherited fds may persist arbitrarily long if a background
>      mux master has been started by ControlPersist. cvs and scp were effected
>      by this.
> 
>      "please commit" markus@
Comment 3 Jan F. Chadima 2010-09-02 05:50:35 EDT
Is there any way how to work with the new openssh? Or shoul I report this as a bug to openssh authors?
Comment 4 Tomas Mraz 2010-09-02 06:03:16 EDT
Why don't you just ignore the EIO error in gvfs? That ssh closed the fd also means that it will not write anything useful to it. You should just capture the stdout/stderr file descriptors output.
Comment 5 Jan F. Chadima 2010-09-02 06:08:13 EDT
The better way to make sftp is to use libssh as the KDE project does. It is safer way how to do it and it not wastes so much resources
Comment 6 Tomáš Bžatek 2010-09-02 09:12:54 EDT
(In reply to comment #3)
> Is there any way how to work with the new openssh? Or shoul I report this as a
> bug to openssh authors?
I haven't found any new option to work around this new feature. This looks to me like a regression and would be great to discuss with upstream. Thanks.

(In reply to comment #5)
> The better way to make sftp is to use libssh as the KDE project does. It is
> safer way how to do it and it not wastes so much resources
Yes, this is planned, however it means complete gvfs sftp backend rewrite. Better to fix this tiny issue for the moment.

(In reply to comment #4)
> Why don't you just ignore the EIO error in gvfs? That ssh closed the fd also
> means that it will not write anything useful to it. You should just capture the
> stdout/stderr file descriptors output.
Tried that, apparently nothing is written to stdout nor stderr (we pipe them too). From the strace it looks that ssh opens /dev/tty which has just lost, not sure where the data goes then.

When trying to redirect stderr and stdout in bash, I still can see prompt on the terminal. Is there an ssh option not to use local tty? (tried -T and -tt with no luck)
Comment 7 Tomas Mraz 2010-09-02 11:53:20 EDT
That's weird - sftp localhost <cmds >out works perfectly well here. So I still think this is some mistake on your side.
Comment 8 Tomas Mraz 2010-09-02 12:03:23 EDT
Ah, now I understand - you need that for the password prompts. So you must make the pseudo terminal you allocated a controlling terminal for the process that execs the ssh. Then when it opens and communicates with /dev/tty it will go to your end of the pseudoterminal.
Comment 9 Tomáš Bžatek 2010-09-03 08:02:55 EDT
(In reply to comment #8)
> Ah, now I understand - you need that for the password prompts. So you must make
> the pseudo terminal you allocated a controlling terminal for the process that
> execs the ssh. Then when it opens and communicates with /dev/tty it will go to
> your end of the pseudoterminal.
Exactly, we don't use local sftp client, but rather a ssh client to open connection and tunnel SSH_FXP commands through it. Our gvfs backend is built on this and cannot be easily changed to anything else. TTY is only used for password prompt on startup, commands themselves are sent to stdin then, waiting for replies on stdout and stderr.
Comment 10 Tomas Mraz 2010-09-03 08:06:55 EDT
That's no problem, just make the pseudotty you've allocated a proper controlling terminal of the ssh client process and it should work.
Comment 11 Tomáš Bžatek 2010-09-15 12:18:36 EDT
(In reply to comment #10)
> That's no problem, just make the pseudotty you've allocated a proper
> controlling terminal of the ssh client process and it should work.
So I've been digging more into the code, tried to port our code to use openpty(). We basically did the same things ourselves so the behaviour is the same after the port. We also properly set the slave pty as a controlling terminal, checked against the glibc login_tty() code.

Openssh still closes the slave pty fd, which somewhat causes all reads from /dev/tty to fail immediately (should block and wait for data). Using forkpty() is not an option since we fork twice to avoid zombies and also login_tty() closes the pty terminal, leading to broken pipes. Writing a test string to tty_fd in the current code just before execvp() is being properly read on the parent side, so pipes are set up properly.

Any more ideas?
Comment 12 Tomas Mraz 2010-09-15 13:35:40 EDT
But you should not read in your controlling process from the /dev/tty but from the master end of the pty returned by openpty().
Comment 13 Tomáš Bžatek 2010-09-16 05:17:53 EDT
(In reply to comment #12)
> But you should not read in your controlling process from the /dev/tty but from
> the master end of the pty returned by openpty().
Yes of course, we do that and it works before ssh is spawned. Ssh then opens and uses /dev/tty.

Aside from tty, we need to grab stdout and stderr separately, for sftp protocol. But that happens only after authentication, we're not that far yet.
Comment 14 Tomáš Bžatek 2010-09-30 06:04:26 EDT
Alright, we solved this by using poll() instead of select() which can be set up to ignore HUP events.