From c4d12a743c73a5b88a8705ca68ff620ce0f8bba7 Mon Sep 17 00:00:00 2001 From: Stefan Weil Date: Tue, 4 Sep 2012 23:20:35 +0200 Subject: [PATCH 1/6] slirp: Remove wrong type casts ins debug statements The type casts of pointers to long are not allowed when sizeof(pointer) != sizeof(long). Signed-off-by: Stefan Weil Signed-off-by: Jan Kiszka --- slirp/tcp_subr.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/slirp/tcp_subr.c b/slirp/tcp_subr.c index 025b374367..5890d7a827 100644 --- a/slirp/tcp_subr.c +++ b/slirp/tcp_subr.c @@ -114,9 +114,9 @@ tcp_respond(struct tcpcb *tp, struct tcpiphdr *ti, struct mbuf *m, int win = 0; DEBUG_CALL("tcp_respond"); - DEBUG_ARG("tp = %lx", (long)tp); - DEBUG_ARG("ti = %lx", (long)ti); - DEBUG_ARG("m = %lx", (long)m); + DEBUG_ARG("tp = %p", tp); + DEBUG_ARG("ti = %p", ti); + DEBUG_ARG("m = %p", m); DEBUG_ARG("ack = %u", ack); DEBUG_ARG("seq = %u", seq); DEBUG_ARG("flags = %x", flags); From e56afbc54a2132c56931f44bae1992c28119944f Mon Sep 17 00:00:00 2001 From: Stefan Weil Date: Tue, 4 Sep 2012 23:20:36 +0200 Subject: [PATCH 2/6] slirp: Fix error reported by static code analysis Report from smatch: slirp/tcp_subr.c:127 tcp_respond(17) error: we previously assumed 'tp' could be null (see line 124) Return if 'tp' is NULL. Signed-off-by: Stefan Weil Signed-off-by: Jan Kiszka --- slirp/tcp_subr.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/slirp/tcp_subr.c b/slirp/tcp_subr.c index 5890d7a827..1542e43619 100644 --- a/slirp/tcp_subr.c +++ b/slirp/tcp_subr.c @@ -124,7 +124,7 @@ tcp_respond(struct tcpcb *tp, struct tcpiphdr *ti, struct mbuf *m, if (tp) win = sbspace(&tp->t_socket->so_rcv); if (m == NULL) { - if ((m = m_get(tp->t_socket->slirp)) == NULL) + if (!tp || (m = m_get(tp->t_socket->slirp)) == NULL) return; tlen = 0; m->m_data += IF_MAXLINKHDR; From 78be056628c76ff73eedeade86fde44b97343c79 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Herv=C3=A9=20Poussineau?= Date: Mon, 10 Sep 2012 20:52:25 +0200 Subject: [PATCH 3/6] slirp: improve TFTP performance MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When transferring a file, keep it open during the whole transfer, instead of opening/closing it for each block. Signed-off-by: Hervé Poussineau Reviewed-by: Aurelien Jarno Signed-off-by: Jan Kiszka --- slirp/tftp.c | 32 ++++++++++++++++++-------------- slirp/tftp.h | 1 + 2 files changed, 19 insertions(+), 14 deletions(-) diff --git a/slirp/tftp.c b/slirp/tftp.c index b78765f3af..520dbd6d3c 100644 --- a/slirp/tftp.c +++ b/slirp/tftp.c @@ -37,6 +37,10 @@ static inline void tftp_session_update(struct tftp_session *spt) static void tftp_session_terminate(struct tftp_session *spt) { + if (spt->fd >= 0) { + close(spt->fd); + spt->fd = -1; + } g_free(spt->filename); spt->slirp = NULL; } @@ -54,7 +58,7 @@ static int tftp_session_allocate(Slirp *slirp, struct tftp_t *tp) /* sessions time out after 5 inactive seconds */ if ((int)(curtime - spt->timestamp) > 5000) { - g_free(spt->filename); + tftp_session_terminate(spt); goto found; } } @@ -64,6 +68,7 @@ static int tftp_session_allocate(Slirp *slirp, struct tftp_t *tp) found: memset(spt, 0, sizeof(*spt)); memcpy(&spt->client_ip, &tp->ip.ip_src, sizeof(spt->client_ip)); + spt->fd = -1; spt->client_port = tp->udp.uh_sport; spt->slirp = slirp; @@ -95,24 +100,23 @@ static int tftp_session_find(Slirp *slirp, struct tftp_t *tp) static int tftp_read_data(struct tftp_session *spt, uint16_t block_nr, uint8_t *buf, int len) { - int fd; - int bytes_read = 0; + int bytes_read = 0; - fd = open(spt->filename, O_RDONLY | O_BINARY); + if (spt->fd < 0) { + spt->fd = open(spt->filename, O_RDONLY | O_BINARY); + } - if (fd < 0) { - return -1; - } + if (spt->fd < 0) { + return -1; + } - if (len) { - lseek(fd, block_nr * 512, SEEK_SET); + if (len) { + lseek(spt->fd, block_nr * 512, SEEK_SET); - bytes_read = read(fd, buf, len); - } + bytes_read = read(spt->fd, buf, len); + } - close(fd); - - return bytes_read; + return bytes_read; } static int tftp_send_oack(struct tftp_session *spt, diff --git a/slirp/tftp.h b/slirp/tftp.h index 72e5e91bef..9c364ea28e 100644 --- a/slirp/tftp.h +++ b/slirp/tftp.h @@ -33,6 +33,7 @@ struct tftp_t { struct tftp_session { Slirp *slirp; char *filename; + int fd; struct in_addr client_ip; uint16_t client_port; From 4aa401f39e048e71020cceb59f126ab941095a42 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Herv=C3=A9=20Poussineau?= Date: Thu, 13 Sep 2012 12:39:36 +0200 Subject: [PATCH 4/6] slirp: Handle more than 65535 blocks in TFTP transfers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RFC 1350 does not mention block count roll-over. However, a lot of TFTP servers implement it to be able to transmit big files, so do it also. Current block size is 512 bytes, so TFTP files were limited to 32 MB. Signed-off-by: Hervé Poussineau Signed-off-by: Jan Kiszka --- slirp/tftp.c | 24 ++++++++++-------------- slirp/tftp.h | 1 + 2 files changed, 11 insertions(+), 14 deletions(-) diff --git a/slirp/tftp.c b/slirp/tftp.c index 520dbd6d3c..c6a5df2dd2 100644 --- a/slirp/tftp.c +++ b/slirp/tftp.c @@ -97,7 +97,7 @@ static int tftp_session_find(Slirp *slirp, struct tftp_t *tp) return -1; } -static int tftp_read_data(struct tftp_session *spt, uint16_t block_nr, +static int tftp_read_data(struct tftp_session *spt, uint32_t block_nr, uint8_t *buf, int len) { int bytes_read = 0; @@ -197,19 +197,14 @@ out: tftp_session_terminate(spt); } -static int tftp_send_data(struct tftp_session *spt, - uint16_t block_nr, - struct tftp_t *recv_tp) +static int tftp_send_next_block(struct tftp_session *spt, + struct tftp_t *recv_tp) { struct sockaddr_in saddr, daddr; struct mbuf *m; struct tftp_t *tp; int nobytes; - if (block_nr < 1) { - return -1; - } - m = m_get(spt->slirp); if (!m) { @@ -223,7 +218,7 @@ static int tftp_send_data(struct tftp_session *spt, m->m_data += sizeof(struct udpiphdr); tp->tp_op = htons(TFTP_DATA); - tp->x.tp_data.tp_block_nr = htons(block_nr); + tp->x.tp_data.tp_block_nr = htons((spt->block_nr + 1) & 0xffff); saddr.sin_addr = recv_tp->ip.ip_dst; saddr.sin_port = recv_tp->udp.uh_dport; @@ -231,7 +226,7 @@ static int tftp_send_data(struct tftp_session *spt, daddr.sin_addr = spt->client_ip; daddr.sin_port = spt->client_port; - nobytes = tftp_read_data(spt, block_nr - 1, tp->x.tp_data.tp_buf, 512); + nobytes = tftp_read_data(spt, spt->block_nr, tp->x.tp_data.tp_buf, 512); if (nobytes < 0) { m_free(m); @@ -255,6 +250,7 @@ static int tftp_send_data(struct tftp_session *spt, tftp_session_terminate(spt); } + spt->block_nr++; return 0; } @@ -373,7 +369,8 @@ static void tftp_handle_rrq(Slirp *slirp, struct tftp_t *tp, int pktlen) } } - tftp_send_data(spt, 1, tp); + spt->block_nr = 0; + tftp_send_next_block(spt, tp); } static void tftp_handle_ack(Slirp *slirp, struct tftp_t *tp, int pktlen) @@ -386,9 +383,8 @@ static void tftp_handle_ack(Slirp *slirp, struct tftp_t *tp, int pktlen) return; } - if (tftp_send_data(&slirp->tftp_sessions[s], - ntohs(tp->x.tp_data.tp_block_nr) + 1, - tp) < 0) { + if (tftp_send_next_block(&slirp->tftp_sessions[s], + tp) < 0) { return; } } diff --git a/slirp/tftp.h b/slirp/tftp.h index 9c364ea28e..51704e4874 100644 --- a/slirp/tftp.h +++ b/slirp/tftp.h @@ -37,6 +37,7 @@ struct tftp_session { struct in_addr client_ip; uint16_t client_port; + uint32_t block_nr; int timestamp; }; From eb7faf0e3a45b0a0089035f972080ca4bd2e15ce Mon Sep 17 00:00:00 2001 From: Jan Kiszka Date: Thu, 13 Sep 2012 12:44:27 +0200 Subject: [PATCH 5/6] slirp: Remove unused return value of tftp_send_next_block No caller actually makes use of this value, so let's simplify the code. Signed-off-by: Jan Kiszka --- slirp/tftp.c | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/slirp/tftp.c b/slirp/tftp.c index c6a5df2dd2..cf7e3b8238 100644 --- a/slirp/tftp.c +++ b/slirp/tftp.c @@ -197,8 +197,8 @@ out: tftp_session_terminate(spt); } -static int tftp_send_next_block(struct tftp_session *spt, - struct tftp_t *recv_tp) +static void tftp_send_next_block(struct tftp_session *spt, + struct tftp_t *recv_tp) { struct sockaddr_in saddr, daddr; struct mbuf *m; @@ -208,7 +208,7 @@ static int tftp_send_next_block(struct tftp_session *spt, m = m_get(spt->slirp); if (!m) { - return -1; + return; } memset(m->m_data, 0, m->m_size); @@ -235,7 +235,7 @@ static int tftp_send_next_block(struct tftp_session *spt, tftp_send_error(spt, 1, "File not found", tp); - return -1; + return; } m->m_len = sizeof(struct tftp_t) - (512 - nobytes) - @@ -251,7 +251,6 @@ static int tftp_send_next_block(struct tftp_session *spt, } spt->block_nr++; - return 0; } static void tftp_handle_rrq(Slirp *slirp, struct tftp_t *tp, int pktlen) @@ -383,10 +382,7 @@ static void tftp_handle_ack(Slirp *slirp, struct tftp_t *tp, int pktlen) return; } - if (tftp_send_next_block(&slirp->tftp_sessions[s], - tp) < 0) { - return; - } + tftp_send_next_block(&slirp->tftp_sessions[s], tp); } static void tftp_handle_error(Slirp *slirp, struct tftp_t *tp, int pktlen) From 95b1ad7ad86793c27ab8e9987be69571937900d1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Herv=C3=A9=20Poussineau?= Date: Thu, 13 Sep 2012 07:55:01 +0200 Subject: [PATCH 6/6] slirp: Implement TFTP Blocksize option MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This option is described in RFC 1783. As this is only an optional field, we may ignore it in some situations and handle it in some others. However, MS Windows 2003 PXE boot client requests a block size of the MTU (most of the times 1472 bytes), and doesn't work if the option is not acknowledged (with whatever value). According to the RFC 1783, we cannot acknowledge the option with a bigger value than the requested one. As current implementation is using 512 bytes by block, accept the option with a value of 512 if the option was specified, and don't acknowledge it if it is not present or less than 512 bytes. Signed-off-by: Hervé Poussineau Signed-off-by: Jan Kiszka --- slirp/tftp.c | 42 +++++++++++++++++++++++++++++++++--------- 1 file changed, 33 insertions(+), 9 deletions(-) diff --git a/slirp/tftp.c b/slirp/tftp.c index cf7e3b8238..1a79c45cfb 100644 --- a/slirp/tftp.c +++ b/slirp/tftp.c @@ -120,13 +120,13 @@ static int tftp_read_data(struct tftp_session *spt, uint32_t block_nr, } static int tftp_send_oack(struct tftp_session *spt, - const char *key, uint32_t value, + const char *keys[], uint32_t values[], int nb, struct tftp_t *recv_tp) { struct sockaddr_in saddr, daddr; struct mbuf *m; struct tftp_t *tp; - int n = 0; + int i, n = 0; m = m_get(spt->slirp); @@ -140,10 +140,12 @@ static int tftp_send_oack(struct tftp_session *spt, m->m_data += sizeof(struct udpiphdr); tp->tp_op = htons(TFTP_OACK); - n += snprintf(tp->x.tp_buf + n, sizeof(tp->x.tp_buf) - n, "%s", - key) + 1; - n += snprintf(tp->x.tp_buf + n, sizeof(tp->x.tp_buf) - n, "%u", - value) + 1; + for (i = 0; i < nb; i++) { + n += snprintf(tp->x.tp_buf + n, sizeof(tp->x.tp_buf) - n, "%s", + keys[i]) + 1; + n += snprintf(tp->x.tp_buf + n, sizeof(tp->x.tp_buf) - n, "%u", + values[i]) + 1; + } saddr.sin_addr = recv_tp->ip.ip_dst; saddr.sin_port = recv_tp->udp.uh_dport; @@ -259,6 +261,9 @@ static void tftp_handle_rrq(Slirp *slirp, struct tftp_t *tp, int pktlen) int s, k; size_t prefix_len; char *req_fname; + const char *option_name[2]; + uint32_t option_value[2]; + int nb_options = 0; /* check if a session already exists and if so terminate it */ s = tftp_session_find(slirp, tp); @@ -336,7 +341,7 @@ static void tftp_handle_rrq(Slirp *slirp, struct tftp_t *tp, int pktlen) return; } - while (k < pktlen) { + while (k < pktlen && nb_options < ARRAY_SIZE(option_name)) { const char *key, *value; key = &tp->x.tp_buf[k]; @@ -363,11 +368,30 @@ static void tftp_handle_rrq(Slirp *slirp, struct tftp_t *tp, int pktlen) } } - tftp_send_oack(spt, "tsize", tsize, tp); - return; + option_name[nb_options] = "tsize"; + option_value[nb_options] = tsize; + nb_options++; + } else if (strcasecmp(key, "blksize") == 0) { + int blksize = atoi(value); + + /* If blksize option is bigger than what we will + * emit, accept the option with our packet size. + * Otherwise, simply do as we didn't see the option. + */ + if (blksize >= 512) { + option_name[nb_options] = "blksize"; + option_value[nb_options] = 512; + nb_options++; + } } } + if (nb_options > 0) { + assert(nb_options <= ARRAY_SIZE(option_name)); + tftp_send_oack(spt, option_name, option_value, nb_options, tp); + return; + } + spt->block_nr = 0; tftp_send_next_block(spt, tp); }