[ECOS] TFTP bug

[Gary Thomas]

On Mon, 2001-10-01 at 05:07, Mark Salter wrote:
> >>>>> Pushparaj  writes:
> 
> > Hi,
> > Today, I looked into the tftp download problem in a more detail. This is what I inferred from
> > the packed view that I captured between the tftp server and the client.
> > I use the following command to load a s record.
> > load c:\image.s19.
> > The target hardware which is running on motorola powerpc 860, sends the first packet over UDP
> > on the port 69. The server is listening on this port and it replies back saying that its port
> > is 1750 and sends the first 512 chunk.
> > After this the board sends the ack for this packet with destination port 1750.
> > This continues for few more 512 chunks of memory. And the transaction is fine with the server
> > sending the packet 'n' and the redboot sending the ack for packet 'n'.
> > Now the server transmits the (n+1) packet and suddenly the board sends the ack for nth packet
> > again with destination port 0.
> 
> I think I see the problem. My guess is that the (n+1) packet is delayed
> from the (n) ACK such that the __udp_recvfrom() times out. __udp_recvfrom
> starts out by zeroing the port number and will fill it in with the server
> port when a packet is received. If no packet is received, the port number
> is left at zero. The tftp client code passes in a pointer to its global
> server sockaddr structure, so it gets trampled by a recvfrom timeout.
> 
> I think this will fix the problem:
> 
> Index: tftp_client.c
> ===================================================================
> RCS file: /home/cvs/ecc/ecc/redboot/current/src/net/tftp_client.c,v
> retrieving revision 1.11
> diff -u -p -5 -c -r1.11 tftp_client.c
> cvs server: conflicting specifications of output style
> *** tftp_client.c	2001/07/27 16:41:38	1.11
> --- tftp_client.c	2001/09/30 20:05:37
> *************** tftp_stream_read(char *buf,
> *** 273,282 ****
> --- 273,283 ----
>   {
>       int total_bytes = 0;
>       int size, recv_len, data_len;
>       struct timeval timeout;
>       struct tftphdr *hdr = (struct tftphdr *)tftp_stream.data;
> +     struct sockaddr_in tmp_addr;
>   
>       while (total_bytes < len) {
>           // Move any bytes which we've already read/buffered
>           if (tftp_stream.avail > 0) {
>               size = tftp_stream.avail;
> *************** tftp_stream_read(char *buf,
> *** 303,322 ****
>                   break;
>               }
>               timeout.tv_sec = TFTP_TIMEOUT_PERIOD;
>               timeout.tv_usec = 0;
>               recv_len = sizeof(tftp_stream.data);
> !             if ((data_len = __udp_recvfrom(&tftp_stream.data[0], recv_len, &tftp_stream.from_addr, 
>                                              &tftp_stream.local_addr,  &timeout)) < 0) {
>                   // No data, try again
>                   if ((++tftp_stream.total_timeouts > TFTP_TIMEOUT_MAX) || 
>                       (tftp_stream.last_good_block == 0)) {
>                       // Timeout - no data received
>                       *err = TFTP_TIMEOUT;
>                       return -1;
>                   }
>               } else {
>                   if (ntohs(hdr->th_opcode) == DATA) {
>                       if (ntohs(hdr->th_block) == (tftp_stream.last_good_block+1)) {
>                           // Consume this data
>                           data_len -= 4;  /* Sizeof TFTP header */
>                           tftp_stream.avail = tftp_stream.actual_len = data_len;
> --- 304,325 ----
>                   break;
>               }
>               timeout.tv_sec = TFTP_TIMEOUT_PERIOD;
>               timeout.tv_usec = 0;
>               recv_len = sizeof(tftp_stream.data);
> !             tmp_addr = tftp_stream.from_addr;
> !             if ((data_len = __udp_recvfrom(&tftp_stream.data[0], recv_len, &tmp_addr, 
>                                              &tftp_stream.local_addr,  &timeout)) < 0) {
>                   // No data, try again
>                   if ((++tftp_stream.total_timeouts > TFTP_TIMEOUT_MAX) || 
>                       (tftp_stream.last_good_block == 0)) {
>                       // Timeout - no data received
>                       *err = TFTP_TIMEOUT;
>                       return -1;
>                   }
>               } else {
> +                 tftp_stream.from_addr = tmp_addr;
>                   if (ntohs(hdr->th_opcode) == DATA) {
>                       if (ntohs(hdr->th_block) == (tftp_stream.last_good_block+1)) {
>                           // Consume this data
>                           data_len -= 4;  /* Sizeof TFTP header */
>                           tftp_stream.avail = tftp_stream.actual_len = data_len;
> 
> 
> --Mark

Or, alternatively since the "from" address is only valid IFF UDP 
receives a packet within the time limit, don't change it until the
packet is received.  This patch would have the same effect, but would
fix it at the source (as it were).

Index: redboot/current/src/net/udp.c
===================================================================
RCS file: /home/cvs/ecc/ecc/redboot/current/src/net/udp.c,v
retrieving revision 1.7
diff -u -5 -p -r1.7 udp.c
--- redboot/current/src/net/udp.c	23 Aug 2001 21:27:13 -0000	1.7
+++ redboot/current/src/net/udp.c	30 Sep 2001 22:47:37 -0000
@@ -233,11 +233,10 @@ __udp_recvfrom(char *data, int len, stru
 {
     int res, my_port, total_ms;
     udp_socket_t skt;
     unsigned long start;
 
-    server->sin_port = 0;
     my_port = ntohs(local->sin_port);
     if (__udp_install_listener(&skt, my_port, __udp_recvfrom_handler) < 0) {
         return -1;
     }
     recvfrom_buf = data;