Bug 3137

Summary: -f keeps stdin and stderr open
Product: Portable OpenSSH Reporter: Mihai Moldovan <ionic>
Component: sshAssignee: Assigned to nobody <unassigned-bugs>
Status: CLOSED FIXED    
Severity: normal CC: djm, mbw
Priority: P5    
Version: 8.2p1   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 3162    

Description Mihai Moldovan 2020-03-16 19:53:00 AEDT 
As a more general case of #1988, the -f flag also seems to keep stdin and stderr open in the general case:

% ssh ionic.de -f -T 'pv -qL 1 /dev/zero'
% ls -ldh /proc/7115/fd/*
lrwx------ 1 ionic ionic 64 Mar 16 09:21 /proc/7115/fd/0 -> /dev/pts/50
l-wx------ 1 ionic ionic 64 Mar 16 09:21 /proc/7115/fd/1 -> /dev/null
lrwx------ 1 ionic ionic 64 Mar 16 09:21 /proc/7115/fd/2 -> /dev/pts/50
lrwx------ 1 ionic ionic 64 Mar 16 09:21 /proc/7115/fd/3 -> 'socket:[1405985948]'
lrwx------ 1 ionic ionic 64 Mar 16 09:21 /proc/7115/fd/5 -> /dev/pts/50
lrwx------ 1 ionic ionic 64 Mar 16 09:21 /proc/7115/fd/6 -> /dev/pts/50


I'd argue that this is unnecessary and harmful because:
  - the channel will dup2() stdin and stderr anyway (see FD 5 and 6)
  - backgrounding usually means that the user intends to close a controlling terminal anyway (or doesn't even have one to begin with), severing the pipes uni-laterally.


Additionally, this effectively lets #1988 resurface when pairing ControlMaster, ControlPersist and the -f flag.

Using the -f flag disables the "normal" ControlPersist code path that closes stderr and daemonizes, and instead uses the other -f code path which does not close stderr. This makes sense to not fork twice, but also means that the fix from #1988 is outright worked around:

% ssh ionic.de -o ControlMaster=yes -o ControlPersist=yes -o ControlPath=/home/ionic/.sshsock -f -N -T 'pv -qL 1 /dev/zero'
% ls -ldh /proc/22122/fd/*
lrwx------ 1 ionic ionic 64 Mar 16 09:45 /proc/22122/fd/0 -> /dev/pts/50
l-wx------ 1 ionic ionic 64 Mar 16 09:45 /proc/22122/fd/1 -> /dev/null
lrwx------ 1 ionic ionic 64 Mar 16 09:45 /proc/22122/fd/2 -> /dev/pts/50
lrwx------ 1 ionic ionic 64 Mar 16 09:45 /proc/22122/fd/3 -> 'socket:[1406165968]'
lrwx------ 1 ionic ionic 64 Mar 16 09:45 /proc/22122/fd/4 -> 'socket:[1406168946]'

Here, no command is actually executed. We're only interested in the control socket, but the forked/backgrounded control socket process retains stdin and stderr.


FWIW, keeping stdin connected/open might be okay, but I don't see any benefit in doing so.

Keeping stderr open might be useful if -v is passed/debugging turned on, but should otherwise be closed like in the "normal" ControlPersist case.
Comment 1 Matt Weatherford 2020-09-19 03:24:08 AEST 
Hi, 
We are interested in getting this fixed for x2go remming integration -  Is more information needed by the OpenSSH folks?  How can we move this forward?  thank you !
Comment 2 Damien Miller 2020-09-22 11:51:24 AEST 
Fixed in commits 0a4a5571 and d14fe25e and will be in the forthcoming 8.4 release
Comment 3 Mihai Moldovan 2020-10-29 12:21:27 AEDT 
Reopening.

d14fe25e is buggy and doesn't actually close stderr.

This line https://github.com/openssh/openssh-portable/commit/d14fe25e6c3b89f8af17e2894046164ac3b45688#diff-ce646b350d7e1b7ca792b15463c7ce19fd0979286424bb3d569e36ab5e435039R1762 should have used STDERR_FILENO. Instead it's just wrongly duping stdout again.
Comment 4 Damien Miller 2020-10-29 17:44:47 AEDT 
Already fixed in 396d32f3a1a16
Comment 5 Mihai Moldovan 2020-10-30 06:34:52 AEDT 
Right, thanks again!
Comment 6 Damien Miller 2021-03-04 09:54:18 AEDT 
close bugs that were resolved in OpenSSH 8.5 release cycle