From 7e04c5e277665105cc9fe85bacb8f8e211722f02 Mon Sep 17 00:00:00 2001
From: Matt Johnston <matt@ucc.asn.au>
Date: Sun, 1 Oct 2006 16:35:13 +0000
Subject: [PATCH 01/10] just shuffle some variables names about, a brief
 comment about the "bad writefd" problem

--HG--
branch : channel-fix
extra : convert_revision : f0b407c3d3e047ed83174e6f4ebd85a19352df5b
---
 channel.h         |   9 ++-
 common-channel.c  | 172 +++++++++++++++++++++-------------------------
 options.h         |   4 +-
 svr-chansession.c |   6 +-
 4 files changed, 88 insertions(+), 103 deletions(-)

diff --git a/channel.h b/channel.h
index 7030a0f..fc3cb90 100644
--- a/channel.h
+++ b/channel.h
@@ -73,10 +73,9 @@ struct Channel {
 	circbuffer *extrabuf; /* extended-data for the program - used like writebuf
 					     but for stderr */
 
-	int sentclosed, recvclosed;
-
-	/* this is set when we receive/send a channel eof packet */
-	int recveof, senteof;
+	/* whether close/eof messages have been exchanged */
+	int sent_close, recv_close;
+	int recv_eof, sent_eof;
 
 	int initconn; /* used for TCP forwarding, whether the channel has been
 					 fully initialised */
@@ -94,7 +93,7 @@ struct ChanType {
 	int sepfds; /* Whether this channel has seperate pipes for in/out or not */
 	char *name;
 	int (*inithandler)(struct Channel*);
-	int (*checkclose)(struct Channel*);
+	int (*check_close)(struct Channel*);
 	void (*reqhandler)(struct Channel*);
 	void (*closehandler)(struct Channel*);
 
diff --git a/common-channel.c b/common-channel.c
index 11760ec..f76a79d 100644
--- a/common-channel.c
+++ b/common-channel.c
@@ -47,14 +47,14 @@ static void send_msg_channel_data(struct Channel *channel, int isextended,
 		unsigned int exttype);
 static void send_msg_channel_eof(struct Channel *channel);
 static void send_msg_channel_close(struct Channel *channel);
-static void removechannel(struct Channel *channel);
-static void deletechannel(struct Channel *channel);
-static void checkinitdone(struct Channel *channel);
-static void checkclose(struct Channel *channel);
+static void remove_channel(struct Channel *channel);
+static void delete_channel(struct Channel *channel);
+static void check_in_progress(struct Channel *channel);
+static void check_close(struct Channel *channel);
 
-static void closewritefd(struct Channel * channel);
-static void closereadfd(struct Channel * channel, int fd);
-static void closechanfd(struct Channel *channel, int fd, int how);
+static void close_write_fd(struct Channel * channel);
+static void close_read_fd(struct Channel * channel, int fd);
+static void close_chan_fd(struct Channel *channel, int fd, int how);
 
 #define FD_UNINIT (-2)
 #define FD_CLOSED (-1)
@@ -85,7 +85,7 @@ void chancleanup() {
 	for (i = 0; i < ses.chansize; i++) {
 		if (ses.channels[i] != NULL) {
 			TRACE(("channel %d closing", i))
-			removechannel(ses.channels[i]);
+			remove_channel(ses.channels[i]);
 		}
 	}
 	m_free(ses.channels);
@@ -135,8 +135,8 @@ struct Channel* newchannel(unsigned int remotechan,
 	newchan = (struct Channel*)m_malloc(sizeof(struct Channel));
 	newchan->type = type;
 	newchan->index = i;
-	newchan->sentclosed = newchan->recvclosed = 0;
-	newchan->senteof = newchan->recveof = 0;
+	newchan->sent_close = newchan->recv_close = 0;
+	newchan->sent_eof = newchan->recv_eof = 0;
 
 	newchan->remotechan = remotechan;
 	newchan->transwindow = transwindow;
@@ -203,31 +203,24 @@ void channelio(fd_set *readfds, fd_set *writefds) {
 				send_msg_channel_data(channel, 1, SSH_EXTENDED_DATA_STDERR);
 		}
 
-		/* if we can read from the writefd, it might be closed, so we try to
-		 * see if it has errors */
-		if (IS_DROPBEAR_SERVER && channel->writefd >= 0 
-				&& channel->writefd != channel->readfd
-				&& FD_ISSET(channel->writefd, readfds)) {
+#if 0
+		/* XXX where is this required? */
 			if (channel->initconn) {
 				/* Handling for "in progress" connection - this is needed
 				 * to avoid spinning 100% CPU when we connect to a server
 				 * which doesn't send anything (tcpfwding) */
-				checkinitdone(channel);
+				check_in_progress(channel);
 				continue; /* Important not to use the channel after 
-							 checkinitdone(), as it may be NULL */
+							 check_in_progress(), as it may be NULL */
 			}
-			ret = write(channel->writefd, NULL, 0); /* Fake write */
-			if (ret < 0 && errno != EINTR && errno != EAGAIN) {
-				closewritefd(channel);
-			}
-		}
+#endif
 
 		/* write to program/pipe stdin */
 		if (channel->writefd >= 0 && FD_ISSET(channel->writefd, writefds)) {
 			if (channel->initconn) {
-				checkinitdone(channel);
+				check_in_progress(channel);
 				continue; /* Important not to use the channel after
-							 checkinitdone(), as it may be NULL */
+							 check_in_progress(), as it may be NULL */
 			}
 			writechannel(channel, channel->writefd, channel->writebuf);
 		}
@@ -239,7 +232,7 @@ void channelio(fd_set *readfds, fd_set *writefds) {
 		}
 	
 		/* now handle any of the channel-closing type stuff */
-		checkclose(channel);
+		check_close(channel);
 
 	} /* foreach channel */
 
@@ -250,12 +243,12 @@ void channelio(fd_set *readfds, fd_set *writefds) {
 }
 
 
-/* do all the EOF/close type stuff checking for a channel */
-static void checkclose(struct Channel *channel) {
+/* EOF/close handling */
+static void check_close(struct Channel *channel) {
 
-	TRACE(("checkclose: writefd %d, readfd %d, errfd %d, sentclosed %d, recvclosed %d",
+	TRACE(("check_close: writefd %d, readfd %d, errfd %d, sent_close %d, recv_close %d",
 				channel->writefd, channel->readfd,
-				channel->errfd, channel->sentclosed, channel->recvclosed))
+				channel->errfd, channel->sent_close, channel->recv_close))
 	TRACE(("writebuf size %d extrabuf ptr 0x%x extrabuf size %d",
 				cbuf_getused(channel->writebuf),
 				channel->writebuf,
@@ -263,21 +256,21 @@ static void checkclose(struct Channel *channel) {
 
 	/* server chansession channels are special, since readfd mightn't
 	 * close in the case of "sleep 4 & echo blah" until the sleep is up */
-	if (channel->type->checkclose) {
-		if (channel->type->checkclose(channel)) {
-			closewritefd(channel);
-			closereadfd(channel, channel->readfd);
-			closereadfd(channel, channel->errfd);
+	if (channel->type->check_close) {
+		if (channel->type->check_close(channel)) {
+			close_write_fd(channel);
+			close_read_fd(channel, channel->readfd);
+			close_read_fd(channel, channel->errfd);
 		}
 	}
 
-	if (!channel->senteof
+	if (!channel->sent_eof
 		&& channel->readfd == FD_CLOSED 
 		&& (channel->extrabuf != NULL || channel->errfd == FD_CLOSED)) {
 		send_msg_channel_eof(channel);
 	}
 
-	if (!channel->sentclosed
+	if (!channel->sent_close
 		&& channel->writefd == FD_CLOSED
 		&& channel->readfd == FD_CLOSED
 		&& (channel->extrabuf != NULL || channel->errfd == FD_CLOSED)) {
@@ -294,12 +287,12 @@ static void checkclose(struct Channel *channel) {
 	 * SSH_MSG_CHANNEL_EOF. 
 	 * (from draft-ietf-secsh-connect)
 	 */
-	if (channel->recvclosed) {
-		if (! channel->sentclosed) {
+	if (channel->recv_close) {
+		if (! channel->sent_close) {
 			TRACE(("Sending MSG_CHANNEL_CLOSE in response to same."))
 			send_msg_channel_close(channel);
 		}
-		removechannel(channel);
+		remove_channel(channel);
 	}
 }
 
@@ -308,26 +301,26 @@ static void checkclose(struct Channel *channel) {
  * if so, set up the channel properly. Otherwise, the channel is cleaned up, so
  * it is important that the channel reference isn't used after a call to this
  * function */
-static void checkinitdone(struct Channel *channel) {
+static void check_in_progress(struct Channel *channel) {
 
 	int val;
 	socklen_t vallen = sizeof(val);
 
-	TRACE(("enter checkinitdone"))
+	TRACE(("enter check_in_progress"))
 
 	if (getsockopt(channel->writefd, SOL_SOCKET, SO_ERROR, &val, &vallen)
 			|| val != 0) {
 		send_msg_channel_open_failure(channel->remotechan,
 				SSH_OPEN_CONNECT_FAILED, "", "");
 		close(channel->writefd);
-		deletechannel(channel);
-		TRACE(("leave checkinitdone: fail"))
+		delete_channel(channel);
+		TRACE(("leave check_in_progress: fail"))
 	} else {
 		send_msg_channel_open_confirmation(channel, channel->recvwindow,
 				channel->recvmaxpacket);
 		channel->readfd = channel->writefd;
 		channel->initconn = 0;
-		TRACE(("leave checkinitdone: success"))
+		TRACE(("leave check_in_progress: success"))
 	}
 }
 
@@ -337,7 +330,6 @@ static void checkinitdone(struct Channel *channel) {
 static void send_msg_channel_close(struct Channel *channel) {
 
 	TRACE(("enter send_msg_channel_close"))
-	/* XXX server */
 	if (channel->type->closehandler) {
 		channel->type->closehandler(channel);
 	}
@@ -349,8 +341,8 @@ static void send_msg_channel_close(struct Channel *channel) {
 
 	encrypt_packet();
 
-	channel->senteof = 1;
-	channel->sentclosed = 1;
+	channel->sent_eof = 1;
+	channel->sent_close = 1;
 	TRACE(("leave send_msg_channel_close"))
 }
 
@@ -365,7 +357,7 @@ static void send_msg_channel_eof(struct Channel *channel) {
 
 	encrypt_packet();
 
-	channel->senteof = 1;
+	channel->sent_eof = 1;
 
 	TRACE(("leave send_msg_channel_eof"))
 }
@@ -387,7 +379,7 @@ static void writechannel(struct Channel* channel, int fd, circbuffer *cbuf) {
 		if (len < 0 && errno != EINTR) {
 			/* no more to write - we close it even if the fd was stderr, since
 			 * that's a nasty failure too */
-			closewritefd(channel);
+			close_write_fd(channel);
 		}
 		TRACE(("leave writechannel: len <= 0"))
 		return;
@@ -396,10 +388,10 @@ static void writechannel(struct Channel* channel, int fd, circbuffer *cbuf) {
 	cbuf_incrread(cbuf, len);
 	channel->recvdonelen += len;
 
-	if (fd == channel->writefd && cbuf_getused(cbuf) == 0 && channel->recveof) { 
+	if (fd == channel->writefd && cbuf_getused(cbuf) == 0 && channel->recv_eof) { 
 		/* Check if we're closing up */
-		closewritefd(channel);
-		TRACE(("leave writechannel: recveof set"))
+		close_write_fd(channel);
+		TRACE(("leave writechannel: recv_eof set"))
 		return;
 	}
 
@@ -446,19 +438,6 @@ void setchannelfds(fd_set *readfds, fd_set *writefds) {
 			}
 		}
 
-		TRACE(("writefd = %d, readfd %d, errfd %d, bufused %d", 
-					channel->writefd, channel->readfd,
-					channel->errfd,
-					cbuf_getused(channel->writebuf) ))
-
-		/* For checking FD status (ie closure etc) - we don't actually
-		 * read data from writefd. We don't want to do this for the client,
-		 * since redirection to /dev/null will make it spin in the select */
-		if (IS_DROPBEAR_SERVER && channel->writefd >= 0 
-				&& channel->writefd != channel->readfd) {
-			FD_SET(channel->writefd, readfds);
-		}
-
 		/* Stuff from the wire */
 		if ((channel->writefd >= 0 && cbuf_getused(channel->writebuf) > 0 )
 				|| channel->initconn) {
@@ -492,11 +471,11 @@ void recv_msg_channel_eof() {
 		dropbear_exit("EOF for unknown channel");
 	}
 
-	channel->recveof = 1;
+	channel->recv_eof = 1;
 	if (cbuf_getused(channel->writebuf) == 0
 			&& (channel->extrabuf == NULL 
 					|| cbuf_getused(channel->extrabuf) == 0)) {
-		closewritefd(channel);
+		close_write_fd(channel);
 	}
 
 	TRACE(("leave recv_msg_channel_eof"))
@@ -516,11 +495,11 @@ void recv_msg_channel_close() {
 		dropbear_exit("Close for unknown channel");
 	}
 
-	channel->recveof = 1;
-	channel->recvclosed = 1;
+	channel->recv_eof = 1;
+	channel->recv_close = 1;
 
-	if (channel->sentclosed) {
-		removechannel(channel);
+	if (channel->sent_close) {
+		remove_channel(channel);
 	}
 
 	TRACE(("leave recv_msg_channel_close"))
@@ -528,9 +507,9 @@ void recv_msg_channel_close() {
 
 /* Remove a channel entry, this is only executed after both sides have sent
  * channel close */
-static void removechannel(struct Channel * channel) {
+static void remove_channel(struct Channel * channel) {
 
-	TRACE(("enter removechannel"))
+	TRACE(("enter remove_channel"))
 	TRACE(("channel index is %d", channel->index))
 
 	cbuf_free(channel->writebuf);
@@ -550,13 +529,13 @@ static void removechannel(struct Channel * channel) {
 
 	channel->typedata = NULL;
 
-	deletechannel(channel);
+	delete_channel(channel);
 
-	TRACE(("leave removechannel"))
+	TRACE(("leave remove_channel"))
 }
 
 /* Remove a channel entry */
-static void deletechannel(struct Channel *channel) {
+static void delete_channel(struct Channel *channel) {
 
 	ses.channels[channel->index] = NULL;
 	m_free(channel);
@@ -607,7 +586,7 @@ static void send_msg_channel_data(struct Channel *channel, int isextended,
 
 	CHECKCLEARTOWRITE();
 
-	dropbear_assert(!channel->sentclosed);
+	dropbear_assert(!channel->sent_close);
 
 	if (isextended) {
 		fd = channel->errfd;
@@ -634,7 +613,7 @@ static void send_msg_channel_data(struct Channel *channel, int isextended,
 	if (len <= 0) {
 		/* on error/eof, send eof */
 		if (len == 0 || errno != EINTR) {
-			closereadfd(channel, fd);
+			close_read_fd(channel, fd);
 		}
 		buf_free(buf);
 		buf = NULL;
@@ -687,10 +666,19 @@ void common_recv_msg_channel_data(struct Channel *channel, int fd,
 
 	TRACE(("enter recv_msg_channel_data"))
 
-	if (channel->recveof) {
+	if (channel->recv_eof) {
 		dropbear_exit("received data after eof");
 	}
 
+	/* XXX this is getting hit by people - maybe should be
+	 * made less fatal (or just fixed). 
+	 *
+	 * The most likely explanation is that the socket is being closed (due to
+	 * write failure etc) but the far end doesn't know yet, so keeps sending
+	 * packets.  We already know that the channel number that was given was
+	 * valid, so probably should skip out here. Maybe
+	 * assert(channel->sent_close), thuogh only if the close-on-failure code is
+	 * doing that */
  	if (fd < 0) {
 		dropbear_exit("received data with bad writefd");
 	}
@@ -767,7 +755,6 @@ static void send_msg_channel_window_adjust(struct Channel* channel,
 }
 	
 /* Handle a new channel request, performing any channel-type-specific setup */
-/* XXX server */
 void recv_msg_channel_open() {
 
 	unsigned char *type;
@@ -830,7 +817,7 @@ void recv_msg_channel_open() {
 				goto cleanup;
 			}
 			errtype = ret;
-			deletechannel(channel);
+			delete_channel(channel);
 			TRACE(("inithandler returned failure %d", ret))
 			goto failure;
 		}
@@ -982,7 +969,7 @@ void recv_msg_channel_open_confirmation() {
 	if (channel->type->inithandler) {
 		ret = channel->type->inithandler(channel);
 		if (ret > 0) {
-			removechannel(channel);
+			remove_channel(channel);
 			TRACE(("inithandler returned failure %d", ret))
 		}
 	}
@@ -1006,34 +993,33 @@ void recv_msg_channel_open_failure() {
 	}
 	channel->await_open = 0;
 
-	removechannel(channel);
+	remove_channel(channel);
 }
 #endif /* USING_LISTENERS */
 
 /* close a stdout/stderr fd */
-static void closereadfd(struct Channel * channel, int fd) {
+static void close_read_fd(struct Channel * channel, int fd) {
 
 	/* don't close it if it is the same as writefd,
 	 * unless writefd is already set -1 */
-	TRACE(("enter closereadfd"))
-	closechanfd(channel, fd, 0);
-	TRACE(("leave closereadfd"))
+	TRACE(("enter close_read_fd"))
+	close_chan_fd(channel, fd, 0);
+	TRACE(("leave close_read_fd"))
 }
 
 /* close a stdin fd */
-static void closewritefd(struct Channel * channel) {
+static void close_write_fd(struct Channel * channel) {
 
-	TRACE(("enter closewritefd"))
-	closechanfd(channel, channel->writefd, 1);
-	TRACE(("leave closewritefd"))
+	TRACE(("enter close_write_fd"))
+	close_chan_fd(channel, channel->writefd, 1);
+	TRACE(("leave close_write_fd"))
 }
 
 /* close a fd, how is 0 for stdout/stderr, 1 for stdin */
-static void closechanfd(struct Channel *channel, int fd, int how) {
+static void close_chan_fd(struct Channel *channel, int fd, int how) {
 
 	int closein = 0, closeout = 0;
 
-	/* XXX server */
 	if (channel->type->sepfds) {
 		TRACE(("shutdown((%d), %d)", fd, how))
 		shutdown(fd, how);
diff --git a/options.h b/options.h
index 886876f..ad82f8b 100644
--- a/options.h
+++ b/options.h
@@ -127,8 +127,8 @@ etc) slower (perhaps by 50%). Recommended for most small systems. */
  * but there's an interface via a PAM module - don't bother using it otherwise.
  * You can't enable both PASSWORD and PAM. */
 
-/*#define ENABLE_SVR_PASSWORD_AUTH*/
-#define ENABLE_SVR_PAM_AUTH 
+#define ENABLE_SVR_PASSWORD_AUTH
+/*#define ENABLE_SVR_PAM_AUTH */
 #define ENABLE_SVR_PUBKEY_AUTH
 
 #define ENABLE_CLI_PASSWORD_AUTH
diff --git a/svr-chansession.c b/svr-chansession.c
index ea23fd3..03590b7 100644
--- a/svr-chansession.c
+++ b/svr-chansession.c
@@ -59,14 +59,14 @@ static void send_msg_chansess_exitstatus(struct Channel * channel,
 		struct ChanSess * chansess);
 static void send_msg_chansess_exitsignal(struct Channel * channel,
 		struct ChanSess * chansess);
-static int sesscheckclose(struct Channel *channel);
+static int sess_check_close(struct Channel *channel);
 static void get_termmodes(struct ChanSess *chansess);
 
 
 /* required to clear environment */
 extern char** environ;
 
-static int sesscheckclose(struct Channel *channel) {
+static int sess_check_close(struct Channel *channel) {
 	return channel->writefd == -1;
 }
 
@@ -967,7 +967,7 @@ const struct ChanType svrchansess = {
 	0, /* sepfds */
 	"session", /* name */
 	newchansess, /* inithandler */
-	sesscheckclose, /* checkclosehandler */
+	sess_check_close, /* checkclosehandler */
 	chansessionrequest, /* reqhandler */
 	closechansess, /* closehandler */
 };

From df57eb382479e5719713c9cc02c2f06882f9b9a7 Mon Sep 17 00:00:00 2001
From: Matt Johnston <matt@ucc.asn.au>
Date: Mon, 2 Oct 2006 16:34:06 +0000
Subject: [PATCH 02/10] Rearranged some more bits, marked some areas that need
 work.

* send_msg_channel_data() no longer allocates a separate buffer
* getchannel() handles unknown channels so callers don't have to

--HG--
branch : channel-fix
extra : convert_revision : 3db645581be0fbb0d2ac8d218fbd55e096cbbbe5
---
 cli-channel.c     |   3 -
 common-channel.c  | 150 +++++++++++++++++++++-------------------------
 svr-chansession.c |   7 +--
 3 files changed, 68 insertions(+), 92 deletions(-)

diff --git a/cli-channel.c b/cli-channel.c
index 1bd49ab..b88e913 100644
--- a/cli-channel.c
+++ b/cli-channel.c
@@ -39,9 +39,6 @@ void recv_msg_channel_extended_data() {
 	TRACE(("enter recv_msg_channel_extended_data"))
 
 	channel = getchannel();
-	if (channel == NULL) {
-		dropbear_exit("Unknown channel");
-	}
 
 	if (channel->type != &clichansess) {
 		TRACE(("leave recv_msg_channel_extended_data: chantype is wrong"))
diff --git a/common-channel.c b/common-channel.c
index f76a79d..a6e3364 100644
--- a/common-channel.c
+++ b/common-channel.c
@@ -164,24 +164,33 @@ struct Channel* newchannel(unsigned int remotechan,
 }
 
 /* Returns the channel structure corresponding to the channel in the current
- * data packet (ses.payload must be positioned appropriately) */
-struct Channel* getchannel() {
+ * data packet (ses.payload must be positioned appropriately).
+ * A valid channel is always returns, it will fail fatally with an unknown
+ * channel */
+static struct Channel* getchannel_msg(const char* kind) {
 
 	unsigned int chan;
 
 	chan = buf_getint(ses.payload);
 	if (chan >= ses.chansize || ses.channels[chan] == NULL) {
-		return NULL;
+		if (kind) {
+			dropbear_exit("%s for unknown channel %d", kind, chan);
+		} else {
+			dropbear_exit("Unknown channel %d", chan);
+		}
 	}
 	return ses.channels[chan];
 }
 
+struct Channel* getchannel() {
+	return getchannel_msg(NULL);
+}
+
 /* Iterate through the channels, performing IO if available */
 void channelio(fd_set *readfds, fd_set *writefds) {
 
 	struct Channel *channel;
 	unsigned int i;
-	int ret;
 
 	/* iterate through all the possible channels */
 	for (i = 0; i < ses.chansize; i++) {
@@ -218,6 +227,7 @@ void channelio(fd_set *readfds, fd_set *writefds) {
 		/* write to program/pipe stdin */
 		if (channel->writefd >= 0 && FD_ISSET(channel->writefd, writefds)) {
 			if (channel->initconn) {
+				/* XXX could this go somewhere cleaner? */
 				check_in_progress(channel);
 				continue; /* Important not to use the channel after
 							 check_in_progress(), as it may be NULL */
@@ -254,6 +264,16 @@ static void check_close(struct Channel *channel) {
 				channel->writebuf,
 				channel->writebuf ? 0 : cbuf_getused(channel->extrabuf)))
 
+	/* XXX not good, doesn't flush out */
+	if (channel->recv_close) {
+		if (! channel->sent_close) {
+			TRACE(("Sending MSG_CHANNEL_CLOSE in response to same."))
+			send_msg_channel_close(channel);
+		}
+		remove_channel(channel);
+		return;
+	}
+
 	/* server chansession channels are special, since readfd mightn't
 	 * close in the case of "sleep 4 & echo blah" until the sleep is up */
 	if (channel->type->check_close) {
@@ -277,6 +297,14 @@ static void check_close(struct Channel *channel) {
 		send_msg_channel_close(channel);
 	}
 
+	/* XXX blah */
+	if (channel->recv_eof &&
+		(cbuf_getused(channel->writebuf) == 0
+			&& (channel->extrabuf == NULL 
+					|| cbuf_getused(channel->extrabuf) == 0))) {
+		close_write_fd(channel);
+	}
+
 	/* When either party wishes to terminate the channel, it sends
 	 * SSH_MSG_CHANNEL_CLOSE.  Upon receiving this message, a party MUST
 	 * send back a SSH_MSG_CHANNEL_CLOSE unless it has already sent this
@@ -287,13 +315,6 @@ static void check_close(struct Channel *channel) {
 	 * SSH_MSG_CHANNEL_EOF. 
 	 * (from draft-ietf-secsh-connect)
 	 */
-	if (channel->recv_close) {
-		if (! channel->sent_close) {
-			TRACE(("Sending MSG_CHANNEL_CLOSE in response to same."))
-			send_msg_channel_close(channel);
-		}
-		remove_channel(channel);
-	}
 }
 
 
@@ -325,7 +346,6 @@ static void check_in_progress(struct Channel *channel) {
 }
 
 
-
 /* Send the close message and set the channel as closed */
 static void send_msg_channel_close(struct Channel *channel) {
 
@@ -341,6 +361,7 @@ static void send_msg_channel_close(struct Channel *channel) {
 
 	encrypt_packet();
 
+	/* XXX is setting sent_eof required? */
 	channel->sent_eof = 1;
 	channel->sent_close = 1;
 	TRACE(("leave send_msg_channel_close"))
@@ -388,13 +409,6 @@ static void writechannel(struct Channel* channel, int fd, circbuffer *cbuf) {
 	cbuf_incrread(cbuf, len);
 	channel->recvdonelen += len;
 
-	if (fd == channel->writefd && cbuf_getused(cbuf) == 0 && channel->recv_eof) { 
-		/* Check if we're closing up */
-		close_write_fd(channel);
-		TRACE(("leave writechannel: recv_eof set"))
-		return;
-	}
-
 	/* Window adjust handling */
 	if (channel->recvdonelen >= RECV_WINDOWEXTEND) {
 		/* Set it back to max window */
@@ -408,7 +422,6 @@ static void writechannel(struct Channel* channel, int fd, circbuffer *cbuf) {
 	dropbear_assert(channel->extrabuf == NULL ||
 			channel->recvwindow <= cbuf_getavail(channel->extrabuf));
 	
-	
 	TRACE(("leave writechannel"))
 }
 
@@ -466,18 +479,11 @@ void recv_msg_channel_eof() {
 
 	TRACE(("enter recv_msg_channel_eof"))
 
-	channel = getchannel();
-	if (channel == NULL) {
-		dropbear_exit("EOF for unknown channel");
-	}
+	channel = getchannel_msg("EOF");
 
 	channel->recv_eof = 1;
-	if (cbuf_getused(channel->writebuf) == 0
-			&& (channel->extrabuf == NULL 
-					|| cbuf_getused(channel->extrabuf) == 0)) {
-		close_write_fd(channel);
-	}
 
+	check_close(channel);
 	TRACE(("leave recv_msg_channel_eof"))
 }
 
@@ -489,19 +495,13 @@ void recv_msg_channel_close() {
 
 	TRACE(("enter recv_msg_channel_close"))
 
-	channel = getchannel();
-	if (channel == NULL) {
-		/* disconnect ? */
-		dropbear_exit("Close for unknown channel");
-	}
+	channel = getchannel_msg("Close");
 
+	/* XXX eof required? */
 	channel->recv_eof = 1;
 	channel->recv_close = 1;
 
-	if (channel->sent_close) {
-		remove_channel(channel);
-	}
-
+	check_close(channel);
 	TRACE(("leave recv_msg_channel_close"))
 }
 
@@ -512,6 +512,9 @@ static void remove_channel(struct Channel * channel) {
 	TRACE(("enter remove_channel"))
 	TRACE(("channel index is %d", channel->index))
 
+	/* XXX shuold we assert for sent_closed and recv_closed?
+	 * but we also cleanup manually, maybe we need a flag. */
+
 	cbuf_free(channel->writebuf);
 	channel->writebuf = NULL;
 
@@ -522,7 +525,7 @@ static void remove_channel(struct Channel * channel) {
 
 
 	/* close the FDs in case they haven't been done
-	 * yet (ie they were shutdown etc */
+	 * yet (they might have been shutdown etc) */
 	close(channel->writefd);
 	close(channel->readfd);
 	close(channel->errfd);
@@ -553,10 +556,6 @@ void recv_msg_channel_request() {
 	TRACE(("enter recv_msg_channel_request"))
 	
 	channel = getchannel();
-	if (channel == NULL) {
-		/* disconnect ? */
-		dropbear_exit("Unknown channel");
-	}
 
 	if (channel->type->reqhandler) {
 		channel->type->reqhandler(channel);
@@ -576,9 +575,8 @@ void recv_msg_channel_request() {
 static void send_msg_channel_data(struct Channel *channel, int isextended,
 		unsigned int exttype) {
 
-	buffer *buf;
 	int len;
-	unsigned int maxlen;
+	size_t maxlen, size_pos;
 	int fd;
 
 /*	TRACE(("enter send_msg_channel_data"))
@@ -600,40 +598,37 @@ static void send_msg_channel_data(struct Channel *channel, int isextended,
 	 * exttype if is extended */
 	maxlen = MIN(maxlen, 
 			ses.writepayload->size - 1 - 4 - 4 - (isextended ? 4 : 0));
+	TRACE(("maxlen %d", maxlen))
 	if (maxlen == 0) {
 		TRACE(("leave send_msg_channel_data: no window"))
-		return; /* the data will get written later */
-	}
-
-	/* read the data */
-	TRACE(("maxlen %d", maxlen))
-	buf = buf_new(maxlen);
-	TRACE(("buf pos %d data %x", buf->pos, buf->data))
-	len = read(fd, buf_getwriteptr(buf, maxlen), maxlen);
-	if (len <= 0) {
-		/* on error/eof, send eof */
-		if (len == 0 || errno != EINTR) {
-			close_read_fd(channel, fd);
-		}
-		buf_free(buf);
-		buf = NULL;
-		TRACE(("leave send_msg_channel_data: read err or EOF for fd %d", 
-					channel->index));
 		return;
 	}
-	buf_incrlen(buf, len);
 
 	buf_putbyte(ses.writepayload, 
 			isextended ? SSH_MSG_CHANNEL_EXTENDED_DATA : SSH_MSG_CHANNEL_DATA);
 	buf_putint(ses.writepayload, channel->remotechan);
-
 	if (isextended) {
 		buf_putint(ses.writepayload, exttype);
 	}
+	/* a dummy size first ...*/
+	size_pos = ses.writepayload->pos;
+	buf_putint(ses.writepayload, 0);
 
-	buf_putstring(ses.writepayload, buf_getptr(buf, len), len);
-	buf_free(buf);
-	buf = NULL;
+	/* read the data */
+	len = read(fd, buf_getwriteptr(ses.writepayload, maxlen), maxlen);
+	if (len <= 0) {
+		if (len == 0 || errno != EINTR) {
+			close_read_fd(channel, fd);
+		}
+		ses.writepayload->len = ses.writepayload->pos = 0;
+		TRACE(("leave send_msg_channel_data: read err or EOF for fd %d", 
+					channel->index));
+		return;
+	}
+	buf_incrwritepos(ses.writepayload, len);
+	/* ... real size here */
+	buf_setpos(ses.writepayload, size_pos);
+	buf_putint(ses.writepayload, len);
 
 	channel->transwindow -= len;
 
@@ -647,9 +642,6 @@ void recv_msg_channel_data() {
 	struct Channel *channel;
 
 	channel = getchannel();
-	if (channel == NULL) {
-		dropbear_exit("Unknown channel");
-	}
 
 	common_recv_msg_channel_data(channel, channel->writefd, channel->writebuf);
 }
@@ -726,9 +718,6 @@ void recv_msg_channel_window_adjust() {
 	unsigned int incr;
 	
 	channel = getchannel();
-	if (channel == NULL) {
-		dropbear_exit("Unknown channel");
-	}
 	
 	incr = buf_getint(ses.payload);
 	TRACE(("received window increment %d", incr))
@@ -786,6 +775,7 @@ void recv_msg_channel_open() {
 	/* Get the channel type. Client and server style invokation will set up a
 	 * different list for ses.chantypes at startup. We just iterate through
 	 * this list and find the matching name */
+	/* XXX fugly */
 	for (cp = &ses.chantypes[0], chantype = (*cp); 
 			chantype != NULL;
 			cp++, chantype = (*cp)) {
@@ -811,11 +801,11 @@ void recv_msg_channel_open() {
 
 	if (channel->type->inithandler) {
 		ret = channel->type->inithandler(channel);
+		if (ret == SSH_OPEN_IN_PROGRESS) {
+			/* We'll send the confirmation later */
+			goto cleanup;
+		}
 		if (ret > 0) {
-			if (ret == SSH_OPEN_IN_PROGRESS) {
-				/* We'll send the confirmation later */
-				goto cleanup;
-			}
 			errtype = ret;
 			delete_channel(channel);
 			TRACE(("inithandler returned failure %d", ret))
@@ -949,9 +939,6 @@ void recv_msg_channel_open_confirmation() {
 	TRACE(("enter recv_msg_channel_open_confirmation"))
 
 	channel = getchannel();
-	if (channel == NULL) {
-		dropbear_exit("Unknown channel");
-	}
 
 	if (!channel->await_open) {
 		dropbear_exit("unexpected channel reply");
@@ -984,9 +971,6 @@ void recv_msg_channel_open_failure() {
 	struct Channel * channel;
 
 	channel = getchannel();
-	if (channel == NULL) {
-		dropbear_exit("Unknown channel");
-	}
 
 	if (!channel->await_open) {
 		dropbear_exit("unexpected channel reply");
diff --git a/svr-chansession.c b/svr-chansession.c
index 03590b7..57ec118 100644
--- a/svr-chansession.c
+++ b/svr-chansession.c
@@ -59,17 +59,12 @@ static void send_msg_chansess_exitstatus(struct Channel * channel,
 		struct ChanSess * chansess);
 static void send_msg_chansess_exitsignal(struct Channel * channel,
 		struct ChanSess * chansess);
-static int sess_check_close(struct Channel *channel);
 static void get_termmodes(struct ChanSess *chansess);
 
 
 /* required to clear environment */
 extern char** environ;
 
-static int sess_check_close(struct Channel *channel) {
-	return channel->writefd == -1;
-}
-
 /* Handler for childs exiting, store the state for return to the client */
 
 /* There's a particular race we have to watch out for: if the forked child
@@ -967,7 +962,7 @@ const struct ChanType svrchansess = {
 	0, /* sepfds */
 	"session", /* name */
 	newchansess, /* inithandler */
-	sess_check_close, /* checkclosehandler */
+	NULL, /* checkclosehandler */
 	chansessionrequest, /* reqhandler */
 	closechansess, /* closehandler */
 };

From 786ea39ac42400096e3f6b545323665f6cfdbb6b Mon Sep 17 00:00:00 2001
From: Matt Johnston <matt@ucc.asn.au>
Date: Sat, 7 Oct 2006 17:48:55 +0000
Subject: [PATCH 03/10] Rearranged (and hopefully simplified) channel close/eof
 handling

--HG--
branch : channel-fix
extra : convert_revision : d44aac5fec50b1d20515da77d410d3c511f16277
---
 common-channel.c | 200 ++++++++++++++++++++---------------------------
 debug.h          |   2 +-
 2 files changed, 87 insertions(+), 115 deletions(-)

diff --git a/common-channel.c b/common-channel.c
index a6e3364..e8175ae 100644
--- a/common-channel.c
+++ b/common-channel.c
@@ -43,17 +43,14 @@ static void send_msg_channel_open_confirmation(struct Channel* channel,
 static void writechannel(struct Channel* channel, int fd, circbuffer *cbuf);
 static void send_msg_channel_window_adjust(struct Channel *channel, 
 		unsigned int incr);
-static void send_msg_channel_data(struct Channel *channel, int isextended,
-		unsigned int exttype);
+static void send_msg_channel_data(struct Channel *channel, int isextended);
 static void send_msg_channel_eof(struct Channel *channel);
 static void send_msg_channel_close(struct Channel *channel);
 static void remove_channel(struct Channel *channel);
 static void delete_channel(struct Channel *channel);
 static void check_in_progress(struct Channel *channel);
+static unsigned int write_pending(struct Channel * channel);
 static void check_close(struct Channel *channel);
-
-static void close_write_fd(struct Channel * channel);
-static void close_read_fd(struct Channel * channel, int fd);
 static void close_chan_fd(struct Channel *channel, int fd, int how);
 
 #define FD_UNINIT (-2)
@@ -192,7 +189,7 @@ void channelio(fd_set *readfds, fd_set *writefds) {
 	struct Channel *channel;
 	unsigned int i;
 
-	/* iterate through all the possible channels */
+	/* foreach channel */
 	for (i = 0; i < ses.chansize; i++) {
 
 		channel = ses.channels[i];
@@ -203,31 +200,19 @@ void channelio(fd_set *readfds, fd_set *writefds) {
 
 		/* read data and send it over the wire */
 		if (channel->readfd >= 0 && FD_ISSET(channel->readfd, readfds)) {
-			send_msg_channel_data(channel, 0, 0);
+			send_msg_channel_data(channel, 0);
 		}
 
 		/* read stderr data and send it over the wire */
 		if (channel->extrabuf == NULL &&
 				channel->errfd >= 0 && FD_ISSET(channel->errfd, readfds)) {
-				send_msg_channel_data(channel, 1, SSH_EXTENDED_DATA_STDERR);
+				send_msg_channel_data(channel, 1);
 		}
 
-#if 0
-		/* XXX where is this required? */
-			if (channel->initconn) {
-				/* Handling for "in progress" connection - this is needed
-				 * to avoid spinning 100% CPU when we connect to a server
-				 * which doesn't send anything (tcpfwding) */
-				check_in_progress(channel);
-				continue; /* Important not to use the channel after 
-							 check_in_progress(), as it may be NULL */
-			}
-#endif
-
 		/* write to program/pipe stdin */
 		if (channel->writefd >= 0 && FD_ISSET(channel->writefd, writefds)) {
 			if (channel->initconn) {
-				/* XXX could this go somewhere cleaner? */
+				/* XXX should this go somewhere cleaner? */
 				check_in_progress(channel);
 				continue; /* Important not to use the channel after
 							 check_in_progress(), as it may be NULL */
@@ -241,10 +226,10 @@ void channelio(fd_set *readfds, fd_set *writefds) {
 			writechannel(channel, channel->errfd, channel->extrabuf);
 		}
 	
-		/* now handle any of the channel-closing type stuff */
+		/* handle any channel closing etc */
 		check_close(channel);
 
-	} /* foreach channel */
+	}
 
 	/* Listeners such as TCP, X11, agent-auth */
 #ifdef USING_LISTENERS
@@ -253,6 +238,20 @@ void channelio(fd_set *readfds, fd_set *writefds) {
 }
 
 
+/* Returns true if there is data remaining to be written to stdin or
+ * stderr of a channel's endpoint. */
+static unsigned int write_pending(struct Channel * channel) {
+
+	if (channel->writefd >= 0 && cbuf_getused(channel->writebuf) > 0) {
+		return 1;
+	} else if (channel->errfd >= 0 && channel->extrabuf && 
+			cbuf_getused(channel->writebuf) > 0) {
+		return 1;
+	}
+	return 0;
+}
+
+
 /* EOF/close handling */
 static void check_close(struct Channel *channel) {
 
@@ -264,8 +263,13 @@ static void check_close(struct Channel *channel) {
 				channel->writebuf,
 				channel->writebuf ? 0 : cbuf_getused(channel->extrabuf)))
 
-	/* XXX not good, doesn't flush out */
-	if (channel->recv_close) {
+	if (!channel->sent_close
+			&& channel->writefd == FD_CLOSED
+			&& (channel->errfd == FD_CLOSED || channel->extrabuf == NULL)) {
+		send_msg_channel_close(channel);
+	}
+
+	if (channel->recv_close && !write_pending(channel)) {
 		if (! channel->sent_close) {
 			TRACE(("Sending MSG_CHANNEL_CLOSE in response to same."))
 			send_msg_channel_close(channel);
@@ -274,8 +278,10 @@ static void check_close(struct Channel *channel) {
 		return;
 	}
 
-	/* server chansession channels are special, since readfd mightn't
-	 * close in the case of "sleep 4 & echo blah" until the sleep is up */
+#if 0
+	// The only use of check_close is "return channel->writefd == -1;" for a server
+	// chansession. Should be able to handle that with just the general
+	// socket close handling...?
 	if (channel->type->check_close) {
 		if (channel->type->check_close(channel)) {
 			close_write_fd(channel);
@@ -283,6 +289,7 @@ static void check_close(struct Channel *channel) {
 			close_read_fd(channel, channel->errfd);
 		}
 	}
+#endif
 
 	if (!channel->sent_eof
 		&& channel->readfd == FD_CLOSED 
@@ -296,25 +303,6 @@ static void check_close(struct Channel *channel) {
 		&& (channel->extrabuf != NULL || channel->errfd == FD_CLOSED)) {
 		send_msg_channel_close(channel);
 	}
-
-	/* XXX blah */
-	if (channel->recv_eof &&
-		(cbuf_getused(channel->writebuf) == 0
-			&& (channel->extrabuf == NULL 
-					|| cbuf_getused(channel->extrabuf) == 0))) {
-		close_write_fd(channel);
-	}
-
-	/* When either party wishes to terminate the channel, it sends
-	 * SSH_MSG_CHANNEL_CLOSE.  Upon receiving this message, a party MUST
-	 * send back a SSH_MSG_CHANNEL_CLOSE unless it has already sent this
-	 * message for the channel.  The channel is considered closed for a
-	 * party when it has both sent and received SSH_MSG_CHANNEL_CLOSE, and
-	 * the party may then reuse the channel number.  A party MAY send
-	 * SSH_MSG_CHANNEL_CLOSE without having sent or received
-	 * SSH_MSG_CHANNEL_EOF. 
-	 * (from draft-ietf-secsh-connect)
-	 */
 }
 
 
@@ -398,9 +386,7 @@ static void writechannel(struct Channel* channel, int fd, circbuffer *cbuf) {
 	len = write(fd, cbuf_readptr(cbuf, maxlen), maxlen);
 	if (len <= 0) {
 		if (len < 0 && errno != EINTR) {
-			/* no more to write - we close it even if the fd was stderr, since
-			 * that's a nasty failure too */
-			close_write_fd(channel);
+			close_chan_fd(channel, fd, SHUT_WR);
 		}
 		TRACE(("leave writechannel: len <= 0"))
 		return;
@@ -409,6 +395,13 @@ static void writechannel(struct Channel* channel, int fd, circbuffer *cbuf) {
 	cbuf_incrread(cbuf, len);
 	channel->recvdonelen += len;
 
+	/* We're closing out */
+	if (channel->recv_eof && cbuf_getused(cbuf) == 0) {
+	TRACE(("leave writechannel"))
+		close_chan_fd(channel, fd, SHUT_WR);
+		return;
+	}
+
 	/* Window adjust handling */
 	if (channel->recvdonelen >= RECV_WINDOWEXTEND) {
 		/* Set it back to max window */
@@ -572,16 +565,12 @@ void recv_msg_channel_request() {
  * chan is the remote channel, isextended is 0 if it is normal data, 1
  * if it is extended data. if it is extended, then the type is in
  * exttype */
-static void send_msg_channel_data(struct Channel *channel, int isextended,
-		unsigned int exttype) {
+static void send_msg_channel_data(struct Channel *channel, int isextended) {
 
 	int len;
 	size_t maxlen, size_pos;
 	int fd;
 
-/*	TRACE(("enter send_msg_channel_data"))
-	TRACE(("extended = %d type = %d", isextended, exttype))*/
-
 	CHECKCLEARTOWRITE();
 
 	dropbear_assert(!channel->sent_close);
@@ -608,7 +597,7 @@ static void send_msg_channel_data(struct Channel *channel, int isextended,
 			isextended ? SSH_MSG_CHANNEL_EXTENDED_DATA : SSH_MSG_CHANNEL_DATA);
 	buf_putint(ses.writepayload, channel->remotechan);
 	if (isextended) {
-		buf_putint(ses.writepayload, exttype);
+		buf_putint(ses.writepayload, SSH_EXTENDED_DATA_STDERR);
 	}
 	/* a dummy size first ...*/
 	size_pos = ses.writepayload->pos;
@@ -618,7 +607,7 @@ static void send_msg_channel_data(struct Channel *channel, int isextended,
 	len = read(fd, buf_getwriteptr(ses.writepayload, maxlen), maxlen);
 	if (len <= 0) {
 		if (len == 0 || errno != EINTR) {
-			close_read_fd(channel, fd);
+			close_chan_fd(channel, fd, SHUT_RD);
 		}
 		ses.writepayload->len = ses.writepayload->pos = 0;
 		TRACE(("leave send_msg_channel_data: read err or EOF for fd %d", 
@@ -891,6 +880,47 @@ static void send_msg_channel_open_confirmation(struct Channel* channel,
 	TRACE(("leave send_msg_channel_open_confirmation"))
 }
 
+/* close a fd, how is SHUT_RD or SHUT_WR */
+static void close_chan_fd(struct Channel *channel, int fd, int how) {
+
+	int closein = 0, closeout = 0;
+
+	if (channel->type->sepfds) {
+		TRACE(("shutdown((%d), %d)", fd, how))
+		shutdown(fd, how);
+		if (how == 0) {
+			closeout = 1;
+		} else {
+			closein = 1;
+		}
+	} else {
+		close(fd);
+		closein = closeout = 1;
+	}
+
+	if (closeout && fd == channel->readfd) {
+		channel->readfd = FD_CLOSED;
+	}
+	if (closeout && (channel->extrabuf == NULL) && (fd == channel->errfd)) {
+		channel->errfd = FD_CLOSED;
+	}
+
+	if (closein && fd == channel->writefd) {
+		channel->writefd = FD_CLOSED;
+	}
+	if (closein && (channel->extrabuf != NULL) && (fd == channel->errfd)) {
+		channel->errfd = FD_CLOSED;
+	}
+
+	/* if we called shutdown on it and all references are gone, then we 
+	 * need to close() it to stop it lingering */
+	if (channel->type->sepfds && channel->readfd == FD_CLOSED 
+		&& channel->writefd == FD_CLOSED && channel->errfd == FD_CLOSED) {
+		close(fd);
+	}
+}
+
+
 #if defined(USING_LISTENERS) || defined(DROPBEAR_CLIENT)
 /* Create a new channel, and start the open request. This is intended
  * for X11, agent, tcp forwarding, and should be filled with channel-specific
@@ -980,61 +1010,3 @@ void recv_msg_channel_open_failure() {
 	remove_channel(channel);
 }
 #endif /* USING_LISTENERS */
-
-/* close a stdout/stderr fd */
-static void close_read_fd(struct Channel * channel, int fd) {
-
-	/* don't close it if it is the same as writefd,
-	 * unless writefd is already set -1 */
-	TRACE(("enter close_read_fd"))
-	close_chan_fd(channel, fd, 0);
-	TRACE(("leave close_read_fd"))
-}
-
-/* close a stdin fd */
-static void close_write_fd(struct Channel * channel) {
-
-	TRACE(("enter close_write_fd"))
-	close_chan_fd(channel, channel->writefd, 1);
-	TRACE(("leave close_write_fd"))
-}
-
-/* close a fd, how is 0 for stdout/stderr, 1 for stdin */
-static void close_chan_fd(struct Channel *channel, int fd, int how) {
-
-	int closein = 0, closeout = 0;
-
-	if (channel->type->sepfds) {
-		TRACE(("shutdown((%d), %d)", fd, how))
-		shutdown(fd, how);
-		if (how == 0) {
-			closeout = 1;
-		} else {
-			closein = 1;
-		}
-	} else {
-		close(fd);
-		closein = closeout = 1;
-	}
-
-	if (closeout && fd == channel->readfd) {
-		channel->readfd = FD_CLOSED;
-	}
-	if (closeout && (channel->extrabuf == NULL) && (fd == channel->errfd)) {
-		channel->errfd = FD_CLOSED;
-	}
-
-	if (closein && fd == channel->writefd) {
-		channel->writefd = FD_CLOSED;
-	}
-	if (closein && (channel->extrabuf != NULL) && (fd == channel->errfd)) {
-		channel->errfd = FD_CLOSED;
-	}
-
-	/* if we called shutdown on it and all references are gone, then we 
-	 * need to close() it to stop it lingering */
-	if (channel->type->sepfds && channel->readfd == FD_CLOSED 
-		&& channel->writefd == FD_CLOSED && channel->errfd == FD_CLOSED) {
-		close(fd);
-	}
-}
diff --git a/debug.h b/debug.h
index 0cb4691..ecc9d4a 100644
--- a/debug.h
+++ b/debug.h
@@ -39,7 +39,7 @@
  * Caution: Don't use this in an unfriendly environment (ie unfirewalled),
  * since the printing may not sanitise strings etc. This will add a reasonable
  * amount to your executable size. */
-/*#define DEBUG_TRACE */
+#define DEBUG_TRACE
 
 /* All functions writing to the cleartext payload buffer call
  * CHECKCLEARTOWRITE() before writing. This is only really useful if you're

From cc340d9cdca97b1fd6d0f484816dbe3414ad3702 Mon Sep 17 00:00:00 2001
From: Matt Johnston <matt@ucc.asn.au>
Date: Mon, 9 Oct 2006 16:31:00 +0000
Subject: [PATCH 04/10] Passes most test cases now

--HG--
branch : channel-fix
extra : convert_revision : 5a5f398411a7a3baa9472daa80fea0574fbd8a9a
---
 common-channel.c  | 37 ++++++++-----------------------------
 svr-chansession.c |  7 ++++++-
 2 files changed, 14 insertions(+), 30 deletions(-)

diff --git a/common-channel.c b/common-channel.c
index e8175ae..688ee56 100644
--- a/common-channel.c
+++ b/common-channel.c
@@ -263,10 +263,12 @@ static void check_close(struct Channel *channel) {
 				channel->writebuf,
 				channel->writebuf ? 0 : cbuf_getused(channel->extrabuf)))
 
-	if (!channel->sent_close
-			&& channel->writefd == FD_CLOSED
-			&& (channel->errfd == FD_CLOSED || channel->extrabuf == NULL)) {
-		send_msg_channel_close(channel);
+	/* A bit of a hack for closing up server session channels */
+	if (channel->writefd >= 0 
+			&& channel->type->check_close 
+			&& channel->type->check_close(channel)) {
+		TRACE(("channel->type->check_close got hit"))
+		close_chan_fd(channel, channel->writefd, SHUT_WR);
 	}
 
 	if (channel->recv_close && !write_pending(channel)) {
@@ -278,31 +280,15 @@ static void check_close(struct Channel *channel) {
 		return;
 	}
 
-#if 0
-	// The only use of check_close is "return channel->writefd == -1;" for a server
-	// chansession. Should be able to handle that with just the general
-	// socket close handling...?
-	if (channel->type->check_close) {
-		if (channel->type->check_close(channel)) {
-			close_write_fd(channel);
-			close_read_fd(channel, channel->readfd);
-			close_read_fd(channel, channel->errfd);
-		}
+	if (channel->recv_eof && !write_pending(channel)) {
+		close_chan_fd(channel, channel->writefd, SHUT_WR);
 	}
-#endif
 
 	if (!channel->sent_eof
 		&& channel->readfd == FD_CLOSED 
 		&& (channel->extrabuf != NULL || channel->errfd == FD_CLOSED)) {
 		send_msg_channel_eof(channel);
 	}
-
-	if (!channel->sent_close
-		&& channel->writefd == FD_CLOSED
-		&& channel->readfd == FD_CLOSED
-		&& (channel->extrabuf != NULL || channel->errfd == FD_CLOSED)) {
-		send_msg_channel_close(channel);
-	}
 }
 
 
@@ -395,13 +381,6 @@ static void writechannel(struct Channel* channel, int fd, circbuffer *cbuf) {
 	cbuf_incrread(cbuf, len);
 	channel->recvdonelen += len;
 
-	/* We're closing out */
-	if (channel->recv_eof && cbuf_getused(cbuf) == 0) {
-	TRACE(("leave writechannel"))
-		close_chan_fd(channel, fd, SHUT_WR);
-		return;
-	}
-
 	/* Window adjust handling */
 	if (channel->recvdonelen >= RECV_WINDOWEXTEND) {
 		/* Set it back to max window */
diff --git a/svr-chansession.c b/svr-chansession.c
index 57ec118..99a67e9 100644
--- a/svr-chansession.c
+++ b/svr-chansession.c
@@ -65,6 +65,11 @@ static void get_termmodes(struct ChanSess *chansess);
 /* required to clear environment */
 extern char** environ;
 
+static int sesscheckclose(struct Channel *channel) {
+	struct ChanSess *chansess = (struct ChanSess*)channel->typedata;
+	return chansess->exit.exitpid != -1;
+}
+
 /* Handler for childs exiting, store the state for return to the client */
 
 /* There's a particular race we have to watch out for: if the forked child
@@ -962,7 +967,7 @@ const struct ChanType svrchansess = {
 	0, /* sepfds */
 	"session", /* name */
 	newchansess, /* inithandler */
-	NULL, /* checkclosehandler */
+	sesscheckclose, /* checkclosehandler */
 	chansessionrequest, /* reqhandler */
 	closechansess, /* closehandler */
 };

From 5b8a26f1d1871726d45dadd12d0080d5fb7b50f6 Mon Sep 17 00:00:00 2001
From: Matt Johnston <matt@ucc.asn.au>
Date: Wed, 11 Oct 2006 14:44:00 +0000
Subject: [PATCH 05/10] Remove accidentally removed block (making sure to close
 the channel). Other minor cleanups.

--HG--
branch : channel-fix
extra : convert_revision : 7559a8cc4f6abe2338636f2aced3a395a79c172c
---
 common-channel.c | 33 +++++++++++++++------------------
 1 file changed, 15 insertions(+), 18 deletions(-)

diff --git a/common-channel.c b/common-channel.c
index 688ee56..4c6eba1 100644
--- a/common-channel.c
+++ b/common-channel.c
@@ -285,10 +285,18 @@ static void check_close(struct Channel *channel) {
 	}
 
 	if (!channel->sent_eof
-		&& channel->readfd == FD_CLOSED 
-		&& (channel->extrabuf != NULL || channel->errfd == FD_CLOSED)) {
+			&& channel->readfd == FD_CLOSED 
+			&& (channel->extrabuf != NULL || channel->errfd == FD_CLOSED)) {
 		send_msg_channel_eof(channel);
 	}
+
+	if (!channel->sent_close
+			&& channel->writefd == FD_CLOSED
+			&& channel->readfd == FD_CLOSED
+			&& (channel->extrabuf != NULL || channel->errfd == FD_CLOSED)) {
+		send_msg_channel_close(channel);
+	}
+
 }
 
 
@@ -335,7 +343,6 @@ static void send_msg_channel_close(struct Channel *channel) {
 
 	encrypt_packet();
 
-	/* XXX is setting sent_eof required? */
 	channel->sent_eof = 1;
 	channel->sent_close = 1;
 	TRACE(("leave send_msg_channel_close"))
@@ -469,7 +476,6 @@ void recv_msg_channel_close() {
 
 	channel = getchannel_msg("Close");
 
-	/* XXX eof required? */
 	channel->recv_eof = 1;
 	channel->recv_close = 1;
 
@@ -484,9 +490,6 @@ static void remove_channel(struct Channel * channel) {
 	TRACE(("enter remove_channel"))
 	TRACE(("channel index is %d", channel->index))
 
-	/* XXX shuold we assert for sent_closed and recv_closed?
-	 * but we also cleanup manually, maybe we need a flag. */
-
 	cbuf_free(channel->writebuf);
 	channel->writebuf = NULL;
 
@@ -630,21 +633,15 @@ void common_recv_msg_channel_data(struct Channel *channel, int fd,
 		dropbear_exit("received data after eof");
 	}
 
-	/* XXX this is getting hit by people - maybe should be
-	 * made less fatal (or just fixed). 
-	 *
-	 * The most likely explanation is that the socket is being closed (due to
-	 * write failure etc) but the far end doesn't know yet, so keeps sending
-	 * packets.  We already know that the channel number that was given was
-	 * valid, so probably should skip out here. Maybe
-	 * assert(channel->sent_close), thuogh only if the close-on-failure code is
-	 * doing that */
  	if (fd < 0) {
-		dropbear_exit("received data with bad writefd");
+		/* If we have encountered failed write, the far side might still
+		 * be sending data without having yet received our close notification.
+		 * We just drop the data. */
+		return;
 	}
 
 	datalen = buf_getint(ses.payload);
-
+	TRACE(("length %d", datalen))
 
 	maxdata = cbuf_getavail(cbuf);
 

From dd06653e53c9a6a3734eaf5ce5e9374f559a2bf4 Mon Sep 17 00:00:00 2001
From: Matt Johnston <matt@ucc.asn.au>
Date: Tue, 5 Dec 2006 13:27:59 +0000
Subject: [PATCH 06/10] Tidy up behaviour when select() is interrupted. We
 follow normal codepaths, just with no FDs set.

--HG--
branch : channel-fix
extra : convert_revision : d348546b80847bc0d42a7b5208bb31a54f1fdfaf
---
 common-session.c | 28 +++++++++++-----------------
 1 file changed, 11 insertions(+), 17 deletions(-)

diff --git a/common-session.c b/common-session.c
index b8ea6f7..a16d1f9 100644
--- a/common-session.c
+++ b/common-session.c
@@ -143,27 +143,21 @@ void session_loop(void(*loophandler)()) {
 			dropbear_exit("Terminated by signal");
 		}
 		
-		if (val < 0) {
-			if (errno == EINTR) {
-				/* This must happen even if we've been interrupted, so that
-				 * changed signal-handler vars can take effect etc */
-				if (loophandler) {
-					loophandler();
-				}
-				continue;
-			} else {
-				dropbear_exit("Error in select");
-			}
+		if (val < 0 && errno != EINTR) {
+			dropbear_exit("Error in select");
+		}
+
+		if (val <= 0) {
+			/* If we were interrupted or the select timed out, we still
+			 * want to iterate over channels etc for reading, to handle
+			 * server processes exiting etc. 
+			 * We don't want to read/write FDs. */
+			FD_ZERO(&writefd);
+			FD_ZERO(&readfd);
 		}
 
 		/* check for auth timeout, rekeying required etc */
 		checktimeouts();
-		
-		if (val == 0) {
-			/* timeout */
-			TRACE(("select timeout"))
-			continue;
-		}
 
 		/* process session socket's incoming/outgoing data */
 		if (ses.sock != -1) {

From e1d3a8a6e9481b9fd53e5e8aa505ffc9b4536660 Mon Sep 17 00:00:00 2001
From: Matt Johnston <matt@ucc.asn.au>
Date: Tue, 5 Dec 2006 14:42:03 +0000
Subject: [PATCH 07/10] - Add some extra tracing. - Be clearer about errfd be
 used for read versus write with ERRFD_IS_READ and ERRFD_IS_WRITE macros

--HG--
branch : channel-fix
extra : convert_revision : 8778af05d9573c68b0d859feb7079502b46ed769
---
 common-channel.c | 55 +++++++++++++++++++++++++-----------------------
 1 file changed, 29 insertions(+), 26 deletions(-)

diff --git a/common-channel.c b/common-channel.c
index 4c6eba1..19b807f 100644
--- a/common-channel.c
+++ b/common-channel.c
@@ -56,6 +56,9 @@ static void close_chan_fd(struct Channel *channel, int fd, int how);
 #define FD_UNINIT (-2)
 #define FD_CLOSED (-1)
 
+#define ERRFD_IS_READ(channel) ((channel)->extrabuf == NULL)
+#define ERRFD_IS_WRITE(channel) (!ERRFD_IS_READ(channel))
+
 /* Initialise all the channels */
 void chaninitialise(const struct ChanType *chantypes[]) {
 
@@ -204,7 +207,7 @@ void channelio(fd_set *readfds, fd_set *writefds) {
 		}
 
 		/* read stderr data and send it over the wire */
-		if (channel->extrabuf == NULL &&
+		if (ERRFD_IS_READ(channel) &&
 				channel->errfd >= 0 && FD_ISSET(channel->errfd, readfds)) {
 				send_msg_channel_data(channel, 1);
 		}
@@ -221,7 +224,7 @@ void channelio(fd_set *readfds, fd_set *writefds) {
 		}
 		
 		/* stderr for client mode */
-		if (channel->extrabuf != NULL 
+		if (ERRFD_IS_WRITE(channel)
 				&& channel->errfd >= 0 && FD_ISSET(channel->errfd, writefds)) {
 			writechannel(channel, channel->errfd, channel->extrabuf);
 		}
@@ -245,7 +248,7 @@ static unsigned int write_pending(struct Channel * channel) {
 	if (channel->writefd >= 0 && cbuf_getused(channel->writebuf) > 0) {
 		return 1;
 	} else if (channel->errfd >= 0 && channel->extrabuf && 
-			cbuf_getused(channel->writebuf) > 0) {
+			cbuf_getused(channel->extrabuf) > 0) {
 		return 1;
 	}
 	return 0;
@@ -258,18 +261,9 @@ static void check_close(struct Channel *channel) {
 	TRACE(("check_close: writefd %d, readfd %d, errfd %d, sent_close %d, recv_close %d",
 				channel->writefd, channel->readfd,
 				channel->errfd, channel->sent_close, channel->recv_close))
-	TRACE(("writebuf size %d extrabuf ptr 0x%x extrabuf size %d",
+	TRACE(("writebuf size %d extrabuf size %d",
 				cbuf_getused(channel->writebuf),
-				channel->writebuf,
-				channel->writebuf ? 0 : cbuf_getused(channel->extrabuf)))
-
-	/* A bit of a hack for closing up server session channels */
-	if (channel->writefd >= 0 
-			&& channel->type->check_close 
-			&& channel->type->check_close(channel)) {
-		TRACE(("channel->type->check_close got hit"))
-		close_chan_fd(channel, channel->writefd, SHUT_WR);
-	}
+				channel->extrabuf ? cbuf_getused(channel->extrabuf) : 0))
 
 	if (channel->recv_close && !write_pending(channel)) {
 		if (! channel->sent_close) {
@@ -284,16 +278,18 @@ static void check_close(struct Channel *channel) {
 		close_chan_fd(channel, channel->writefd, SHUT_WR);
 	}
 
+	/* If we're not going to send any more data, send EOF */
 	if (!channel->sent_eof
 			&& channel->readfd == FD_CLOSED 
-			&& (channel->extrabuf != NULL || channel->errfd == FD_CLOSED)) {
+			&& (ERRFD_IS_WRITE(channel) || channel->errfd == FD_CLOSED)) {
 		send_msg_channel_eof(channel);
 	}
 
+	/* And if we can't receive any more data from them either, close up */
 	if (!channel->sent_close
 			&& channel->writefd == FD_CLOSED
 			&& channel->readfd == FD_CLOSED
-			&& (channel->extrabuf != NULL || channel->errfd == FD_CLOSED)) {
+			&& channel->errfd == FD_CLOSED) {
 		send_msg_channel_close(channel);
 	}
 
@@ -371,19 +367,21 @@ static void writechannel(struct Channel* channel, int fd, circbuffer *cbuf) {
 
 	int len, maxlen;
 
-	TRACE(("enter writechannel"))
+	TRACE(("enter writechannel fd %d", fd))
 
 	maxlen = cbuf_readlen(cbuf);
 
 	/* Write the data out */
 	len = write(fd, cbuf_readptr(cbuf, maxlen), maxlen);
 	if (len <= 0) {
+		TRACE(("errno %d len %d", errno, len))
 		if (len < 0 && errno != EINTR) {
 			close_chan_fd(channel, fd, SHUT_WR);
 		}
 		TRACE(("leave writechannel: len <= 0"))
 		return;
 	}
+	TRACE(("writechannel wrote %d", len))
 
 	cbuf_incrread(cbuf, len);
 	channel->recvdonelen += len;
@@ -425,7 +423,7 @@ void setchannelfds(fd_set *readfds, fd_set *writefds) {
 				FD_SET(channel->readfd, readfds);
 			}
 			
-			if (channel->extrabuf == NULL && channel->errfd >= 0) {
+			if (ERRFD_IS_READ(channel) && channel->errfd >= 0) {
 					FD_SET(channel->errfd, readfds);
 			}
 		}
@@ -436,7 +434,7 @@ void setchannelfds(fd_set *readfds, fd_set *writefds) {
 				FD_SET(channel->writefd, writefds);
 		}
 
-		if (channel->extrabuf != NULL && channel->errfd >= 0 
+		if (ERRFD_IS_WRITE(channel) != NULL && channel->errfd >= 0 
 				&& cbuf_getused(channel->extrabuf) > 0 ) {
 				FD_SET(channel->errfd, writefds);
 		}
@@ -501,8 +499,11 @@ static void remove_channel(struct Channel * channel) {
 
 	/* close the FDs in case they haven't been done
 	 * yet (they might have been shutdown etc) */
+	TRACE(("CLOSE writefd %d", channel->writefd))
 	close(channel->writefd);
+	TRACE(("CLOSE readfd %d", channel->readfd))
 	close(channel->readfd);
+	TRACE(("CLOSE errfd %d", channel->errfd))
 	close(channel->errfd);
 
 	channel->typedata = NULL;
@@ -562,6 +563,7 @@ static void send_msg_channel_data(struct Channel *channel, int isextended) {
 	} else {
 		fd = channel->readfd;
 	}
+	TRACE(("enter send_msg_channel_data isextended %d fd %d", isextended, fd))
 	dropbear_assert(fd >= 0);
 
 	maxlen = MIN(channel->transwindow, channel->transmaxpacket);
@@ -592,8 +594,8 @@ static void send_msg_channel_data(struct Channel *channel, int isextended) {
 			close_chan_fd(channel, fd, SHUT_RD);
 		}
 		ses.writepayload->len = ses.writepayload->pos = 0;
-		TRACE(("leave send_msg_channel_data: read err or EOF for fd %d", 
-					channel->index));
+		TRACE(("leave send_msg_channel_data: len %d read err or EOF for fd %d", 
+					len, channel->index));
 		return;
 	}
 	buf_incrwritepos(ses.writepayload, len);
@@ -740,7 +742,6 @@ void recv_msg_channel_open() {
 	/* Get the channel type. Client and server style invokation will set up a
 	 * different list for ses.chantypes at startup. We just iterate through
 	 * this list and find the matching name */
-	/* XXX fugly */
 	for (cp = &ses.chantypes[0], chantype = (*cp); 
 			chantype != NULL;
 			cp++, chantype = (*cp)) {
@@ -862,7 +863,7 @@ static void close_chan_fd(struct Channel *channel, int fd, int how) {
 	int closein = 0, closeout = 0;
 
 	if (channel->type->sepfds) {
-		TRACE(("shutdown((%d), %d)", fd, how))
+		TRACE(("SHUTDOWN(%d, %d)", fd, how))
 		shutdown(fd, how);
 		if (how == 0) {
 			closeout = 1;
@@ -870,21 +871,22 @@ static void close_chan_fd(struct Channel *channel, int fd, int how) {
 			closein = 1;
 		}
 	} else {
+		TRACE(("CLOSE some fd %d", fd))
 		close(fd);
 		closein = closeout = 1;
 	}
 
-	if (closeout && fd == channel->readfd) {
+	if (closeout && (fd == channel->readfd)) {
 		channel->readfd = FD_CLOSED;
 	}
-	if (closeout && (channel->extrabuf == NULL) && (fd == channel->errfd)) {
+	if (closeout && ERRFD_IS_READ(channel) && (fd == channel->errfd)) {
 		channel->errfd = FD_CLOSED;
 	}
 
 	if (closein && fd == channel->writefd) {
 		channel->writefd = FD_CLOSED;
 	}
-	if (closein && (channel->extrabuf != NULL) && (fd == channel->errfd)) {
+	if (closein && ERRFD_IS_WRITE(channel) && (fd == channel->errfd)) {
 		channel->errfd = FD_CLOSED;
 	}
 
@@ -892,6 +894,7 @@ static void close_chan_fd(struct Channel *channel, int fd, int how) {
 	 * need to close() it to stop it lingering */
 	if (channel->type->sepfds && channel->readfd == FD_CLOSED 
 		&& channel->writefd == FD_CLOSED && channel->errfd == FD_CLOSED) {
+		TRACE(("CLOSE (finally) of %d", fd))
 		close(fd);
 	}
 }

From 4e09d27c6f8e59631becb422a055ede15833af89 Mon Sep 17 00:00:00 2001
From: Matt Johnston <matt@ucc.asn.au>
Date: Tue, 5 Dec 2006 15:23:06 +0000
Subject: [PATCH 08/10] Add some code for testing whether a writefd is closed
 (by read()ing from it)

--HG--
branch : channel-fix
extra : convert_revision : 1dfbc5ef92391d01b576c8506061927869a89887
---
 common-channel.c | 43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/common-channel.c b/common-channel.c
index 19b807f..8159477 100644
--- a/common-channel.c
+++ b/common-channel.c
@@ -186,6 +186,29 @@ struct Channel* getchannel() {
 	return getchannel_msg(NULL);
 }
 
+/* In order to tell if a writefd is closed, we put it in the readfd FD_SET.
+   We then just try reading a single byte from it. It'll give EAGAIN or something
+   if the socket is still alive (but the FD probably shouldn't be set anyway?)*/
+static void check_closed_writefd(struct Channel* channel, int fd) {
+	char c;
+	int ret;
+	TRACE(("enter check_closed_writefd fd %d", fd))
+	if (fd < 0)	{
+		TRACE(("leave check_closed_writefd."))
+		return;
+	}
+
+	/* Read something. doing read(fd,x,0) seems to become a NOP on some platforms */
+	ret = read(fd, &c, 1);
+	TRACE(("ret %d errno %d", ret, errno))
+	if (ret > 0 || (ret < 0 && (errno == EINTR || errno == EAGAIN)))	{
+		TRACE(("leave check_closed_writefd"))
+		return;
+	}
+	close_chan_fd(channel, fd, SHUT_WR);
+	TRACE(("leave check_closed_writefd after closing %d", fd))
+}
+
 /* Iterate through the channels, performing IO if available */
 void channelio(fd_set *readfds, fd_set *writefds) {
 
@@ -229,6 +252,16 @@ void channelio(fd_set *readfds, fd_set *writefds) {
 			writechannel(channel, channel->errfd, channel->extrabuf);
 		}
 	
+		/* Check writefds for close, even if we don't have anything 
+		   to write into them. */
+		if (channel->writefd >= 0) {
+			check_closed_writefd(channel, channel->writefd);
+		}
+		if (ERRFD_IS_WRITE(channel) && channel->errfd >= 0) {
+			check_closed_writefd(channel, channel->errfd);
+		}
+	
+	
 		/* handle any channel closing etc */
 		check_close(channel);
 
@@ -439,6 +472,16 @@ void setchannelfds(fd_set *readfds, fd_set *writefds) {
 				FD_SET(channel->errfd, writefds);
 		}
 
+		/* We also set the writefds for reading, so that we will be notified of close */
+		if (channel->writefd >= 0) {
+			FD_SET(channel->writefd, readfds);
+		}		
+		if (ERRFD_IS_WRITE(channel) && channel->errfd >= 0) {
+			FD_SET(channel->errfd, readfds);
+		}
+		
+		
+
 	} /* foreach channel */
 
 #ifdef USING_LISTENERS

From 41bfa930580c0f81f953cd94340b84e793b14591 Mon Sep 17 00:00:00 2001
From: Matt Johnston <matt@ucc.asn.au>
Date: Sat, 3 Feb 2007 08:09:22 +0000
Subject: [PATCH 09/10] disapproval of revision
 '5fdf69ca60d1683cdd9f4c2595134bed26394834'

--HG--
branch : channel-fix
extra : convert_revision : b24757c764465a206a258bae27ac0509fc56cd16
---
 common-channel.c | 30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/common-channel.c b/common-channel.c
index 7e8d428..54829e5 100644
--- a/common-channel.c
+++ b/common-channel.c
@@ -203,6 +203,24 @@ void channelio(fd_set *readfds, fd_set *writefds) {
 				send_msg_channel_data(channel, 1, SSH_EXTENDED_DATA_STDERR);
 		}
 
+		/* if we can read from the writefd, it might be closed, so we try to
+		 * see if it has errors */
+		if (channel->writefd >= 0 && channel->writefd != channel->readfd
+				&& FD_ISSET(channel->writefd, readfds)) {
+			if (channel->initconn) {
+				/* Handling for "in progress" connection - this is needed
+				 * to avoid spinning 100% CPU when we connect to a server
+				 * which doesn't send anything (tcpfwding) */
+				checkinitdone(channel);
+				continue; /* Important not to use the channel after 
+							 checkinitdone(), as it may be NULL */
+			}
+			ret = write(channel->writefd, NULL, 0); /* Fake write */
+			if (ret < 0 && errno != EINTR && errno != EAGAIN) {
+				closewritefd(channel);
+			}
+		}
+
 		/* write to program/pipe stdin */
 		if (channel->writefd >= 0 && FD_ISSET(channel->writefd, writefds)) {
 			if (channel->initconn) {
@@ -427,7 +445,17 @@ void setchannelfds(fd_set *readfds, fd_set *writefds) {
 			}
 		}
 
-		/* Stuff from the wire */
+		/* For checking FD status (ie closure etc) - we don't actually
+		 * read data from writefd */
+		TRACE(("writefd = %d, readfd %d, errfd %d, bufused %d", 
+					channel->writefd, channel->readfd,
+					channel->errfd,
+					cbuf_getused(channel->writebuf) ))
+		if (channel->writefd >= 0 && channel->writefd != channel->readfd) {
+			FD_SET(channel->writefd, readfds);
+		}
+
+		/* Stuff from the wire, to local program/shell/user etc */
 		if ((channel->writefd >= 0 && cbuf_getused(channel->writebuf) > 0 )
 				|| channel->initconn) {
 

From 5acf7a6aaa9c0d4f43825945b0d2552246d22f13 Mon Sep 17 00:00:00 2001
From: Matt Johnston <matt@ucc.asn.au>
Date: Sat, 3 Feb 2007 08:10:09 +0000
Subject: [PATCH 10/10] disapproval of revision
 '1250b8af44b62d8f4fe0f8d9fc7e7a1cc34e7e1c'

--HG--
branch : channel-fix
extra : convert_revision : 275426b7a4b94a0507c78327f86bcd2cd0b0f985
---
 common-channel.c  | 52 +++++++++++++++++++++++++----------------------
 svr-chansession.c |  3 +--
 2 files changed, 29 insertions(+), 26 deletions(-)

diff --git a/common-channel.c b/common-channel.c
index 5765f07..0cd6ef8 100644
--- a/common-channel.c
+++ b/common-channel.c
@@ -181,6 +181,7 @@ void channelio(fd_set *readfds, fd_set *writefds) {
 
 	struct Channel *channel;
 	unsigned int i;
+	int ret;
 
 	/* iterate through all the possible channels */
 	for (i = 0; i < ses.chansize; i++) {
@@ -204,7 +205,8 @@ void channelio(fd_set *readfds, fd_set *writefds) {
 
 		/* if we can read from the writefd, it might be closed, so we try to
 		 * see if it has errors */
-		if (channel->writefd >= 0 && channel->writefd != channel->readfd
+		if (IS_DROPBEAR_SERVER && channel->writefd >= 0 
+				&& channel->writefd != channel->readfd
 				&& FD_ISSET(channel->writefd, readfds)) {
 			if (channel->initconn) {
 				/* Handling for "in progress" connection - this is needed
@@ -259,27 +261,27 @@ static void checkclose(struct Channel *channel) {
 				channel->writebuf,
 				channel->writebuf ? 0 : cbuf_getused(channel->extrabuf)))
 
-	if (!channel->sentclosed) {
-
-		/* check for exited - currently only used for server sessions,
-		 * if the shell has exited etc */
-		if (channel->type->checkclose) {
-			if (channel->type->checkclose(channel)) {
-				closewritefd(channel);
-			}
+	/* server chansession channels are special, since readfd mightn't
+	 * close in the case of "sleep 4 & echo blah" until the sleep is up */
+	if (channel->type->checkclose) {
+		if (channel->type->checkclose(channel)) {
+			closewritefd(channel);
+			closereadfd(channel, channel->readfd);
+			closereadfd(channel, channel->errfd);
 		}
+	}
 
-		if (!channel->senteof
-			&& channel->readfd == FD_CLOSED 
-			&& (channel->extrabuf != NULL || channel->errfd == FD_CLOSED)) {
-			send_msg_channel_eof(channel);
-		}
+	if (!channel->senteof
+		&& channel->readfd == FD_CLOSED 
+		&& (channel->extrabuf == NULL || channel->errfd == FD_CLOSED)) {
+		send_msg_channel_eof(channel);
+	}
 
-		if (channel->writefd == FD_CLOSED
-			&& channel->readfd == FD_CLOSED
-			&& (channel->extrabuf != NULL || channel->errfd == FD_CLOSED)) {
-			send_msg_channel_close(channel);
-		}
+	if (!channel->sentclosed
+		&& channel->writefd == FD_CLOSED
+		&& channel->readfd == FD_CLOSED
+		&& (channel->extrabuf == NULL || channel->errfd == FD_CLOSED)) {
+		send_msg_channel_close(channel);
 	}
 
 	/* When either party wishes to terminate the channel, it sends
@@ -444,20 +446,22 @@ void setchannelfds(fd_set *readfds, fd_set *writefds) {
 			}
 		}
 
-		/* For checking FD status (ie closure etc) - we don't actually
-		 * read data from writefd */
 		TRACE(("writefd = %d, readfd %d, errfd %d, bufused %d", 
 					channel->writefd, channel->readfd,
 					channel->errfd,
 					cbuf_getused(channel->writebuf) ))
-		if (channel->writefd >= 0 && channel->writefd != channel->readfd) {
+
+		/* For checking FD status (ie closure etc) - we don't actually
+		 * read data from writefd. We don't want to do this for the client,
+		 * since redirection to /dev/null will make it spin in the select */
+		if (IS_DROPBEAR_SERVER && channel->writefd >= 0 
+				&& channel->writefd != channel->readfd) {
 			FD_SET(channel->writefd, readfds);
 		}
 
-		/* Stuff from the wire, to local program/shell/user etc */
+		/* Stuff from the wire */
 		if ((channel->writefd >= 0 && cbuf_getused(channel->writebuf) > 0 )
 				|| channel->initconn) {
-
 				FD_SET(channel->writefd, writefds);
 		}
 
diff --git a/svr-chansession.c b/svr-chansession.c
index 605bb60..fed8240 100644
--- a/svr-chansession.c
+++ b/svr-chansession.c
@@ -67,8 +67,7 @@ static void get_termmodes(struct ChanSess *chansess);
 extern char** environ;
 
 static int sesscheckclose(struct Channel *channel) {
-	struct ChanSess *chansess = (struct ChanSess*)channel->typedata;
-	return chansess->exit.exitpid >= 0;
+	return channel->writefd == -1;
 }
 
 /* Handler for childs exiting, store the state for return to the client */