Mercurial > dropbear

changeset 359:78518751cb82 channel-fix

Find changesets by keywords (author, files, the commit message), revision number or hash, or revset expression.
just shuffle some variables names about, a brief comment about the "bad writefd" problem
author Matt Johnston <matt@ucc.asn.au>
date Sun, 01 Oct 2006 16:35:13 +0000
parents e81d3bc1dc78
children 1c7bf9cec6c8
files channel.h common-channel.c options.h svr-chansession.c
diffstat 4 files changed, 88 insertions(+), 103 deletions(-) [+]
line wrap: on
line diff
--- a/channel.h	Mon Sep 11 11:22:52 2006 +0000
+++ b/channel.h	Sun Oct 01 16:35:13 2006 +0000
@@ -73,10 +73,9 @@
 	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 @@
 	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*);
 
--- a/common-channel.c	Mon Sep 11 11:22:52 2006 +0000
+++ b/common-channel.c	Sun Oct 01 16:35:13 2006 +0000
@@ -47,14 +47,14 @@
 		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 @@
 	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 @@
 	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 @@
 				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 @@
 		}
 	
 		/* now handle any of the channel-closing type stuff */
-		checkclose(channel);
+		check_close(channel);
 
 	} /* foreach channel */
 
@@ -250,12 +243,12 @@
 }
 
 
-/* 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 @@
 
 	/* 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 @@
 	 * 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 @@
  * 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 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 @@
 
 	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 @@
 
 	encrypt_packet();
 
-	channel->senteof = 1;
+	channel->sent_eof = 1;
 
 	TRACE(("leave send_msg_channel_eof"))
 }
@@ -387,7 +379,7 @@
 		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 @@
 	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 @@
 			}
 		}
 
-		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 @@
 		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 @@
 		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 @@
 
 /* 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 @@
 
 	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 @@
 
 	CHECKCLEARTOWRITE();
 
-	dropbear_assert(!channel->sentclosed);
+	dropbear_assert(!channel->sent_close);
 
 	if (isextended) {
 		fd = channel->errfd;
@@ -634,7 +613,7 @@
 	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 @@
 
 	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 @@
 }
 	
 /* 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 @@
 				goto cleanup;
 			}
 			errtype = ret;
-			deletechannel(channel);
+			delete_channel(channel);
 			TRACE(("inithandler returned failure %d", ret))
 			goto failure;
 		}
@@ -982,7 +969,7 @@
 	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 @@
 	}
 	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);
--- a/options.h	Mon Sep 11 11:22:52 2006 +0000
+++ b/options.h	Sun Oct 01 16:35:13 2006 +0000
@@ -127,8 +127,8 @@
  * 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
--- a/svr-chansession.c	Mon Sep 11 11:22:52 2006 +0000
+++ b/svr-chansession.c	Sun Oct 01 16:35:13 2006 +0000
@@ -59,14 +59,14 @@
 		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 @@
 	0, /* sepfds */
 	"session", /* name */
 	newchansess, /* inithandler */
-	sesscheckclose, /* checkclosehandler */
+	sess_check_close, /* checkclosehandler */
 	chansessionrequest, /* reqhandler */
 	closechansess, /* closehandler */
 };