Bug 1988 - ControlPersist causes stderr to be left open until the master connection times out
Summary: ControlPersist causes stderr to be left open until the master connection time...
Status: CLOSED FIXED
Alias: None
Product: Portable OpenSSH
Classification: Unclassified
Component: ssh (show other bugs)
Version: -current
Hardware: All All
: P2 normal
Assignee: Damien Miller
URL:
Keywords:
: 2000 (view as bug list)
Depends on:
Blocks: V_7_3
  Show dependency treegraph
 
Reported: 2012-03-03 10:53 AEDT by Andrew McNabb
Modified: 2022-01-19 10:34 AEDT (History)
8 users (show)

See Also:


Attachments
daemonize backgrounded mux master with controlpersist (580 bytes, patch)
2013-06-07 02:54 AEST, Darren Tucker
djm: ok+
Details | Diff
close ControlPersist stderr when logging to file/syslog or when not in debug mode (1.07 KB, patch)
2016-04-28 23:27 AEST, Damien Miller
no flags Details | Diff
close ControlPersist stderr when logging to file/syslog or when not in debug mode (v2) (1.06 KB, patch)
2016-04-28 23:34 AEST, Damien Miller
dtucker: ok+
Details | Diff
镜像出错总是报出下载yii asset连接超时 (deleted)
2020-06-12 11:25 AEST, huming
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew McNabb 2012-03-03 10:53:21 AEDT
As has been noted previously, when ControlPersist is set, the background process hangs on to stderr until it terminates:

https://bugzilla.mindrot.org/show_bug.cgi?id=1330#c1
https://lwn.net/Articles/401651/

This means that any script that uses stderr from an ssh script will hang until the master connection eventually times out. I'm opening this bug because a user noticed that this makes pssh hang:

http://code.google.com/p/parallel-ssh/issues/detail?id=67

