Mercurial > dropbear

changeset 707:564e7f87ecc3

Find changesets by keywords (author, files, the commit message), revision number or hash, or revset expression.
Fix memory leak when direct TCP connections time out on connection. Long-standing bug probably stemming from the awkwardly named delete_channel() versus remove_channel()
author Matt Johnston <matt@ucc.asn.au>
date Tue, 19 Mar 2013 23:54:32 +0800
parents 002cf09827c0
children b207d5183bb7
files channel.h common-channel.c
diffstat 2 files changed, 41 insertions(+), 30 deletions(-) [+]
line wrap: on
line diff
--- a/channel.h	Tue Mar 19 20:55:11 2013 +0800
+++ b/channel.h	Tue Mar 19 23:54:32 2013 +0800
@@ -61,7 +61,8 @@
 	int readfd; /* read from insecure side, written to wire */
 	int errfd; /* used like writefd or readfd, depending if it's client or server.
 				  Doesn't exactly belong here, but is cleaner here */
-	circbuffer *writebuf; /* data from the wire, for local consumption */
+	circbuffer *writebuf; /* data from the wire, for local consumption. Can be
+							 initially NULL */
 	circbuffer *extrabuf; /* extended-data for the program - used like writebuf
 					     but for stderr */
 
@@ -102,9 +103,6 @@
 void setchannelfds(fd_set *readfd, fd_set *writefd);
 void channelio(fd_set *readfd, fd_set *writefd);
 struct Channel* getchannel();
-struct Channel* newchannel(unsigned int remotechan, 
-		const struct ChanType *type, 
-		unsigned int transwindow, unsigned int transmaxpacket);
 
 void recv_msg_channel_open();
 void recv_msg_channel_request();
--- a/common-channel.c	Tue Mar 19 20:55:11 2013 +0800
+++ b/common-channel.c	Tue Mar 19 23:54:32 2013 +0800
@@ -48,7 +48,6 @@
 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);
@@ -93,11 +92,19 @@
 	TRACE(("leave chancleanup"))
 }
 
+static void
+chan_initwritebuf(struct Channel *channel)
+{
+	dropbear_assert(channel->writebuf == NULL && channel->recvwindow == 0);
+	channel->writebuf = cbuf_new(opts.recv_window);
+	channel->recvwindow = opts.recv_window;
+}
+
 /* Create a new channel entry, send a reply confirm or failure */
 /* If remotechan, transwindow and transmaxpacket are not know (for a new
  * outgoing connection, with them to be filled on confirmation), they should
  * all be set to 0 */
-struct Channel* newchannel(unsigned int remotechan, 
+static struct Channel* newchannel(unsigned int remotechan, 
 		const struct ChanType *type, 
 		unsigned int transwindow, unsigned int transmaxpacket) {
 
@@ -152,9 +159,10 @@
 	newchan->await_open = 0;
 	newchan->flushing = 0;
 
-	newchan->writebuf = cbuf_new(opts.recv_window);
+	newchan->writebuf = NULL;
+	newchan->recvwindow = 0;
+
 	newchan->extrabuf = NULL; /* The user code can set it up */
-	newchan->recvwindow = opts.recv_window;
 	newchan->recvdonelen = 0;
 	newchan->recvmaxpacket = RECV_MAX_PAYLOAD_LEN;
 
@@ -225,6 +233,7 @@
 				continue; /* Important not to use the channel after
 							 check_in_progress(), as it may be NULL */
 			}
+			dropbear_assert(channel->writebuf);
 			writechannel(channel, channel->writefd, channel->writebuf);
 		}
 		
