TFTP file transfer issue in Curl 7.19.4

The patch for this issue is

/**********************************************************
*
* tftp_tx
*
* Event handler for the TX state
*
**********************************************************/
static CURLcode tftp_tx(tftp_state_data_t *state, tftp_event_t event)
{
struct SessionHandle *data = state->conn->data;
int sbytes;
int rblock;
CURLcode res = CURLE_OK;
struct SingleRequest *k = &data->req;

switch(event) {

case TFTP_EVENT_ACK:
/* Ack the packet */
rblock = getrpacketblock(&state->rpacket);

if(rblock != state->block) {
/* This isn't the expected block. Log it and up the retry counter */
infof(data, "Received ACK for block %d, expecting %d\n",
rblock, state->block);
state->retries++;
/* Bail out if over the maximum */
if(state->retries>state->retry_max) {
failf(data, "tftp_tx: giving up waiting for block %d ack",
state->block);
res = CURLE_SEND_ERROR;
}
else {
/* Re-send the data packet */
sbytes = sendto(state->sockfd, (void *)&state->spacket,
4+state->sbytes, SEND_4TH_ARG,
(struct sockaddr *)&state->remote_addr,
state->remote_addrlen);
/* Check all sbytes were sent */
if(sbytes<0) {
failf(data, "%s", Curl_strerror(state->conn, SOCKERRNO));
res = CURLE_SEND_ERROR;
}
}
return res;
}
/* This is the expected packet. Reset the counters and send the next
block */
state->block++;
state->retries = 0;
setpacketevent(&state->spacket, TFTP_EVENT_DATA);
setpacketblock(&state->spacket, state->block);
if(state->block > 1 && state->sbytes < state->blksize) {
state->state = TFTP_STATE_FIN;
return CURLE_OK;
}
res = Curl_fillreadbuffer(state->conn, state->blksize,
(int *)&state->sbytes);
if(res)
return res;
sbytes = sendto(state->sockfd, (void *)state->spacket.data,
4+state->sbytes, SEND_4TH_ARG,
(struct sockaddr *)&state->remote_addr,
state->remote_addrlen);
/* Check all sbytes were sent */
if(sbytes<0) {
failf(data, "%s", Curl_strerror(state->conn, SOCKERRNO));
return CURLE_SEND_ERROR;
}
/* Update the progress meter */
k->writebytecount += state->sbytes;
Curl_pgrsSetUploadCounter(data, k->writebytecount);
break;
case TFTP_EVENT_OACK:
/* This is the expected packet. Reset the counters and send the next
block */
state->block++;
state->retries = 0;
setpacketevent(&state->spacket, TFTP_EVENT_DATA);
setpacketblock(&state->spacket, state->block);
if(state->block > 1 && state->sbytes < state->blksize) {
state->state = TFTP_STATE_FIN;
return CURLE_OK;
}
res = Curl_fillreadbuffer(state->conn, state->blksize,
(int *)&state->sbytes);
if(res)
return res;
sbytes = sendto(state->sockfd, (void *)state->spacket.data,
4+state->sbytes, SEND_4TH_ARG,
(struct sockaddr *)&state->remote_addr,
state->remote_addrlen);
/* Check all sbytes were sent */
if(sbytes<0) {
failf(data, "%s", Curl_strerror(state->conn, SOCKERRNO));
return CURLE_SEND_ERROR;
}
/* Update the progress meter */
k->writebytecount += state->sbytes;
#ifndef CURL_DISABLE_PROGRESS
Curl_pgrsSetUploadCounter(data, k->writebytecount);
#endif

break;

case TFTP_EVENT_TIMEOUT:
/* Increment the retry counter and log the timeout */
state->retries++;
infof(data, "Timeout waiting for block %d ACK. "
" Retries = %d\n", state->retries);
/* Decide if we've had enough */
if(state->retries > state->retry_max) {
state->error = TFTP_ERR_TIMEOUT;
state->state = TFTP_STATE_FIN;
}
else {
/* Re-send the data packet */
sbytes = sendto(state->sockfd, (void *)state->spacket.data,
4+state->sbytes, SEND_4TH_ARG,
(struct sockaddr *)&state->remote_addr,
state->remote_addrlen);
/* Check all sbytes were sent */
if(sbytes<0) {
failf(data, "%s", Curl_strerror(state->conn, SOCKERRNO));
return CURLE_SEND_ERROR;
}
/* since this was a re-send, we remain at the still byte position */
Curl_pgrsSetUploadCounter(data, k->writebytecount);
}
break;

case TFTP_EVENT_ERROR:
state->state = TFTP_STATE_FIN;
break;

default:
failf(data, "%s", "tftp_tx: internal error");
break;
}

return res;
}

