openssh bugs - Mar 2020 - [Bug 3137] New: -f keeps stdin and stderr open

https://bugzilla.mindrot.org/show_bug.cgi?id=3137

            Bug ID: 3137
           Summary: -f keeps stdin and stderr open
           Product: Portable OpenSSH
           Version: 8.2p1
          Hardware: All
                OS: All
            Status: NEW
          Severity: normal
          Priority: P5
         Component: ssh
          Assignee: unassigned-bugs at mindrot.org
          Reporter: ionic at ionic.de

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.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=3137

Matt Weatherford <mbw at uw.edu> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |mbw at uw.edu

--- Comment #1 from Matt Weatherford <mbw at uw.edu> ---
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 !

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=3137

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |FIXED
                 CC|                            |djm at mindrot.org
             Blocks|                            |3162

--- Comment #2 from Damien Miller <djm at mindrot.org> ---
Fixed in commits 0a4a5571 and d14fe25e and will be in the forthcoming
8.4 release


Referenced Bugs:

https://bugzilla.mindrot.org/show_bug.cgi?id=3162
[Bug 3162] Tracking bug for 8.4 release
-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=3137

Mihai Moldovan <ionic at ionic.de> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |REOPENED
         Resolution|FIXED                       |---

--- Comment #3 from Mihai Moldovan <ionic at ionic.de> ---
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.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=3137

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|REOPENED                    |RESOLVED
         Resolution|---                         |FIXED

--- Comment #4 from Damien Miller <djm at mindrot.org> ---
Already fixed in 396d32f3a1a16

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=3137

--- Comment #5 from Mihai Moldovan <ionic at ionic.de> ---
Right, thanks again!

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=3137

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |CLOSED

--- Comment #6 from Damien Miller <djm at mindrot.org> ---
close bugs that were resolved in OpenSSH 8.5 release cycle

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.