http://bugzilla.mindrot.org/show_bug.cgi?id=52
dtucker at zip.com.au changed:
What |Removed |Added
----------------------------------------------------------------------------
OtherBugsDependingO| |1274
nThis| |
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
http://bugzilla.mindrot.org/show_bug.cgi?id=52 ------- Comment #33 from djm at mindrot.org 2007-01-21 15:54 ------- (From update of attachment 1215) 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?> 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) 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()>- } 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).> > 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? ------- You are receiving this mail because: ------- You are the assignee for the bug, or are watching the assignee.
http://bugzilla.mindrot.org/show_bug.cgi?id=52 ------- Comment #34 from djm at mindrot.org 2007-01-21 19:52 ------- (In reply to comment #33)> > 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)meh, I meant: && (errno == EINTR || (errno == EAGAIN && !c->detach_close)) ------- You are receiving this mail because: ------- You are the assignee for the bug, or are watching the assignee.
http://bugzilla.mindrot.org/show_bug.cgi?id=52 ------- Comment #35 from djm at mindrot.org 2007-01-21 21:08 ------- Created an attachment (id=1227) --> (http://bugzilla.mindrot.org/attachment.cgi?id=1227&action=view) patch against current 20070121 This patch makes the changes I mentioned in the previous comment, and turns on the immediate close behaviour for ttys only when protocol 2 is in use (it was on for ttys only for protocol 1 already). Testcases for checking tty vs. non-tty behaviour: 1. ssh -1tt host "echo go; (sleep 10; echo done) & exit" 2. ssh -2tt host "echo go; (sleep 10; echo done) & exit" TTY case: should see "go" and an immediate exit 3. ssh -1 host "echo go; (sleep 10; echo done) & exit" 4. ssh -2 host "echo go; (sleep 10; echo done) & exit" No TTY case: should see "go" and then "done" after a 10 second delay. ------- You are receiving this mail because: ------- You are the assignee for the bug, or are watching the assignee.
http://bugzilla.mindrot.org/show_bug.cgi?id=52
djm at mindrot.org changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #1227| |ok?
Flag| |
------- Comment #36 from djm at mindrot.org 2007-01-22 23:30 -------
(From update of attachment 1227)
attachment 1227 tests OK on Linux and OpenSolaris, passing all regress
tests and the tests mentioned in comment 19 and comment 35
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
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.
http://bugzilla.mindrot.org/show_bug.cgi?id=52 ------- Comment #39 from dtucker at zip.com.au 2007-01-23 06:55 ------- (From update of attachment 1227) Is this intended to be a Portable-only change or is it intended to be fed back to OpenBSD too? ------- You are receiving this mail because: ------- You are the assignee for the bug, or are watching the assignee.
http://bugzilla.mindrot.org/show_bug.cgi?id=52 ------- Comment #40 from djm at mindrot.org 2007-01-23 09:00 ------- (In reply to comment #39)> Is this intended to be a Portable-only change or is it intended to be > fed back to OpenBSD too?I think it should be portable-only. OpenBSD has never "hung" at exit, and the patch brings other systems into line with the OpenBSD behaviour. So there is no need for the patch there, and only risk (albeit minor). ------- You are receiving this mail because: ------- You are the assignee for the bug, or are watching the assignee.
http://bugzilla.mindrot.org/show_bug.cgi?id=52
dtucker at zip.com.au changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #1227| |ok+
Flag| |
------- Comment #41 from dtucker at zip.com.au 2007-01-29 00:25 -------
(From update of attachment 1227)
I have tested this on pretty much everything I have (FC6, OpenBSD 4.0,
NetBSD 3.1, AIX 4+5, Solaris 2.5+8, HP-UX 11). I ran both the normal
regress tests and Damien's test case and both passed.
I did note that "ssh server 'sleep 20 & exit'" still hangs
but I think
that's supposed to since the descriptor is still open. Redirecting
stderr and stdout lets the connection close right away.
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
http://bugzilla.mindrot.org/show_bug.cgi?id=52
djm at mindrot.org changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|NEW |RESOLVED
Resolution| |FIXED
------- Comment #42 from tsi at ualberta.ca 2007-01-29 00:39 -------
(In reply to comment #41)> (From update of attachment 1227 [details])
> I have tested this on pretty much everything I have (FC6, OpenBSD 4.0,
> NetBSD 3.1, AIX 4+5, Solaris 2.5+8, HP-UX 11). I ran both the normal
> regress tests and Damien's test case and both passed.
> I did note that "ssh server 'sleep 20 & exit'" still
hangs but I think
> that's supposed to since the descriptor is still open. Redirecting
> stderr and stdout lets the connection close right away.
Yes, that's correct. One of my goals for this change was to make
portable sshd's behaviour in this respect match OpenBSD sshd's.
------- Comment #43 from djm at mindrot.org 2007-01-29 10:17 -------
This patch has been committed and will be in OpenSSH-4.6p1. Thanks to
Marc Aurele La France for the great work in tracking this down and
fixing it!
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
http://bugzilla.mindrot.org/show_bug.cgi?id=52 ------- Comment #44 from senthilkumar_sen at hotpop.com 2007-02-02 16:48 ------- I am under the impression that the latest patch is not fixing the problem in HP-UX 11.11. I run the following to reproduce the problem. # cat test1.ksh #!/usr/bin/ksh while (( 1 )) do sleep 10 done # cat test2.ksh #!/usr/bin/ksh ./test1.ksh & ------- Comment #45 from dtucker at zip.com.au 2007-02-05 17:10 ------- (In reply to comment #44)> #!/usr/bin/ksh > ./test1.ksh &This leaves std{in,out,err} connected. Try redirecting them to /dev/null instead: ./test1/ksh </dev/null >/dev/null 2>&1 ------- Comment #46 from senthilkumar_sen at hotpop.com 2007-02-06 17:42 ------- Yes, but that workaround doesn't need the patch. However, with latest patch, there is no hang when I run test2.ksh and exit from the shell on linux. But it is not the case with HP-UX. So Im wondering what the patch is doing with respect to HP-UX. ------- You are receiving this mail because: ------- You are the assignee for the bug, or are watching the assignee.
http://bugzilla.mindrot.org/show_bug.cgi?id=52
senthilkumar_sen at hotpop.com changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |senthilkumar_sen at hotpop.com
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.