From d81b1d76a0e1d877ee7fd2ba28e49b5d148887a7 Mon Sep 17 00:00:00 2001 From: Simon Kelley Date: Thu, 21 Aug 2025 12:49:09 +0100 Subject: [PATCH] Fix TFTP problems with large files. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit TFTP fails with a timeout when the transfer exceeds 128K blocks. The block field in an ACK packet is 16 bits, so large transfers need to deal with this field wrapping. Testing failure meant that it worked for the first wrap, but not for subsequent ones. Having fixed the block number problem, file size calculations accidentally being done in 32 bits not 64 bits then broke transfers larger than 2G or maybe 4G. The first problem was probably introduced in commit ebef27f321a03c0fc6d8c887bb177e33f386f192 and has not appeared in a stable release. The second appears to have been there forever. Clearly, nobody is mad enough to transfer multi-gigabyte files over TFTP. Thanks to Jean-François JUBLIN for spotting this and supplying useful packet dumps that made it easy to diagnose. --- src/dnsmasq.h | 2 +- src/forward.c | 2 +- src/tftp.c | 38 ++++++++++++++++++++++++++------------ 3 files changed, 28 insertions(+), 14 deletions(-) diff --git a/src/dnsmasq.h b/src/dnsmasq.h index 9d38256..b66c58b 100644 --- a/src/dnsmasq.h +++ b/src/dnsmasq.h @@ -1118,7 +1118,7 @@ struct tftp_file { struct tftp_transfer { int sockfd; time_t retransmit, start; - unsigned int lastack, block, blocksize, windowsize, timeout, expansion; + unsigned int lastack, block, ackblock, blocksize, windowsize, timeout, expansion; off_t offset; union mysockaddr peer; union all_addr source; diff --git a/src/forward.c b/src/forward.c index 340e681..310b84e 100644 --- a/src/forward.c +++ b/src/forward.c @@ -3106,7 +3106,7 @@ static void free_frec(struct frec *f) /* Impose an absolute limit of 4*TIMEOUT before we wipe things (for random sockets). If force is set, always return a result, even if we have - to allocate above the limit, and don'y free any records. + to allocate above the limit, and don't free any records. This is set when allocating for DNSSEC to avoid cutting off the branch we are sitting on. */ static struct frec *get_new_frec(time_t now, struct server *master, int force) diff --git a/src/tftp.c b/src/tftp.c index 5b8caf1..8f3768b 100644 --- a/src/tftp.c +++ b/src/tftp.c @@ -326,6 +326,7 @@ static void tftp_request(struct listener *listen, time_t now) transfer->start = now; transfer->backoff = 1; transfer->block = 1; + transfer->ackblock = 0; transfer->blocksize = 512; transfer->windowsize = 1; @@ -754,26 +755,39 @@ static void handle_tftp(time_t now, struct tftp_transfer *transfer, ssize_t len) { if (ntohs(mess->op) == OP_ACK) { - /* try and handle 16-bit blockno wrap-around */ - unsigned int block = (unsigned short)ntohs(mess->block); - if (block < transfer->lastack) - block |= transfer->block & 0xffff0000; + /* Handle 16-bit block number wrap-around. */ + u16 new = ntohs(mess->block); + u16 prev = transfer->ackblock; - /* ignore duplicate ACKs and ACKs for blocks we've not yet sent. */ - if (block >= transfer->lastack && - block <= transfer->block) + transfer->ackblock = new | (transfer->ackblock & 0xffff0000); + + /* If the last ack received was in the top quarter of a 64k block + and this one is in the bottom quarter, assume it has wrapped. + + Since this is UDP and an old packet can in theory wander in we may also + need to drop back to a previous segment. Such an ACK is ignored below; + here we're just getting the most likely 32 bit value from the + 16 bits that we have. */ + if (new <= 0x4000 && prev >= 0xc000) + transfer->ackblock += 0x10000; + else if (new >= 0xc000 && prev <= 0x4000 && transfer->ackblock != 0) + transfer->ackblock -= 0x10000; + + /* Ignore duplicate ACKs and ACKs for blocks we've not yet sent. */ + if (transfer->ackblock >= transfer->lastack && + transfer->ackblock <= transfer->block) { /* Got ack, move forward and ensure we take the (re)transmit path */ transfer->retransmit = transfer->start = now; transfer->backoff = 0; - transfer->lastack = block + 1; + transfer->lastack = transfer->ackblock + 1; /* We have no easy function from block no. to file offset when expanding line breaks in netascii mode, so we update the offset here as each block is acknowledged. This explains why the window size must be one for a netascii transfer; to avoid the block no. doing anything other than incrementing by one. */ - if (transfer->netascii && block != 0) + if (transfer->netascii && transfer->ackblock != 0) transfer->offset += transfer->blocksize - transfer->expansion; } } @@ -936,13 +950,13 @@ static ssize_t get_block(char *packet, struct tftp_transfer *transfer) size_t size; if (!transfer->netascii) - transfer->offset = (transfer->block - 1) * transfer->blocksize; + transfer->offset = (off_t)(transfer->block - 1) * (off_t)transfer->blocksize; if (transfer->offset > transfer->file->size) return 0; /* finished */ - if ((size = transfer->file->size - transfer->offset) > transfer->blocksize) - size = transfer->blocksize; + if ((size = transfer->file->size - transfer->offset) > (size_t)transfer->blocksize) + size = (size_t)transfer->blocksize; mess->op = htons(OP_DATA); mess->block = htons((unsigned short)(transfer->block));