Ignore the line #ifndef CURL_DISABLE_PROGRESS.

I have added the case statement for TFTP_EVENT_OACK to fix the issue.

Please make the patch with 'diff -u' and post that here instead of a full code snippet.

This is the patch created using ""diff -u"

--- tftp_1.80.c 2009-03-25 09:34:33.007209600 +0530
+++ tftp.c 2009-04-01 15:05:48.895218000 +0530
@@ -723,6 +723,37 @@
Curl_pgrsSetUploadCounter(data, k->writebytecount);
break;

+case TFTP_EVENT_OACK:
+ /* This is the expected packet. Reset the counters and send the next
+ block */
+ state->block++;
+ state->retries = 0;
+ setpacketevent(&state->spacket, TFTP_EVENT_DATA);
+ setpacketblock(&state->spacket, state->block);
+ if(state->block > 1 && state->sbytes < state->blksize) {
+ state->state = TFTP_STATE_FIN;
+ return CURLE_OK;
+ }
+ res = Curl_fillreadbuffer(state->conn, state->blksize,
+ (int *)&state->sbytes);
+ if(res)
+ return res;
+ sbytes = sendto(state->sockfd, (void *)state->spacket.data,
+ 4+state->sbytes, SEND_4TH_ARG,
+ (struct sockaddr *)&state->remote_addr,
+ state->remote_addrlen);
+ /* Check all sbytes were sent */
+ if(sbytes<0) {
+ failf(data, "%s", Curl_strerror(state->conn, SOCKERRNO));
+ return CURLE_SEND_ERROR;
+ }
+ /* Update the progress meter */
+ k->writebytecount += state->sbytes;
+ Curl_pgrsSetUploadCounter(data, k->writebytecount);
+
+break;
+
+
case TFTP_EVENT_TIMEOUT:
/* Increment the retry counter and log the timeout */
state->retries++;

Daniel,

Can I know when these changes will be merged to the tftp.c file

Any chance you can write up a test case for the test suite that shows the problem and thus how this fix cures it?

I don't see any TFTP expert commenting on this patch and I personally know little about these details and I haven't had the time to read up so it would help a lot.

Without this fix I was not able to upload any files. That is the only test case I can think of.

Test case 285 and 286 already do TFTP uploads and they've worked for a long time. So no, "just uploading" is not a proper test case. It clearly requires something more special than so...

So there's no chance you can help us make the test suite test for this? I'm still reluctant to add this without understanding it further.

My doubt is that the tftp file upload/download has not been tested on a server which supports RFC2348.

On TFTP servers which do not support RFC2348, no issue will be seen since the server will not respond with an OACK message. But TFTP servers which support RFC 2348 will send an OACK to the client. If this case is not handled in tftp_tx(), file upload will fail. When the same OACK is handled in tftp_rx(), it should be handled in tftp_tx() as well.

RFC 2348 says "If the server is willing to accept the blocksize option, it sends an
Option Acknowledgment (OACK) to the client."

To me that's just more reasons to add support for this in the test suite so that we can be sure our stuff works. We have a test TFTP server there and a first step would thus be to make sure it supports RFC2348, I think. Do you think you could have a look at it? It doesn't even have to be a full-fledged implementation but it would be good with at least some kind of awareness so that we can use that to make sure curl behaves at least mostly right.

And I do believe someone has used such a server as the guy who implemented this feature probably didn't do it without using it at all.

You can try changing the TFTP blocksize from 512 to 2048 or something. If upload is working, then thats great.

I can try changing the size where?

I asked you about changing the tftp server to work with this.

my take at the oack change

Ok, seeing that your patch introduces a lot of code duplication. How about simply doing the change as I did in my just uploaded tftp-oack.patch ?

I think the changes in your patch should be enough.Thanks.

Thanks, this was committed to CVS now. It'd be great if you could verify that it works for you.

Also, if you tell me your real name I'll credit you properly in changelogs etc!