If the background ssh process were to close stderr, then pssh would not hang.
Comment 1 Damien Miller 2012-03-09 10:15:12 AEDT
Closing stderr in the backgrounded process would lose any output from it (usually visiable at loglevel verbose or higher)
Comment 2 Andrew McNabb 2012-03-09 10:46:36 AEDT
(In reply to comment #1)
> Closing stderr in the backgrounded process would lose any output from
> it (usually visiable at loglevel verbose or higher)

But if loglevel is not verbose, there's nothing to lose, right?
Comment 3 Damien Miller 2012-03-30 11:39:32 AEDT
not necessarily - e.g. messages sent at loglevel fatal() or error()
Comment 4 Andrew McNabb 2012-04-01 03:35:25 AEST
If ssh has forked a background daemon, the user can't reasonably expect to see messages on stderr anyway. For example, they might close the terminal in which the original ssh connection was created. Furthermore, such messages might not even be welcome. After all, the user is probably running some other application.

Any fatal errors would be relevant to other foreground ssh processes, but not to a terminal window that is no longer running any ssh processes.

Might the best long-term solution be to send any logging messages to foreground ssh processes? In any case, I think it's incorrect to send logging messages to a now-unrelated tty. Especially since this can make scripts hang.
Comment 5 Darren Tucker 2013-06-07 02:54:24 AEST
Created attachment 2299 [details]
daemonize backgrounded mux master with controlpersist

please try this patch from tedu: daemon'ize the backgrounded mux master.

the new -E options should make the logging more useful in this case.
Comment 6 Damien Miller 2013-06-07 11:40:47 AEST
Comment on attachment 2299 [details]
daemonize backgrounded mux master with controlpersist

>Index: ssh.c
>===================================================================
>RCS file: /cvs/src/usr.bin/ssh/ssh.c,v
>retrieving revision 1.378
>diff -u -p -r1.378 ssh.c
>--- ssh.c	17 May 2013 00:13:14 -0000	1.378
>+++ ssh.c	6 Jun 2013 01:51:58 -0000
>@@ -947,6 +947,8 @@ control_persist_detach(void)
> 		fatal("%s: fork: %s", __func__, strerror(errno));
> 	case 0:
> 		/* Child: master process continues mainloop */
>+		if (daemon(1, 0) == -1)
>+ 			fatal("Failed to daemonize control master");

I'd prefer this happen in the block after the switch statement, where the fd cleanup already occurs.

Otherwise ok by me
Comment 7 Damien Miller 2013-07-25 12:17:07 AEST
Retarget to openssh-6.4
Comment 8 Damien Miller 2013-07-25 12:19:55 AEST
Retarget 6.3 -> 6.4
Comment 9 Darren Tucker 2013-10-10 11:00:49 AEDT
nope, it's actually in 6.3:

revision 1.381
date: 2013/07/25 00:29:10;  author: djm;  state: Exp;  lines: +2 -1;
daemonise backgrounded (ControlPersist'ed) multiplexing master to ensure
it is fully detached from its controlling terminal. based on debugging
and patch from tedu@
ok dtucker@ "be careful" deraadt@


please reopen if you can reproduce the problem with 6.3 or newer.
Comment 10 Jeremy A. 2014-07-13 03:01:36 AEST
(In reply to Darren Tucker from comment #9)
> please reopen if you can reproduce the problem with 6.3 or newer.

I've been able to reproduce this on 6.2 and 6.6.  In those versions (and 6.3 as well), a daemon(1, 1) call is used and stderr is not detached.
Comment 11 Dmitry Malinovsky 2015-08-11 14:55:12 AEST
I can confirm this bug with
$ ssh -V
OpenSSH_6.6.1p1 Ubuntu-2ubuntu2, OpenSSL 1.0.1f 6 Jan 2014
$ cat /etc/issue
Linux Mint 17.2 Rafaela \n \l
Comment 12 Alex Vandiver 2016-02-23 06:00:51 AEDT
I agree with the other recent comments on this ticket; while the patch discussed on this ticket suggested `daemon(1,0)` (which would have adjusted the file descriptors) the commit as it was landed used `daemon(1,1)` instead, which has no effect on the FDs.

It's also not clear why the return code check was omitted.
Comment 13 Damien Miller 2016-04-28 23:15:30 AEST
*** Bug 2000 has been marked as a duplicate of this bug. ***
Comment 14 Damien Miller 2016-04-28 23:27:08 AEST
Created attachment 2810 [details]
close ControlPersist stderr when logging to file/syslog or when not in debug mode

We want to keep stderr open when in debug mode, so it's a little more complex than that. This patch tries to do the right thing.
Comment 15 Christoph Anton Mitterer 2016-04-28 23:32:54 AEST
Seems to also cause quite some troubles in the use case I've described in bug #2000, which Damien just marked a dup of this one.
So people may want to have a look there as well for possibly further information.

Can anyone make some predictions whether this issue can be solved?
It would even be already helpful, when it would offer a special option that cause the FDs to be closed, just in case there are reasons why you don't want to make that the default.


Further, when you have a look at the use case I describe in #2000, it should become obvious that this is done with likely many different versions of OpenSSH (our oldest still in use 5.3, as part of Scientific Linux 6)... do you think it requires just an up to date client (which would be easy for us, as there's one node which opens all the muxes, which anyway runs recent Debian)... or would all servers need to be updated as well?
Comment 16 Damien Miller 2016-04-28 23:34:50 AEST
Created attachment 2811 [details]
close ControlPersist stderr when logging to file/syslog or when not in debug mode (v2)

Same as last patch, but the logic is a little more readable this way
Comment 17 Damien Miller 2016-04-29 18:09:23 AEST
patch applied - this will be in openssh-7.3

commit d2d6bf864e52af8491a60dd507f85b74361f5da3
Author: djm@openbsd.org <djm@openbsd.org>
Date:   Fri Apr 29 08:07:53 2016 +0000

    upstream commit
    
    close ControlPersist background process stderr when not
     in debug mode or when logging to a file or syslog. bz#1988 ok dtucker
    
    Upstream-ID: 4fb726f0fdcb155ad419913cea10dc4afd409d24
Comment 18 Damien Miller 2016-08-02 10:40:43 AEST
Close all resolved bugs after 7.3p1 release
Comment 19 huming 2020-06-12 11:25:02 AEST
Created attachment 3409 [details]
镜像出错总是报出下载yii  asset连接超时
Comment 20 Christoph Anton Mitterer 2022-01-19 10:34:13 AEDT
@Damien... have a look at the previous message... weird form of spam (or some image that tries to abuse a security hole)... guess that should be removed and the user banned.