Mercurial > dropbear

changeset 1077:26c1f9b8f82e coverity

Find changesets by keywords (author, files, the commit message), revision number or hash, or revset expression.
merge main
author Matt Johnston <matt@ucc.asn.au>
date Tue, 14 Apr 2015 20:44:30 +0800
parents 210982935887 (current diff) d92597ef089e (diff)
children 45830474d83c
files
diffstat 10 files changed, 91 insertions(+), 36 deletions(-) [+]
line wrap: on
line diff
--- a/CHANGES	Mon Mar 02 21:17:41 2015 +0800
+++ b/CHANGES	Tue Apr 14 20:44:30 2015 +0800
@@ -1,3 +1,19 @@
+- Improve efficiency of writing data to local program/pipes, measured 30% for
+  connections to localhost
+
+- Use TCP Fast Open on Linux if available. saves a round trip at connection
+  to hosts that have previously been connected. 
+  Needs a recent Linux kernel and possibly "sysctl -w net.ipv4.tcp_fastopen=3"
+
+- Forwarded TCP ports connect asynchronously and retry with other available
+  addresses (IPv4 or IPv6)
+
+- Free memory before exiting, patch from Thorsten Horstmann. Useful for
+  Dropbear ports to embedded systems and for checking memory leaks
+  with valgrind. Only partially implemented for client side.
+
+- Fix small ECC memory leaks
+
 2015.67 - Wednesday 28 January 2015
 
 - Call fsync() after generating private keys to ensure they aren't lost if a
--- a/channel.h	Mon Mar 02 21:17:41 2015 +0800
+++ b/channel.h	Tue Apr 14 20:44:30 2015 +0800
@@ -106,7 +106,7 @@
 
 void chaninitialise(const struct ChanType *chantypes[]);
 void chancleanup();
-void setchannelfds(fd_set *readfd, fd_set *writefd);
+void setchannelfds(fd_set *readfds, fd_set *writefds, int allow_reads);
 void channelio(fd_set *readfd, fd_set *writefd);
 struct Channel* getchannel();
 /* Returns an arbitrary channel that is in a ready state - not
--- a/cli-main.c	Mon Mar 02 21:17:41 2015 +0800
+++ b/cli-main.c	Tue Apr 14 20:44:30 2015 +0800
@@ -87,6 +87,7 @@
 static void cli_dropbear_exit(int exitcode, const char* format, va_list param) {
 
 	char fmtbuf[300];
+	char exitmsg[500];
 
 	if (!sessinitdone) {
 		snprintf(fmtbuf, sizeof(fmtbuf), "Exited: %s",
@@ -98,12 +99,15 @@
 				cli_opts.remoteport, format);
 	}
 
+	/* Arguments to the exit printout may be unsafe to use after session_cleanup() */
+	vsnprintf(exitmsg, sizeof(exitmsg), fmtbuf, param);
+
 	/* Do the cleanup first, since then the terminal will be reset */
 	session_cleanup();
 	/* Avoid printing onwards from terminal cruft */
 	fprintf(stderr, "\n");
 
-	_dropbear_log(LOG_INFO, fmtbuf, param);
+	dropbear_log(LOG_INFO, "%s", exitmsg);;
 	exit(exitcode);
 }
 
--- a/common-channel.c	Mon Mar 02 21:17:41 2015 +0800
+++ b/common-channel.c	Tue Apr 14 20:44:30 2015 +0800
@@ -473,6 +473,14 @@
 		io_count++;
 	}
 
