curl-library
Re: TFTP Option Negotiation
Date: Tue, 30 Dec 2008 13:20:46 -0800
On Dec 27, 2008, at 2:29 PM, Daniel Stenberg wrote:
> On Sat, 27 Dec 2008, Chad Monroe wrote:
>
>>> it would be great if you also could take a look and see if you can
>>> extend/make the test suite's TFTP-server to support this and
>>> excerise some of this new logic in a test case or two.
>>
>> I have the libcurl code done according to what we've talked about
>> in this thread, but am struggling a bit adding some test cases. I
>> ran through all of the existing test cases today and they run fine
>> with the new code in place, but it doesn't test the option
>> negotiation at all.
>
> Well, running the existing tests (and your own use cases) fine are
> of course a very good start and a precondition for the code to get
> accepted.
>
>> I looked at and started modifying the test TFTP server code a bit
>> but am not happy with the results yet. Would you like me to add
>> full option negotiation support to the server side as well so we
>> can add test cases which use varying block sizes, or is there some
>> other way you'd prefer to see this done?
>
> I haven't read up on the details on how this TFTP negotiation is
> being done, but in general the test servers don't necessarily have
> to be that clever, to actually support "the real thing". They merely
> need to have a way to send custom things and record what it gets
> back, and then we can make up test cases that make the server send
> the right things and make sure the client (curl) deals with it fine,
> but we can also make sure the server sends the _wrong_ thing and
> verify the client in that aspect as well.
>
>> Adding option negotiation support to the test server is probably as
>> much or more work than adding it to libcurl, so I want to make sure
>> this is something necessary before finishing the work.
>
> It's certainly not _necessary_, only a good thing to make sure the
> functionality is there, is solid and will remain so when things
> change ahead.
>
>> Also, since I have the library code mostly complete, I could submit
>> that patch in the near future and continue to work on updating the
>> test server later, or I could wait until it's all done and submit
>> one big patch, though it will probably take a bit longer. Whatever
>> approach is easiest for you works for me.
>
> I'm glad to hear the patch is near arrival!
>
> I'd say the patch should be separated into two separate ones to make
> them easier to digest and review! Please send over the libcurl patch
> once you have something you want reviewed/applied and we'll have a
> look. With the TFTP negotiation code in place, the chances of
> someone else helping out with getting it tested by the test suite
> also increases!
>
> --
>
> / daniel.haxx.se
Hi Daniel,
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:
* Block size can be specified via CURLOPT_TFTP_BLKSIZE, min 8 max
65464 as per RFC 2348
* If no block size is specified, the default of 512 is used
* Memory is allocated in tftp_connect() based on the requested block
size, if the server rejects our requested block size in the option ACK
we will fall back to a block size of 512 bytes, but there will still
be memory allocated for whatever block size the user defined.
* tftp_disconnect() was added to free packet buffers
I've ran all WRQ and RRQ test cases which exist today and they pass.
This shows that current support against servers which do not support
option negotiation works as expected. I have tested option negotiation
in my client against servers which support option negotiation and it
works as expected as well. I have tested block sizes ranging from
512-1428 so far and transfers succeed.
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?
I'm not fully aware of all the ways libcurl can be used, but in all of
my testing (multi and easy) things look great so far. If you have any
suggestions for improvements it would be much appreciated. Also, I've
never posted a patch to this list, but think I'm following all the
guidelines in the docs. If not, please let me know as well. Thanks!
--
Chad Monroe
--- curl-7.19.2.orig/docs/libcurl/curl_easy_setopt.3 2008-11-05
13:48:00.000000000 -0800
+++ curl-7.19.2/docs/libcurl/curl_easy_setopt.3 2008-12-25
19:10:11.000000000 -0800
@@ -18,7 +18,7 @@
.\" * This software is distributed on an "AS IS" basis, WITHOUT
WARRANTY OF ANY
.\" * KIND, either express or implied.
.\" *
-.\" * $Id: curl_easy_setopt.3,v 1.238 2008-11-05 21:48:00 bagder Exp $
+.\" * $Id: curl_easy_setopt.3,v 1.2 2008/12/26 03:10:11 cmonroe Exp $
.\"
**************************************************************************
.\"
.TH curl_easy_setopt 3 "28 Oct 2008" "libcurl 7.19.1" "libcurl Manual"
@@ -989,6 +989,11 @@
transfer decoding will be disabled, if set to 1 it is enabled
(default). libcurl does chunked transfer decoding by default unless
this
option is set to zero. (added in 7.16.2)
+.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.
.SH FTP OPTIONS
.IP CURLOPT_FTPPORT
Pass a pointer to a zero terminated string as parameter. It will be
used to
--- curl-7.19.2.orig/include/curl/curl.h 2008-10-16 22:58:26.000000000
-0700
+++ curl-7.19.2/include/curl/curl.h 2008-12-25 19:10:11.000000000 -0800
@@ -20,7 +20,7 @@
* This software is distributed on an "AS IS" basis, WITHOUT
WARRANTY OF ANY
* KIND, either express or implied.
*
- * $Id: curl.h,v 1.370 2008-10-17 03:59:02 yangtse Exp $
+ * $Id: curl.h,v 1.2 2008/12/26 03:10:11 cmonroe Exp $
***************************************************************************/
/*
@@ -1149,6 +1149,9 @@
CINIT(PROXYUSERNAME, OBJECTPOINT, 175),
CINIT(PROXYPASSWORD, OBJECTPOINT, 176),
+ /* block size for TFTP transfers */
+ CINIT(TFTP_BLKSIZE, LONG, 177),
+
CURLOPT_LASTENTRY /* the last unused */
} CURLoption;
--- curl-7.19.2.orig/lib/urldata.h 2008-11-11 11:19:56.000000000 -0800
+++ curl-7.19.2/lib/urldata.h 2008-12-25 19:10:11.000000000 -0800
@@ -20,7 +20,7 @@
* This software is distributed on an "AS IS" basis, WITHOUT
WARRANTY OF ANY
* KIND, either express or implied.
*
- * $Id: urldata.h,v 1.395 2008-11-03 16:24:56 bagder Exp $
+ * $Id: urldata.h,v 1.2 2008/12/26 03:10:11 cmonroe Exp $
***************************************************************************/
/* This file is for lib internal stuff */
@@ -1404,6 +1404,7 @@
long timeout; /* in milliseconds, 0 means no timeout */
long connecttimeout; /* in milliseconds, 0 means no timeout */
long ftp_response_timeout; /* in milliseconds, 0 means no timeout */
+ long tftp_blksize ; /* in bytes, 0 means use default */
curl_off_t infilesize; /* size of file to upload, -1 means
unknown */
long low_speed_limit; /* bytes/second */
long low_speed_time; /* number of seconds */
--- curl-7.19.2.orig/lib/url.c 2008-11-11 05:20:02.000000000 -0800
+++ curl-7.19.2/lib/url.c 2008-12-25 19:10:11.000000000 -0800
@@ -18,7 +18,7 @@
* This software is distributed on an "AS IS" basis, WITHOUT
WARRANTY OF ANY
* KIND, either express or implied.
*
- * $Id: url.c,v 1.771 2008-11-05 21:46:40 bagder Exp $
+ * $Id: url.c,v 1.2 2008/12/26 03:10:11 cmonroe Exp $
***************************************************************************/
/* -- WIN32 approved -- */
@@ -903,6 +903,12 @@
*/
data->set.ftp_response_timeout = va_arg( param , long ) * 1000;
break;
+ case CURLOPT_TFTP_BLKSIZE:
+ /*
+ * TFTP option that specifies the block size to use for data
transmission
+ */
+ data->set.tftp_blksize = va_arg(param, long);
+ break;
case CURLOPT_DIRLISTONLY:
/*
* An option that changes the command to one that asks for a list
--- curl-7.19.2.orig/lib/tftp.c 2008-09-29 14:45:38.000000000 -0700
+++ curl-7.19.2/lib/tftp.c 2008-12-30 11:20:14.000000000 -0800
@@ -18,7 +18,7 @@
* This software is distributed on an "AS IS" basis, WITHOUT
WARRANTY OF ANY
* KIND, either express or implied.
*
- * $Id: tftp.c,v 1.73 2008-09-29 21:44:50 danf Exp $
+ * $Id: tftp.c,v 1.6 2008/12/30 17:58:51 cmonroe Exp $
***************************************************************************/
#include "setup.h"
@@ -80,8 +80,13 @@
/* The last #include file should be: */
#include "memdebug.h"
-/* RFC2348 allows the block size to be negotiated, but we don't
support that */
-#define TFTP_BLOCKSIZE 512
+/* RFC2348 allows the block size to be negotiated */
+#define TFTP_BLKSIZE_DEFAULT 512
+#define TFTP_BLKSIZE_MIN 8
+#define TFTP_BLKSIZE_MAX 65464
+#define TFTP_OPTION_BLKSIZE "blksize"
+#define TFTP_OPTION_TSIZE "tsize"
+#define TFTP_OPTION_INTERVAL "interval"
typedef enum {
TFTP_MODE_NETASCII=0,
@@ -102,6 +107,7 @@
TFTP_EVENT_DATA = 3,
TFTP_EVENT_ACK = 4,
TFTP_EVENT_ERROR = 5,
+ TFTP_EVENT_OACK = 6,
TFTP_EVENT_TIMEOUT
} tftp_event_t;
@@ -122,7 +128,7 @@
} tftp_error_t;
typedef struct tftp_packet {
- unsigned char data[2 + 2 + TFTP_BLOCKSIZE];
+ unsigned char *data;
} tftp_packet_t;
typedef struct tftp_state_data {
@@ -142,6 +148,8 @@
socklen_t remote_addrlen;
ssize_t rbytes;
int sbytes;
+ int blksize;
+ int requested_blksize;
tftp_packet_t rpacket;
tftp_packet_t spacket;
} tftp_state_data_t;
@@ -151,6 +159,7 @@
static CURLcode tftp_rx(tftp_state_data_t *state, tftp_event_t
event) ;
static CURLcode tftp_tx(tftp_state_data_t *state, tftp_event_t
event) ;
static CURLcode tftp_connect(struct connectdata *conn, bool *done);
+static CURLcode tftp_disconnect(struct connectdata *conn);
static CURLcode tftp_do(struct connectdata *conn, bool *done);
static CURLcode tftp_done(struct connectdata *conn,
CURLcode, bool premature);
@@ -172,7 +181,7 @@
ZERO_NULL, /* doing */
ZERO_NULL, /* proto_getsock */
ZERO_NULL, /* doing_getsock */
- ZERO_NULL, /* disconnect */
+ tftp_disconnect, /* disconnect */
PORT_TFTP, /* defport */
PROT_TFTP /* protocol */
};
@@ -290,11 +299,103 @@
return (unsigned short)((packet->data[2] << 8) | packet->data[3]);
}
+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++);
+
+ 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);
+ if(tmp == NULL) {
+ failf(data, "%s\n", "Malformed ACK packet, rejecting");
+ return CURLE_TFTP_ILLEGAL;
+ }
+
+ if(strncasecmp(option, TFTP_OPTION_BLKSIZE,
strlen(TFTP_OPTION_BLKSIZE)) == 0) {
+ int blksize;
+
+ blksize = strtol( value, NULL, 10 );
+
+ if(!blksize) {
+ failf(data, "%s\n", "invalid blocksize value in OACK packet");
+ return CURLE_TFTP_ILLEGAL;
+ }
+ else if(blksize > TFTP_BLKSIZE_MAX) {
+ failf(data, "%s (%d)\n", "blksize is larger than max
supported",
+ TFTP_BLKSIZE_MAX);
+ return CURLE_TFTP_ILLEGAL;
+ }
+ else if(blksize < TFTP_BLKSIZE_MIN) {
+ failf(data, "%s (%d)\n", "blksize is smaller than min
supported",
+ TFTP_BLKSIZE_MIN);
+ return CURLE_TFTP_ILLEGAL;
+ }
+ else if (blksize > state->requested_blksize) {
+ /* could realloc pkt buffers here, but the spec doesn't call
out support
+ * for the server requesting a bigger blksize than the client
requests */
+ failf(data, "%s (%d)",
+ "server requested blksize larger than allocated", blksize);
+ return CURLE_TFTP_ILLEGAL;
+ }
+
+ state->blksize = blksize;
+ infof(data, "%s (%d) %s (%d)\n", "blksize parsed from OACK",
+ state->blksize, "requested", state->requested_blksize);
+ }
+ else if(strncasecmp(option, TFTP_OPTION_TSIZE,
strlen(TFTP_OPTION_TSIZE)) == 0) {
+ int tsize;
+
+ tsize = strtol( value, NULL, 10 );
+ if(!tsize) {
+ failf(data, "%s\n", "invalid tsize value in OACK packet");
+ return CURLE_TFTP_ILLEGAL;
+ }
+ Curl_pgrsSetDownloadSize(data, tsize);
+ infof(data, "%s (%d)\n", "tsize parsed from OACK", tsize);
+ }
+ }
+
+ return CURLE_OK;
+}
+
+static int tftp_option_add(tftp_state_data_t *state, int csize, char
*buf, char *option)
+{
+ if( ( strlen(option) + csize + 1 ) > state->blksize )
+ return 0;
+ strcpy(buf, option);
+ return( strlen(option) + 1 );
+}
+
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);
+ sbytes += tftp_option_add(state, sbytes,
+ (char *)state->spacket.data+sbytes, "0");
+ /* add blksize option */
+ snprintf( buf, sizeof(buf), "%d", state->requested_blksize );
+ sbytes += tftp_option_add(state, sbytes,
+ (char *)state->spacket.data+sbytes, TFTP_OPTION_BLKSIZE);
+ sbytes += tftp_option_add(state, sbytes,
+ (char *)state->spacket.data+sbytes, buf );
+ /* add timeout option */
+ snprintf( buf, sizeof(buf), "%d", state->retry_time );
+ sbytes += tftp_option_add(state, sbytes,
+ (char *)state->spacket.data+sbytes, TFTP_OPTION_INTERVAL);
+ sbytes += tftp_option_add(state, sbytes,
+ (char *)state->spacket.data+sbytes, buf );
+
+ sbytes = sendto(state->sockfd, (void *)state->spacket.data,
sbytes, 0,
state->conn->ip_addr->ai_addr,
state->conn->ip_addr->ai_addrlen);
@@ -355,6 +475,7 @@
break;
return tftp_tx(state, event);
+ case TFTP_EVENT_OACK:
case TFTP_EVENT_DATA: /* connected for receive */
infof(data, "%s\n", "Connected for receive");
state->state = TFTP_STATE_RX;
@@ -390,7 +511,6 @@
switch(event) {
case TFTP_EVENT_DATA:
-
/* Is this the block we expect? */
rblock = getrpacketblock(&state->rpacket);
if((state->block+1) != rblock) {
@@ -419,7 +539,7 @@
}
/* Check if completed (That is, a less than full packet is
received) */
- if(state->rbytes < (ssize_t)sizeof(state->spacket)){
+ if(state->rbytes < (ssize_t)state->blksize+4){
state->state = TFTP_STATE_FIN;
}
else {
@@ -427,6 +547,25 @@
}
break;
+ case TFTP_EVENT_OACK:
+ /* ACK option acknowledgement so we can move on to data */
+ state->block = 0;
+ state->retries = 0;
+ setpacketevent(&state->spacket, TFTP_EVENT_ACK);
+ setpacketblock(&state->spacket, state->block);
+ sbytes = sendto(state->sockfd, (void *)state->spacket.data,
+ 4, SEND_4TH_ARG,
+ (struct sockaddr *)&state->remote_addr,
+ state->remote_addrlen);
+ if(sbytes < 0) {
+ failf(data, "%s", Curl_strerror(state->conn, SOCKERRNO));
+ return CURLE_SEND_ERROR;
+ }
+
+ /* we're ready to RX data */
+ state->state = TFTP_STATE_RX;
+ break;
+
case TFTP_EVENT_TIMEOUT:
/* Increment the retry count and fail if over the limit */
state->retries++;
@@ -438,7 +577,7 @@
}
else {
/* Resend the previous ACK */
- sbytes = sendto(state->sockfd, (void *)&state->spacket,
+ sbytes = sendto(state->sockfd, (void *)state->spacket.data,
4, SEND_4TH_ARG,
(struct sockaddr *)&state->remote_addr,
state->remote_addrlen);
@@ -514,11 +653,11 @@
state->retries = 0;
setpacketevent(&state->spacket, TFTP_EVENT_DATA);
setpacketblock(&state->spacket, state->block);
- if(state->block > 1 && state->sbytes < TFTP_BLOCKSIZE) {
+ if(state->block > 1 && state->sbytes < state->blksize) {
state->state = TFTP_STATE_FIN;
return CURLE_OK;
}
- res = Curl_fillreadbuffer(state->conn, TFTP_BLOCKSIZE, &state-
>sbytes);
+ res = Curl_fillreadbuffer(state->conn, state->blksize, &state-
>sbytes);
if(res)
return res;
sbytes = sendto(state->sockfd, (void *)state->spacket.data,
@@ -547,7 +686,7 @@
}
else {
/* Re-send the data packet */
- sbytes = sendto(state->sockfd, (void *)&state->spacket,
+ sbytes = sendto(state->sockfd, (void *)state->spacket.data,
4+state->sbytes, SEND_4TH_ARG,
(struct sockaddr *)&state->remote_addr,
state->remote_addrlen);
@@ -610,6 +749,29 @@
return res;
}
+/**********************************************************
+ *
+ * tftp_disconnect
+ *
+ * The disconnect callback
+ *
+ **********************************************************/
+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);
+ }
+ }
+
+ return CURLE_OK;
+}
/**********************************************************
*
@@ -622,7 +784,9 @@
{
CURLcode code;
tftp_state_data_t *state;
- int rc;
+ int blksize, rc;
+
+ blksize = TFTP_BLKSIZE_DEFAULT;
/* If there already is a protocol-specific struct allocated for this
sessionhandle, deal with it */
@@ -636,12 +800,36 @@
return CURLE_OUT_OF_MEMORY;
}
+ /* alloc pkt buffers based on specified blksize */
+ if(conn->data->set.tftp_blksize)
+ {
+ blksize = conn->data->set.tftp_blksize;
+ if(blksize > TFTP_BLKSIZE_MAX || blksize < TFTP_BLKSIZE_MIN )
+ return CURLE_TFTP_ILLEGAL;
+ }
+
+ if(!state->rpacket.data) {
+ state->rpacket.data = calloc(1, blksize + 2 + 2);
+
+ if(!state->rpacket.data)
+ return CURLE_OUT_OF_MEMORY;
+ }
+
+ if(!state->spacket.data) {
+ state->spacket.data = calloc(1, blksize + 2 + 2);
+
+ if(!state->spacket.data)
+ return CURLE_OUT_OF_MEMORY;
+ }
+
conn->bits.close = FALSE; /* keep it open if possible */
state->conn = conn;
state->sockfd = state->conn->sock[FIRSTSOCKET];
state->state = TFTP_STATE_START;
state->error = TFTP_ERR_NONE;
+ state->blksize = TFTP_BLKSIZE_DEFAULT;
+ state->requested_blksize = blksize;
((struct sockaddr *)&state->local_addr)->sa_family =
(unsigned short)(conn->ip_addr->ai_family);
@@ -765,8 +953,8 @@
/* Receive the packet */
fromlen = sizeof(fromaddr);
state->rbytes = (ssize_t)recvfrom(state->sockfd,
- (void *)&state->rpacket,
- sizeof(state->rpacket),
+ (void *)state->rpacket.data,
+ state->blksize+4,
0,
(struct sockaddr *)&fromaddr,
&fromlen);
@@ -792,7 +980,7 @@
if(state->rbytes > 4 &&
((state->block+1) == getrpacketblock(&state-
>rpacket))) {
code = Curl_client_write(conn, CLIENTWRITE_BODY,
- (char *)&state->rpacket.data[4],
+ (char *)state->rpacket.data+4,
state->rbytes-4);
if(code)
return code;
@@ -802,10 +990,15 @@
break;
case TFTP_EVENT_ERROR:
state->error = (tftp_error_t)getrpacketblock(&state-
>rpacket);
- infof(data, "%s\n", (const char *)&state->rpacket.data[4]);
+ infof(data, "%s\n", (const char *)state->rpacket.data+4);
break;
case TFTP_EVENT_ACK:
break;
+ case TFTP_EVENT_OACK:
+ code = tftp_parse_option_ack(state, (const char *)state-
>rpacket.data+2, state->rbytes-2);
+ if(code)
+ return code;
+ break;
case TFTP_EVENT_RRQ:
case TFTP_EVENT_WRQ:
default:
Received on 2008-12-30