On Tue, Dec 30, 2008 at 01:20:46PM -0800, Chad Monroe wrote:
> Below is my first attempt at a patch for option negotiation. I have not written
> any test cases for it, but have tested each option in my own app which I use to
> download files (RRQ). I have not tested upload (WRQ) but support is there for
> it and it should use the same code path, however I'd recommend we test it. Some
> misc. notes regarding the fix:
[...]
> My one main concern is in the tftp_parse_option_ack() function. Am I correct in
> returning CURLE_TFTP_ILLEGAL in the cases where the option ACK is malformed or
> values in the option ACK are out of range?

The other option would be to log a warning and just continue as though the
server doesn't support option negotiation.

Here are a few comments on the patch.

> +.SH TFTP OPTIONS
> +.IP CURLOPT_TFTPBLKSIZE
> +Specify block size to use for TFTP data transmission. Valid range as per
> +RFC 2348 is 8-65464 bytes. The default of 512 bytes will be used if this
> +option is not specified.

This should probably mention that use of this block size is dependent
on support by the remote server.

> +static const char *tftp_option_get(const char *buf, int len, const char
> **option, const char **value)
> +{
> + int i;
> +
> + for (i = 0; i < len && buf[i]; i++);

This idiom is confusing to read and will produce compiler warnings on some
compilers. At the very least, the semicolon should be moved to a new line
and a comment added that the body is empty. Better yet would be to simply
use the optimized function strlen (or strnlen).

> +
> + if (i == len)
> + return NULL;
> + *option = buf;
> +
> + for (i++; i < len && buf[i]; i++);
> +
> + if (i == len)
> + return NULL;
> + *value = &buf[strlen(*option)+1];
> +
> + return &buf[i+1];
> +}
> +
> +static CURLcode tftp_parse_option_ack(tftp_state_data_t *state, const char
> *ptr, int len)
> +{
> + const char *tmp = ptr;
> + struct SessionHandle *data = state->conn->data;
> +
> + /* if OACK doesn't contain blksize option, the default (512) must be used */
> + state->blksize = TFTP_BLKSIZE_DEFAULT;
> +
> + while (tmp < ptr + len) {
> + const char *option, *value;
> +
> + tmp = tftp_option_get(tmp, len + (ptr - tmp), &option, &value);

The parenthesis here are confusing, IMHO.

> + if(tmp == NULL) {
> + failf(data, "%s\n", "Malformed ACK packet, rejecting");

failf strings shouldn't end with \n. Also, using %s is redundant--just put
the message in there.

> + return CURLE_TFTP_ILLEGAL;
> + }
> +
> + if(strncasecmp(option, TFTP_OPTION_BLKSIZE, strlen(TFTP_OPTION_BLKSIZE)) =
> = 0) {

Use checkprefix here, as that's more portable and doesn't break in some
locales. Same goes for all the strncasecmp calls.

> + }
> + else if(strncasecmp(option, TFTP_OPTION_TSIZE, strlen(TFTP_OPTION_TSIZE))
> == 0) {
> + int tsize;
> +
> + tsize = strtol( value, NULL, 10 );

This should probably be using a long (or curl_off_t) to support the (rather
improbable but possible) case of downloading a > 2 GB file via TFTP.

> +static int tftp_option_add(tftp_state_data_t *state, int csize, char *buf,
> char *option)

option should be const char*.

> +{
> + if( ( strlen(option) + csize + 1 ) > state->blksize )
> + return 0;
> + strcpy(buf, option);
> + return( strlen(option) + 1 );

This could be simplified with Curl_strlcat.

> +}
> +
> static CURLcode tftp_send_first(tftp_state_data_t *state, tftp_event_t event)
> {
> int sbytes;
> const char *mode = "octet";
> char *filename;
> + char buf[8];
> struct SessionHandle *data = state->conn->data;
> CURLcode res = CURLE_OK;
>
> @@ -318,7 +419,7 @@
> /* If we are uploading, send an WRQ */
> setpacketevent(&state->spacket, TFTP_EVENT_WRQ);
> state->conn->data->req.upload_fromhere =
> - (char *)&state->spacket.data[4];
> + (char *)state->spacket.data+4;
> if(data->set.infilesize != -1)
> Curl_pgrsSetUploadSize(data, data->set.infilesize);
> }
> @@ -333,11 +434,30 @@
> if(!filename)
> return CURLE_OUT_OF_MEMORY;
>
> - snprintf((char *)&state->spacket.data[2],
> - TFTP_BLOCKSIZE,
> + snprintf((char *)state->spacket.data+2,
> + state->blksize,
> "%s%c%s%c", filename, '\0', mode, '\0');
> sbytes = 4 + (int)strlen(filename) + (int)strlen(mode);
> - sbytes = sendto(state->sockfd, (void *)&state->spacket,
> +
> + /* add tsize option */
> + sbytes += tftp_option_add(state, sbytes,
> + (char *)state->spacket.data+sbytes, TFTP_OPTION_TSIZE);

It would be nice if there were some way to disable sending these options
in case a server doesn't react well to their presence. It could easily be
done by adding them only if the user explicitly specified a block size, but
then you lose the potential benefit of an automatic tsize in all cases. If
might be a sufficient trade-off, though, since most new TFTP-using apps are
going to want to set a large blocksize anyway.

> + **********************************************************/
> +static CURLcode tftp_disconnect(struct connectdata *conn)
> +{
> + tftp_state_data_t *state = conn->data->state.proto.tftp;
> +
> + /* done, free dynamically allocated pkt buffers */
> + if(state) {
> + if(state->rpacket.data) {
> + free(state->rpacket.data);
> + }
> + if(state->spacket.data) {
> + free(state->spacket.data);

You could just use Curl_safefree here.

>>> Dan

-- 
http://www.MoveAnnouncer.com              The web change of address service
          Let webmasters know that your web site has moved