# HG changeset patch # User Matt Johnston # Date 1363708472 -28800 # Node ID 564e7f87ecc3f95787b4245084f4da67cee34ee2 # Parent 002cf09827c08801b9dddbdfe3ed1ead3db3ca87 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() diff -r 002cf09827c0 -r 564e7f87ecc3 channel.h --- 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(); diff -r 002cf09827c0 -r 564e7f87ecc3 common-channel.c --- 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);