On Dec 30, 2008, at 3:44 PM, Dan Fandrich wrote:

> 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

Hi Dan,

You recommend possibly using strnlen() above, but as far as I know
this function isn't very portable (though I could be wrong). I also
don't see it used anywhere else in the libcurl code base. I would use
strlen, but would be concerned about a crash when a server returns an
option acknowledgment which has a field that is not properly NULL
terminated. This should really never happen in the real world, but you
never know what types of servers you'll run into and it would be nice
to protect against this. What would you recommend using in place of my
for() loop or strnlen()? Or is strnlen() actually OK to use within
libcurl? Thanks!

--
Chad Monroe