+	if (io_count == 0) {
+		/* writechannel may sometimes be called twice in a main loop iteration.
+		From common_recv_msg_channel_data() then channelio().
+		The second call may not have any data to write, so we just return. */
+		TRACE(("leave writechannel, no data"))
+		return;
+	}
+
 	if (morelen) {
 		/* Default return value, none consumed */
 		*morelen = 0;
@@ -482,7 +490,7 @@
 
 	if (written < 0) {
 		if (errno != EINTR && errno != EAGAIN) {
-			TRACE(("errno %d len %d", errno, len))
+			TRACE(("channel IO write error fd %d %s", fd, strerror(errno)))
 			close_chan_fd(channel, fd, SHUT_WR);
 		}
 	} else {
@@ -531,7 +539,7 @@
 
 /* Set the file descriptors for the main select in session.c
  * This avoid channels which don't have any window available, are closed, etc*/
-void setchannelfds(fd_set *readfds, fd_set *writefds) {
+void setchannelfds(fd_set *readfds, fd_set *writefds, int allow_reads) {
 	
 	unsigned int i;
 	struct Channel * channel;
@@ -549,7 +557,7 @@
 		FD if there's the possibility of "~."" to kill an 
 		interactive session (the read_mangler) */
 		if (channel->transwindow > 0
-		   && (ses.dataallowed || channel->read_mangler)) {
+		   && ((ses.dataallowed && allow_reads) || channel->read_mangler)) {
 
 			if (channel->readfd >= 0) {
 				FD_SET(channel->readfd, readfds);
@@ -830,6 +838,7 @@
 	channel->recvwindow -= datalen;
 	dropbear_assert(channel->recvwindow <= opts.recv_window);
 
+	/* Attempt to write the data immediately without having to put it in the circular buffer */
 	consumed = datalen;
 	writechannel(channel, fd, cbuf, buf_getptr(ses.payload, datalen), &consumed);
 
--- a/common-session.c	Mon Mar 02 21:17:41 2015 +0800
+++ b/common-session.c	Tue Apr 14 20:44:30 2015 +0800
@@ -64,6 +64,13 @@
 	ses.sock_out = sock_out;
 	ses.maxfd = MAX(sock_in, sock_out);
 
+	if (sock_in >= 0) {
+		setnonblocking(sock_in);
+	}
+	if (sock_out >= 0) {
+		setnonblocking(sock_out);
+	}
+
 	ses.socket_prio = DROPBEAR_PRIO_DEFAULT;
 	/* Sets it to lowdelay */
 	update_channel_prio();
@@ -145,6 +152,7 @@
 
 	/* main loop, select()s for all sockets in use */
 	for(;;) {
+		const int writequeue_has_space = (ses.writequeue_len <= 2*TRANS_MAX_PAYLOAD_LEN);
 
 		timeout.tv_sec = select_timeout();
 		timeout.tv_usec = 0;
@@ -155,8 +163,12 @@
 		/* We delay reading from the input socket during initial setup until
 		after we have written out our initial KEXINIT packet (empty writequeue). 
 		This means our initial packet can be in-flight while we're doing a blocking
-		read for the remote ident */
-		if (ses.sock_in != -1 && (ses.remoteident || isempty(&ses.writequeue))) {
+		read for the remote ident.
+		We also avoid reading from the socket if the writequeue is full, that avoids
+		replies backing up */
+		if (ses.sock_in != -1 
+			&& (ses.remoteident || isempty(&ses.writequeue)) 
+			&& writequeue_has_space) {
 			FD_SET(ses.sock_in, &readfd);
 		}
 		if (ses.sock_out != -1 && !isempty(&ses.writequeue)) {
@@ -168,7 +180,7 @@
 		FD_SET(ses.signal_pipe[0], &readfd);
 
 		/* set up for channels which can be read/written */
-		setchannelfds(&readfd, &writefd);
+		setchannelfds(&readfd, &writefd, writequeue_has_space);
 
 		/* Pending connections to test */
 		set_connect_fds(&writefd);
@@ -318,9 +330,7 @@
 void send_session_identification() {
 	buffer *writebuf = buf_new(strlen(LOCAL_IDENT "\r\n") + 1);
 	buf_putbytes(writebuf, LOCAL_IDENT "\r\n", strlen(LOCAL_IDENT "\r\n"));
-	buf_putbyte(writebuf, 0x0); /* packet type */
-	buf_setpos(writebuf, 0);
-	enqueue(&ses.writequeue, writebuf);
+	writebuf_enqueue(writebuf, 0);
 }
 
 static void read_session_identification() {
--- a/netio.c	Mon Mar 02 21:17:41 2015 +0800
+++ b/netio.c	Tue Apr 14 20:44:30 2015 +0800
@@ -76,7 +76,7 @@
 
 	for (r = c->res_iter; r; r = r->ai_next)
 	{
-		assert(c->sock == -1);
+		dropbear_assert(c->sock == -1);
 
 		c->sock = socket(c->res_iter->ai_family, c->res_iter->ai_socktype, c->res_iter->ai_protocol);
 		if (c->sock < 0) {
@@ -99,11 +99,16 @@
 		message.msg_namelen = r->ai_addrlen;
 
 		if (c->writequeue) {
-			int iovlen; /* Linux msg_iovlen is a size_t */
-			message.msg_iov = packet_queue_to_iovec(c->writequeue, &iovlen);
+			/* 6 is arbitrary, enough to hold initial packets */
+			int iovlen = 6; /* Linux msg_iovlen is a size_t */
+			struct iovec iov[6];
+			packet_queue_to_iovec(c->writequeue, iov, &iovlen);
+			message.msg_iov = iov;
 			message.msg_iovlen = iovlen;
 			res = sendmsg(c->sock, &message, MSG_FASTOPEN);
 			if (res < 0 && errno != EINPROGRESS) {
+				m_free(c->errstring);
+				c->errstring = m_strdup(strerror(errno));
 				/* Not entirely sure which kind of errors are normal - 2.6.32 seems to 
 				return EPIPE for any (nonblocking?) sendmsg(). just fall back */
 				TRACE(("sendmsg tcp_fastopen failed, falling back. %s", strerror(errno)));
@@ -112,7 +117,6 @@
 				/* Set to NULL to avoid trying again */
 				c->writequeue = NULL;
 			}
-			m_free(message.msg_iov);
 			packet_queue_consume(c->writequeue, res);
 		}
 #endif
@@ -124,6 +128,8 @@
 
 		if (res < 0 && errno != EINPROGRESS) {
 			/* failure */
+			m_free(c->errstring);
+			c->errstring = m_strdup(strerror(errno));
 			close(c->sock);
 			c->sock = -1;
 			continue;
@@ -237,7 +243,7 @@
 			c->sock = -1;
 
 			m_free(c->errstring);
-			c->errstring = strerror(val);
+			c->errstring = m_strdup(strerror(val));
 		} else {
 			/* New connection has been established */
 			c->cb(DROPBEAR_SUCCESS, c->sock, c->cb_data, NULL);
@@ -254,8 +260,7 @@
 	c->writequeue = writequeue;
 }
 
-struct iovec * packet_queue_to_iovec(struct Queue *queue, int *ret_iov_count) {
-	struct iovec *iov = NULL;
+void packet_queue_to_iovec(struct Queue *queue, struct iovec *iov, unsigned int *iov_count) {
 	struct Link *l;
 	unsigned int i;
 	int len;
@@ -265,10 +270,9 @@
 	#define IOV_MAX UIO_MAXIOV
 	#endif
 
-	*ret_iov_count = MIN(queue->count, IOV_MAX);
+	*iov_count = MIN(MIN(queue->count, IOV_MAX), *iov_count);
 
-	iov = m_malloc(sizeof(*iov) * *ret_iov_count);
-	for (l = queue->head, i = 0; l; l = l->link, i++)
+	for (l = queue->head, i = 0; i < *iov_count; l = l->link, i++)
 	{
 		writebuf = (buffer*)l->item;
 		len = writebuf->len - 1 - writebuf->pos;
@@ -278,8 +282,6 @@
 		iov[i].iov_base = buf_getptr(writebuf, len);
 		iov[i].iov_len = len;
 	}
-
-	return iov;
 }
 
 void packet_queue_consume(struct Queue *queue, ssize_t written) {
--- a/netio.h	Mon Mar 02 21:17:41 2015 +0800
+++ b/netio.h	Tue Apr 14 20:44:30 2015 +0800
@@ -44,7 +44,8 @@
 void connect_set_writequeue(struct dropbear_progress_connection *c, struct Queue *writequeue);
 
 /* TODO: writev #ifdef guard */
-struct iovec * packet_queue_to_iovec(struct Queue *queue, int *ret_iov_count);
+/* Fills out iov which contains iov_count slots, returning the number filled in iov_count */
+void packet_queue_to_iovec(struct Queue *queue, struct iovec *iov, unsigned int *iov_count);
 void packet_queue_consume(struct Queue *queue, ssize_t written);
 
 #ifdef DROPBEAR_TCP_FAST_OPEN
--- a/packet.c	Mon Mar 02 21:17:41 2015 +0800
+++ b/packet.c	Tue Apr 14 20:44:30 2015 +0800
@@ -58,8 +58,9 @@
 
 	ssize_t written;
 #ifdef HAVE_WRITEV
-	struct iovec *iov = NULL;
-	int iov_count;
+	/* 50 is somewhat arbitrary */
+	unsigned int iov_count = 50;
+	struct iovec iov[50];
 #endif
 	
 	TRACE2(("enter write_packet"))
@@ -67,7 +68,7 @@
 
 #if defined(HAVE_WRITEV) && (defined(IOV_MAX) || defined(UIO_MAXIOV))
 
-	iov = packet_queue_to_iovec(&ses.writequeue, &iov_count);
+	packet_queue_to_iovec(&ses.writequeue, iov, &iov_count);
 	/* This may return EAGAIN. The main loop sometimes
 	calls write_packet() without bothering to test with select() since
 	it's likely to be necessary */
@@ -75,15 +76,14 @@
 	if (written < 0) {
 		if (errno == EINTR || errno == EAGAIN) {
 			TRACE2(("leave write_packet: EINTR"))
-			m_free(iov);
 			return;
 		} else {
 			dropbear_exit("Error writing: %s", strerror(errno));
 		}
 	}
-	m_free(iov);
 
 	packet_queue_consume(&ses.writequeue, written);
+	ses.writequeue_len -= written;
 
 	if (written == 0) {
 		ses.remoteclosed();
@@ -114,6 +114,8 @@
 		ses.remoteclosed();
 	}
 
+	ses.writequeue_len -= written;
+
 	if (written == len) {
 		/* We've finished with the packet, free it */
 		dequeue(&ses.writequeue);
@@ -571,15 +573,12 @@
     /* stick the MAC on it */
     buf_putbytes(writebuf, mac_bytes, mac_size);
 
-	/* The last byte of the buffer stores the cleartext packet_type. It is not
-	 * transmitted but is used for transmit timeout purposes */
-	buf_putbyte(writebuf, packet_type);
-	/* enqueue the packet for sending. It will get freed after transmission. */
-	buf_setpos(writebuf, 0);
-	enqueue(&ses.writequeue, (void*)writebuf);
+	/* Update counts */
+	ses.kexstate.datatrans += writebuf->len;
+
+	writebuf_enqueue(writebuf, packet_type);
 
 	/* Update counts */
-	ses.kexstate.datatrans += writebuf->len;
 	ses.transseq++;
 
 	now = monotonic_now();
@@ -597,6 +596,16 @@
 	TRACE2(("leave encrypt_packet()"))
 }
 
+void writebuf_enqueue(buffer * writebuf, unsigned char packet_type) {
+	/* The last byte of the buffer stores the cleartext packet_type. It is not
+	 * transmitted but is used for transmit timeout purposes */
+	buf_putbyte(writebuf, packet_type);
+	/* enqueue the packet for sending. It will get freed after transmission. */
+	buf_setpos(writebuf, 0);
+	enqueue(&ses.writequeue, (void*)writebuf);
+	ses.writequeue_len += writebuf->len-1;
+}
+
 
 /* Create the packet mac, and append H(seqno|clearbuf) to the output */
 /* output_mac must have ses.keys->trans.algo_mac->hashsize bytes. */
--- a/packet.h	Mon Mar 02 21:17:41 2015 +0800
+++ b/packet.h	Tue Apr 14 20:44:30 2015 +0800
@@ -28,12 +28,15 @@
 
 #include "includes.h"
 #include "queue.h"
+#include "buffer.h"
 
 void write_packet();
 void read_packet();
 void decrypt_packet();
 void encrypt_packet();
 
+void writebuf_enqueue(buffer * writebuf, unsigned char packet_type);
+
 void process_packet();
 
 void maybe_flush_reply_queue();
--- a/session.h	Mon Mar 02 21:17:41 2015 +0800
+++ b/session.h	Tue Apr 14 20:44:30 2015 +0800
@@ -125,6 +125,7 @@
 							 throughout the code, as handlers fill out this
 							 buffer with the packet to send. */
 	struct Queue writequeue; /* A queue of encrypted packets to send */
+	unsigned int writequeue_len; /* Number of bytes pending to send in writequeue */
 	buffer *readbuf; /* From the wire, decrypted in-place */
 	buffer *payload; /* Post-decompression, the actual SSH packet. 
 						May have extra data at the beginning, will be