libexec/tftpd handled the case where we'd get an ack for the previous

block by flushing the data on the socket and waiting for a retransmit
by timeout to occur.

my stuff (usr.sbin/tftpd) had XXXs and failure in those places.

this diff fixes that.

this should address the problems that sthen and jcs have been having
(and which i was finally able to reproduce here).

it also avoids reusing the clients buffer to both send and recv
frames. we recv onto the stack now so retry always sends what we
originally built for the peer.

tested by and ok jcs@ sthen@

1 files changed, 89 insertions, 19 deletions

diff --git a/usr.sbin/tftpd/tftpd.c b/usr.sbin/tftpd/tftpd.c
index 52df5c13447..70a82bba83e 100644
--- a/usr.sbin/tftpd/tftpd.c
+++ b/usr.sbin/tftpd/tftpd.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: tftpd.c,v 1.5 2012/03/15 07:18:35 nicm Exp $	*/
+/*	$OpenBSD: tftpd.c,v 1.6 2012/07/10 07:21:34 dlg Exp $	*/
 
 /*
  * Copyright (c) 2012 David Gwynne <dlg@uq.edu.au>
@@ -162,6 +162,7 @@ int		tftpd_listen(const char *, const char *, int);
 void		tftpd_events(void);
 void		tftpd_recv(int, short, void *);
 int		retry(struct tftp_client *);
+int		tftp_flush(struct tftp_client *);
 
 void		tftp(struct tftp_client *, struct tftphdr *, size_t);
 void		tftp_open(struct tftp_client *, const char *);
@@ -1077,16 +1078,19 @@ tftp_rrq_ack(int fd, short events, void *arg)
 {
 	struct tftp_client *client = arg;
 	struct tftphdr *ap; /* ack packet */
+	char rbuf[SEGSIZE_MIN];
 	ssize_t n;
 
 	if (events & EV_TIMEOUT) {
-		if (retry(client) == -1)
+		if (retry(client) == -1) {
+			lwarn("%s: retry", getip(&client->ss));
 			goto done;
+		}
 
 		return;
 	}
 
-	n = recv(fd, client->buf, client->packet_size, 0);
+	n = recv(fd, rbuf, sizeof(rbuf), 0);
 	if (n == -1) {
 		switch (errno) {
 		case EINTR:
@@ -1095,18 +1099,35 @@ tftp_rrq_ack(int fd, short events, void *arg)
 			return;
 
 		default:
+			lwarn("%s: recv", getip(&client->ss));
 			goto done;
 		}
 	}
 
-	ap = (struct tftphdr *)client->buf;
+	ap = (struct tftphdr *)rbuf;
 	ap->th_opcode = ntohs((u_short)ap->th_opcode);
 	ap->th_block = ntohs((u_short)ap->th_block);
 
-	if (ap->th_opcode == ERROR)
+	switch (ap->th_opcode) {
+	case ERROR:
 		goto done;
-	if (ap->th_opcode != ACK || ap->th_block != client->block)
-		goto done; /* XXX */
+	case ACK:
+		break;
+	default:
+		goto retry;
+	}
+
+	if (ap->th_block != client->block) {
+		if (tftp_flush(client) == -1) {
+			lwarnx("%s: flush", getip(&client->ss));
+			goto done;
+		}
+
+		if (ap->th_block != (client->block - 1))
+			goto done;
+
+		goto retry;
+	}
 
 	if (client->buflen != client->packet_size) {
 		/* this was the last packet in the stream */
@@ -1117,10 +1138,37 @@ tftp_rrq_ack(int fd, short events, void *arg)
 	file_read(client);
 	return;
 
+retry:
+	event_add(&client->sev, &client->tv);
+	return;
+
 done:
 	client_free(client);
 }
 
+int
+tftp_flush(struct tftp_client *client)
+{
+	char rbuf[SEGSIZE_MIN];
+	ssize_t n;
+
+	for (;;) {
+		n = recv(client->sock, rbuf, sizeof(rbuf), 0);
+		if (n == -1) {
+			switch (errno) {
+			case EAGAIN:
+				return (0);
+
+			case EINTR:
+				break;
+
+			default:
+				return (-1);
+			}
+		}
+	}
+}
+
 void
 recvfile(struct tftp_client *client)
 {
@@ -1137,8 +1185,8 @@ tftp_wrq_ack_packet(struct tftp_client *client)
 	ap->th_opcode = htons((u_short)ACK);
 	ap->th_block = htons(client->block);
 
-	client->retries = RETRIES;
 	client->buflen = 4;
+	client->retries = RETRIES;
 
 	return (send(client->sock, client->buf, client->buflen, 0) != 4);
 }
@@ -1159,19 +1207,22 @@ tftp_wrq_ack(struct tftp_client *client)
 void
 tftp_wrq(int fd, short events, void *arg)
 {
+	char wbuf[SEGSIZE_MAX + 4];
 	struct tftp_client *client = arg;
 	struct tftphdr *dp;
 	ssize_t n;
 	int i;
 
 	if (events & EV_TIMEOUT) {
-		if (retry(client) == -1)
+		if (retry(client) == -1) {
+			lwarn("%s", getip(&client->ss));
 			goto done;
+		}
 
 		return;
 	}
 
-	n = recv(client->sock, client->buf, client->packet_size, 0);
+	n = recv(fd, wbuf, client->packet_size, 0);
 	if (n == -1) {
 		switch (errno) {
 		case EINTR:
@@ -1187,18 +1238,33 @@ tftp_wrq(int fd, short events, void *arg)
 	if (n < 4)
 		goto done;
 
-	dp = (struct tftphdr *)client->buf;
+	dp = (struct tftphdr *)wbuf;
 	dp->th_opcode = ntohs((u_short)dp->th_opcode);
 	dp->th_block = ntohs((u_short)dp->th_block);
 
-	if (dp->th_opcode != DATA)
+	switch (dp->th_opcode) {
+	case ERROR:
 		goto done;
+	case DATA:
+		break;
+	default:
+		goto retry;
+	}
 
-	if (dp->th_block != client->block)
-		goto done;
+	if (dp->th_block != client->block) {
+		if (tftp_flush(client) == -1) {
+			lwarnx("%s: flush", getip(&client->ss));
+			goto done;
+		}
+
+		if (dp->th_block != (client->block - 1))
+			goto done;
+
+		goto retry;
+	}
 
 	for (i = 4; i < n; i++) {
-		if (client->fputc(client, client->buf[i]) == EOF) {
+		if (client->fputc(client, wbuf[i]) == EOF) {
 			lwarn("tftp wrq");
 			goto done;
 		}
@@ -1317,11 +1383,13 @@ error:
 int
 retry(struct tftp_client *client)
 {
-	if (--client->retries == 0)
+	if (--client->retries == 0) {
+		errno = ETIMEDOUT;
 		return (-1);
+	}
 
 	if (send(client->sock, client->buf, client->buflen, 0) == -1)
-		return -1;
+		return (-1);
 
 	event_add(&client->sev, &client->tv);
 
@@ -1336,8 +1404,10 @@ oack_done(int fd, short events, void *arg)
 	ssize_t n;
 
 	if (events & EV_TIMEOUT) {
-		if (retry(client) == -1)
+		if (retry(client) == -1) {
+			lwarn("%s", getip(&client->ss));
 			goto done;
+		}
 
 		return;
 	}
@@ -1351,7 +1421,7 @@ oack_done(int fd, short events, void *arg)
 			return;
 
 		default:
-			lwarn("recv");
+			lwarn("%s: recv", getip(&client->ss));
 			goto done;
 		}
 	}