@@ -250,7 +259,9 @@
  * stderr of a channel's endpoint. */
 static unsigned int write_pending(struct Channel * channel) {
 
-	if (channel->writefd >= 0 && cbuf_getused(channel->writebuf) > 0) {
+	if (channel->writefd >= 0 
+			&& channel->writebuf 
+			&& cbuf_getused(channel->writebuf) > 0) {
 		return 1;
 	} else if (channel->errfd >= 0 && channel->extrabuf && 
 			cbuf_getused(channel->extrabuf) > 0) {
@@ -268,7 +279,7 @@
 				channel->writefd, channel->readfd,
 				channel->errfd, channel->sent_close, channel->recv_close))
 	TRACE(("writebuf size %d extrabuf size %d",
-				cbuf_getused(channel->writebuf),
+				channel->writebuf ? cbuf_getused(channel->writebuf) : 0,
 				channel->extrabuf ? cbuf_getused(channel->extrabuf) : 0))
 
 	if (!channel->flushing 
@@ -352,9 +363,10 @@
 		send_msg_channel_open_failure(channel->remotechan,
 				SSH_OPEN_CONNECT_FAILED, "", "");
 		close(channel->writefd);
-		delete_channel(channel);
+		remove_channel(channel);
 		TRACE(("leave check_in_progress: fail"))
 	} else {
+		chan_initwritebuf(channel);
 		send_msg_channel_open_confirmation(channel, channel->recvwindow,
 				channel->recvmaxpacket);
 		channel->readfd = channel->writefd;
@@ -440,7 +452,8 @@
 	}
 
 	dropbear_assert(channel->recvwindow <= opts.recv_window);
-	dropbear_assert(channel->recvwindow <= cbuf_getavail(channel->writebuf));
+	dropbear_assert(channel->writebuf == NULL ||
+			channel->recvwindow <= cbuf_getavail(channel->writebuf));
 	dropbear_assert(channel->extrabuf == NULL ||
 			channel->recvwindow <= cbuf_getavail(channel->extrabuf));
 	
@@ -474,13 +487,13 @@
 		}
 
 		/* Stuff from the wire */
-		if ((channel->writefd >= 0 && cbuf_getused(channel->writebuf) > 0 )
-				|| channel->initconn) {
+		if (channel->initconn
+			||(channel->writefd >= 0 && cbuf_getused(channel->writebuf) > 0)) {
 				FD_SET(channel->writefd, writefds);
 		}
 
 		if (ERRFD_IS_WRITE(channel) && channel->errfd >= 0 
-				&& cbuf_getused(channel->extrabuf) > 0 ) {
+				&& cbuf_getused(channel->extrabuf) > 0) {
 				FD_SET(channel->errfd, writefds);
 		}
 
@@ -533,8 +546,10 @@
 	TRACE(("enter remove_channel"))
 	TRACE(("channel index is %d", channel->index))
 
-	cbuf_free(channel->writebuf);
-	channel->writebuf = NULL;
+	if (channel->writebuf) {
+		cbuf_free(channel->writebuf);
+		channel->writebuf = NULL;
+	}
 
 	if (channel->extrabuf) {
 		cbuf_free(channel->extrabuf);
@@ -553,21 +568,13 @@
 
 	channel->typedata = NULL;
 
-	delete_channel(channel);
+	ses.channels[channel->index] = NULL;
+	m_free(channel);
+	ses.chancount--;
 
 	TRACE(("leave remove_channel"))
 }
 
-/* Remove a channel entry */
-static void delete_channel(struct Channel *channel) {
-
-	ses.channels[channel->index] = NULL;
-	m_free(channel);
-	ses.chancount--;
-	
-}
-
-
 /* Handle channel specific requests, passing off to corresponding handlers
  * such as chansession or x11fwd */
 void recv_msg_channel_request() {
@@ -700,7 +707,7 @@
 		dropbear_exit("Received data after eof");
 	}
 
-	if (fd < 0) {
+	if (fd < 0 || !cbuf) {
 		/* 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. */
@@ -838,12 +845,14 @@
 		}
 		if (ret > 0) {
 			errtype = ret;
-			delete_channel(channel);
+			remove_channel(channel);
 			TRACE(("inithandler returned failure %d", ret))
 			goto failure;
 		}
 	}
 
+	chan_initwritebuf(channel);
+
 	/* success */
 	send_msg_channel_open_confirmation(channel, channel->recvwindow,
 			channel->recvmaxpacket);
@@ -982,6 +991,10 @@
 		return DROPBEAR_FAILURE;
 	}
 
+	/* Outbound opened channels don't make use of in-progress connections,
+	 * we can set it up straight away */
+	chan_initwritebuf(chan);
+
 	/* set fd non-blocking */
 	setnonblocking(fd);