On May 31, 12:05pm, Patrick Welche wrote:
} On Thu, Jan 07, 2010 at 06:28:06AM -0800, John Nemeth wrote:
} > On May 29, 1:18pm, Patrick Welche wrote:
} > Whilst I think the client should be fixed, this option is
} > non-contentious. The only thing is I would check to see if any of the
} > other BSDs supports and use the same option letter.
}
} Free/Open don't use 'p', and don't offer similar functionality.
Good enough.
} > The block counter should wrap as is done by both FreeBSD and
} > OpenBSD. This is most likely necessary to support downloading large
} > files on certain devices (i.e. Cisco devices running IOS). I need to
} > further investigate this, but will most likely be implementing this
} > change.
}
} The attached patch does wrap to zero (successfully booted a vista image, and
} the wrap warning appeared in the logs.
}
} Index: tftpd.8
} ===================================================================
} RCS file: /cvsroot/src/libexec/tftpd/tftpd.8,v
} retrieving revision 1.21
} diff -u -r1.21 tftpd.8
} --- tftpd.8 7 Aug 2003 09:46:53 -0000 1.21
} +++ tftpd.8 8 Jan 2010 17:16:46 -0000
} @@ -29,7 +29,7 @@
} .\"
} .\" from: @(#)tftpd.8 8.1 (Berkeley) 6/4/93
} .\"
} -.Dd June 11, 2003
} +.Dd November 13, 2009
This will get changed to the actual commit date.
} .Dt TFTPD 8
} .Os
} .Sh NAME
} @@ -43,6 +43,7 @@
} .Op Fl g Ar group
} .Op Fl l
} .Op Fl n
} +.Op Fl p Ar path separator
} .Op Fl s Ar directory
} .Op Fl u Ar user
} .Op Ar directory ...
} @@ -107,6 +108,11 @@
} .It Fl n
} Suppresses negative acknowledgement of requests for nonexistent
} relative filenames.
} +.It Fl p Ar path separator
} +All occurances of the single character
} +.Ar path separator
} +in the requested filename are replaced with
} +.Sq / .
} .It Fl s Ar directory
} .Nm
} will
} @@ -193,11 +199,15 @@
} and first appeared in
} .Nx 2.0 .
} .Sh BUGS
} -Files larger than 33488896 octets (65535 blocks) cannot be transferred
} -without client and server supporting blocksize negotiation (RFCs
} -2347 and 2348).
} -.Pp
} -Many tftp clients will not transfer files over 16744448 octets (32767 blocks).
} +Files larger than 33,553,919 octets (65535 blocks, last one <512
} +octets) cannot be correctly transferred without client and server
} +supporting blocksize negotiation (RFCs 2347 and 2348). As a kludge,
} +.Nm
} +accepts a sequence of block numbers which wrap to zero after 65535.
I'm not sure I'm crazy about the wording here, but I'm not sure
how I would fix it up. I would probably make it a couple of sentences
and say something about legacy clients that don't support options.
} +.Pp
} +Many tftp clients will not transfer files over 16,776,703 octets
} +(32767 blocks), as they incorrectly count the block number using
} +a signed rather than unsigned 16-bit integer.
} .Sh SECURITY CONSIDERATIONS
} You are
} .Em strongly
} Index: tftpd.c
} ===================================================================
} RCS file: /cvsroot/src/libexec/tftpd/tftpd.c,v
} retrieving revision 1.32
} diff -u -r1.32 tftpd.c
} --- tftpd.c 16 Mar 2009 01:56:21 -0000 1.32
} +++ tftpd.c 8 Jan 2010 17:16:47 -0000
} @@ -107,6 +107,7 @@
} static int suppress_naks;
} static int logging;
} static int secure;
} +static char pathsep = '\0';
} static char *securedir;
}
} struct formats;
} @@ -141,7 +142,7 @@
} {
}
} syslog(LOG_ERR,
} - "Usage: %s [-dln] [-u user] [-g group] [-s directory] [directory ...]",
} + "Usage: %s [-dln] [-u user] [-g group] [-s directory] [-p pathsep] [directory ...]",
} getprogname());
} exit(1);
} }
} @@ -170,7 +171,7 @@
} curuid = getuid();
} curgid = getgid();
}
} - while ((ch = getopt(argc, argv, "dg:lns:u:")) != -1)
} + while ((ch = getopt(argc, argv, "dg:lnp:s:u:w:")) != -1)
What's this -w option?
} switch (ch) {
} case 'd':
} debug++;
} @@ -188,6 +189,11 @@
} suppress_naks = 1;
} break;
}
} + case 'p':
} + pathsep = optarg[0];
} + if (optarg[1] != '\0' || optarg[0] == '\0') usage();
usage() should be on the next line to make the code easier to read.
} + break;
} +
} case 's':
} secure = 1;
} securedir = optarg;
} @@ -662,6 +668,15 @@
} exit(1);
} }
} }
} + /*
} + * Globally replace the path separator given in the -p option
} + * with / to cope with clients expecting a non-unix path separator.
} + */
} + if (pathsep != '\0') {
} + for (cp = filename; *cp != '\0'; ++cp) {
} + if (*cp == pathsep) *cp = '/';
} + }
} + }
} ecode = (*pf->f_validate)(&filename, tp->th_opcode);
} if (logging) {
} syslog(LOG_INFO, "%s: %s request for %s: %s",
} @@ -853,7 +868,7 @@
} case OACK:
} return "OACK";
} default:
} - (void)snprintf(obuf, sizeof(obuf), "*code %d*", code);
} + (void)snprintf(obuf, sizeof(obuf), "*code 0x%x*", code);
} return obuf;
} }
} }
} @@ -918,20 +933,20 @@
} goto abort;
}
} case ACK:
} - if (ap->th_block == 0) {
} + if (etftp && ap->th_block == 0) {
} etftp = 0;
} acklength = 0;
} dp = r_init();
} goto done;
} }
} - if (ap->th_block == block)
} + if (ap->th_block == (u_short)block)
I would use uint16_t to make it clear what size is wanted. If you
need a specific size, it's much better to specify it then make
assumptions about what size a type is.
} goto done;
} if (debug)
} syslog(LOG_DEBUG, "Resync ACK %u != %u",
} (unsigned int)ap->th_block, block);
} /* Re-synchronize with the other side */
} (void) synchnet(peer, tftp_blksize);
} - if (ap->th_block == (block -1))
} + if (ap->th_block == (u_short)(block - 1))
Same thing, use uint16_t.
} goto send_data;
} default:
} syslog(LOG_INFO, "Received %s in sendfile\n",
} @@ -942,6 +957,9 @@
} done:
} if (debug)
} syslog(LOG_DEBUG, "Received ACK for block %u", block);
} + if (block == UINT16_MAX && size == tftp_blksize)
} + syslog(LOG_WARNING,
} + "Block number wrapped (hint: increase block size)");
Don't bother with the hint since the client is usually out of the
sysadmin's control. Maybe also use LOG_DEBUG since this is normal
operation for the clients that do it.
} block++;
} } while (size == tftp_blksize || block == 1);
} abort:
} @@ -980,6 +998,9 @@
} }
} if (debug)
} syslog(LOG_DEBUG, "Sending ACK for block %u\n", block);
} + if (block == UINT16_MAX)
} + syslog(LOG_WARNING,
} + "Block number wrapped (hint: increase block size)");
Where's the wrap? block isn't being wrapped back to 0 nor are
comparisons being done against short/uint16_t. It looks like this will
die at line 1011 when the block number from the client is compared
against 65536. Have you tried uploading a large file?
} block++;
} (void) setjmp(timeoutbuf);
} send_ack:
The only other thing I would do is make wrapping an option in
order to satisfy the purists. With the option, maybe also make it
possible to wrap to 1 instead of 0, since apparently there are some
clients that expect that. However, I don't think that's of vital
importance for the first pass.
}-- End of excerpt from Patrick Welche
--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de