[Bug 52] ssh hangs on exit

[bugzilla-daemon at mindrot.org]

http://bugzilla.mindrot.org/show_bug.cgi?id=52





------- Comment #37 from tsi at ualberta.ca  2007-01-23 03:42 -------
(In reply to comment #33)
> (From update of attachment 1215 [details])
> Sorry for taking so long to review this. Some comments on the diff (I'd
> really like to see this in 4.6p1)

> >Index: channels.c
> >===================================================================
> >RCS file: /usr/local/src/security/openssh/cvs/openssh/channels.c,v
> >retrieving revision 1.248
> >diff -u -p -r1.248 channels.c
> >--- channels.c	30 Aug 2006 01:07:40 -0000	1.248
> >+++ channels.c	27 Nov 2006 03:39:26 -0000
> >@@ -1449,10 +1449,10 @@ channel_handle_rfd(Channel *c, fd_set *r
> > 	int len;
> > 
> > 	if (c->rfd != -1 &&
> >-	    FD_ISSET(c->rfd, readset)) {
> >+	    (c->detach_close || FD_ISSET(c->rfd, readset))) {

> Is c->detach_close used here to chose "session channels that have
> exitied but not yet closed"? or something else?

Yes.

> > 		errno = 0;
> > 		len = read(c->rfd, buf, sizeof(buf));
> >-		if (len < 0 && (errno == EINTR || errno == EAGAIN))
> >+		if (len < 0 && errno == EINTR)

> Can the second condition instead be made:

> && (errno == EINTR || c->detach_close || errno == EAGAIN)

Yes (with your correction from comment #34).

> This would limit the scope of the change to only session channels which
> have already received a SIGCHLD.

> >@@ -1604,11 +1604,11 @@ channel_handle_efd(Channel *c, fd_set *r
> > 				c->local_consumed += len;
> > 			}
> > 		} else if (c->extended_usage == CHAN_EXTENDED_READ &&
> >-		    FD_ISSET(c->efd, readset)) {
> >+		    (c->detach_close || FD_ISSET(c->efd, readset))) {
> > 			len = read(c->efd, buf, sizeof(buf));
> > 			debug2("channel %d: read %d from efd %d",
> > 			    c->self, len, c->efd);
> >-			if (len < 0 && (errno == EINTR || errno == EAGAIN))
> >+			if (len < 0 && errno == EINTR)

> Likewise.

> >Index: serverloop.c
> >===================================================================
> >RCS file: /usr/local/src/security/openssh/cvs/openssh/serverloop.c,v
> >retrieving revision 1.150
> >diff -u -p -r1.150 serverloop.c
> >--- serverloop.c	23 Oct 2006 17:02:41 -0000	1.150
> >+++ serverloop.c	27 Nov 2006 03:37:16 -0000
> >@@ -280,6 +280,7 @@ wait_until_can_do_something(fd_set **rea
> > 	struct timeval tv, *tvp;
> > 	int ret;
> > 	int client_alive_scheduled = 0;
> >+	int program_alive_scheduled = 0;
> > 
> > 	/*
> > 	 * if using client_alive, set the max timeout accordingly,
> >@@ -317,6 +318,7 @@ wait_until_can_do_something(fd_set **rea
> > 		 * the client, try to get some more data from the program.
> > 		 */
> > 		if (packet_not_very_much_data_to_write()) {
> >+			program_alive_scheduled = child_terminated;

> Why not just use child_terminated directly here? Is it because of
> children exiting during select()

Yes, as I infer in comment #32.

> >-	} else if (ret == 0 && client_alive_scheduled)
> >-		client_alive_check();
> >+	} else {
> >+		if (ret == 0 && client_alive_scheduled)
> >+			client_alive_check();
> >+		if (program_alive_scheduled && fdin_is_tty) {
> >+			if (!fdout_eof)
> >+				FD_SET(fdout, *readsetp);
> >+			if (!fderr_eof)
> >+				FD_SET(fderr, *readsetp);
> >+		}
> >+	}

> I think the condition for this if() should include !compat20 (I know
> that fdin_is_tty is never set for !compat20, but you have to hunt for
> it).

Ok.

> > 
> > 	notify_done(*readsetp);
> > }
> >@@ -407,7 +417,7 @@ process_input(fd_set *readset)
> > 	if (!fdout_eof && FD_ISSET(fdout, readset)) {
> > 		errno = 0;
> > 		len = read(fdout, buf, sizeof(buf));
> >-		if (len < 0 && (errno == EINTR || errno == EAGAIN)) {
> >+		if (len < 0 && errno == EINTR) {
> > 			/* do nothing */
> > #ifndef PTY_ZEROREAD
> > 		} else if (len <= 0) {
> >@@ -425,7 +435,7 @@ process_input(fd_set *readset)
> > 	if (!fderr_eof && FD_ISSET(fderr, readset)) {
> > 		errno = 0;
> > 		len = read(fderr, buf, sizeof(buf));
> >-		if (len < 0 && (errno == EINTR || errno == EAGAIN)) {
> >+		if (len < 0 && errno == EINTR) {

> similar comment as for channels.c bit: can we use program_terminated
> here instead of c->detach close?

Yes, but 's/program_terminated/child_terminated/' as you have it in
attachment #1227.


------- Comment #38 from tsi at ualberta.ca  2007-01-23 03:44 -------
(From update of attachment 1227)
This looks good to me.




------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.