Comment 9 for bug 687535

Revision history for this message
Scott James Remnant (Canonical) (canonical-scott) wrote : Re: [Bug 687535] Re: upstart loses track of ssh daemon after reload ssh

Not using "expect fork" of course would mean that ssh would be reported as
running before it was actually listening for connections.

On Mon, Dec 13, 2010 at 10:13 PM, Clint Byrum <email address hidden> wrote:

> In looking at the code in OpenSSH, the -D flag only affects this portion
> of sshd.c, around line 1740:
>
>
> /*
> * If not in debugging mode, and not started from inetd, disconnect
> * from the controlling terminal, and fork. The original process
> * exits.
> */
> if (!(debug_flag || inetd_flag || no_daemon_flag)) {
> #ifdef TIOCNOTTY
> int fd;
> #endif /* TIOCNOTTY */
> if (daemon(0, 0) < 0)
> fatal("daemon() failed: %.200s", strerror(errno));
>
> /* Disconnect from the controlling tty. */
> #ifdef TIOCNOTTY
> fd = open(_PATH_TTY, O_RDWR | O_NOCTTY);
> if (fd >= 0) {
> (void) ioctl(fd, TIOCNOTTY, NULL);
> close(fd);
> }
> #endif /* TIOCNOTTY */
> }
>
> "no_daemon_flag" isn't used anywhere else in the code.
>
> Consequently, this code is why the daemon forks on reload, because it
> re-execs itself, causing this code to be hit. When run with -D, there's
> no call to daemon.
>
> The call to daemon should only chdir to / and close stdin/stdout/stderr,
> redirecting them to /dev/null. Given that upstart will already have done
> that, this is a non issue. The code then ioctl's on stdin to get rid of
> the controlling TTY, which also has no effect when run via upstart since
> it is connected to /dev/null. I believe this would change the behavior
> if we were using console owner, and sshd would exit on control-C in
> console if one was using that, but we're not going to do that, nor would
> be a reason to do that for sshd.
>
> I tested this on natty and maverick, killing the daemon in various ways,
> making sure that user sessions stay alive, and that reloads keep track
> of the daemon. I'm confident that this change is safe and will not cause
> any operational issues with openssh-server.
>
> I've submitted a merge proposal adding -D and dropping expect fork,
> which would fix this bug.
>
> ** Changed in: openssh (Ubuntu)
> Assignee: (unassigned) => Clint Byrum (clint-fewbar)
>
> --
> You received this bug notification because you are a member of Upstart
> Developers, which is subscribed to upstart .
> https://bugs.launchpad.net/bugs/687535
>
> Title:
> upstart loses track of ssh daemon after reload ssh
>
> Status in Upstart:
> New
> Status in “openssh” package in Ubuntu:
> Confirmed
> Status in “openssh” source package in Lucid:
> Confirmed
> Status in “openssh” source package in Maverick:
> Confirmed
>
> Bug description:
> When sshd gets a signal 1 for reload, it forks a new process and ditches
> the old. This causes upstart to believe that ssh has crashed, and loses
> track of it. A second reload (or any other initctl operation on ssh) will
> thus say:
>
> reload: Unknown instance:
>
> There would be 2 ways to fix this:
> 1. Don't have ssh fork on relod, but keep the same pid
> 2. Use a different mechanism in upstart to keep track of ssh. Maybe a pid
> file? Just tracking children of the exited ssh won't work, or it might
> accidentally track a particular session rather than the master, if somebody
> just happens to log in close to reload time.
>
>
> # lsb_release -rd
> Description: Ubuntu 10.04.1 LTS
> Release: 10.04
>
> # dpkg -l openssh-server | cat
> Desired=Unknown/Install/Remove/Purge/Hold
> |
> Status=Not/Inst/Cfg-files/Unpacked/Failed-cfg/Half-inst/trig-aWait/Trig-pend
> |/ Err?=(none)/Reinst-required (Status,Err: uppercase=bad)
> ||/ Name Version
> Description
>
> +++-===========================================-==========================================================-=================================================================
> ii openssh-server 1:5.3p1-3ubuntu4
> secure shell (SSH) server, for secure access
> from remote machines
>
> # dpkg -l upstart
> Desired=Unknown/Install/Remove/Purge/Hold
> |
> Status=Not/Inst/Cfg-files/Unpacked/Failed-cfg/Half-inst/trig-aWait/Trig-pend
> |/ Err?=(none)/Reinst-required (Status,Err: uppercase=bad)
> ||/ Name Version Description
>
> +++-==============-==============-============================================
> ii upstart 0.6.5-7 event-based init daemon
>
>
>