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()

1 files changed, 38 insertions, 25 deletions

diff --git a/common-channel.c b/common-channel.c
index 5f22d44..be68266 100644
--- a/common-channel.c
+++ b/common-channel.c
@@ -48,7 +48,6 @@ 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);
@@ -93,11 +92,19 @@ void chancleanup() {
 	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 @@ struct Channel* newchannel(unsigned int remotechan,
 	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 @@ void channelio(fd_set *readfds, fd_set *writefds) {
 				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 @@ void channelio(fd_set *readfds, fd_set *writefds) {
  * 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 @@ static void check_close(struct Channel *channel) {
 				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 @@ static void check_in_progress(struct Channel *channel) {
 		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 @@ static void writechannel(struct Channel* channel, int fd, circbuffer *cbuf) {
 	}
 
 	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 @@ void setchannelfds(fd_set *readfds, fd_set *writefds) {
 		}
 
 		/* 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 @@ static void remove_channel(struct Channel * channel) {
 	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,20 +568,12 @@ static void remove_channel(struct Channel * channel) {
 
 	channel->typedata = NULL;
 
-	delete_channel(channel);
-
-	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--;
-	
-}
 
+	TRACE(("leave remove_channel"))
+}
 
 /* Handle channel specific requests, passing off to corresponding handlers
  * such as chansession or x11fwd */
@@ -700,7 +707,7 @@ void common_recv_msg_channel_data(struct Channel *channel, int fd,
 		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 @@ void recv_msg_channel_open() {
 		}
 		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 @@ int send_msg_channel_open_init(int fd, const struct ChanType *type) {
 		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);