From 6307e783d83835ffaa70fc594b2a08307b2e359b Mon Sep 17 00:00:00 2001 From: Daniel Stenberg Date: Mon, 8 May 2006 15:09:50 +0000 Subject: [PATCH] Fixed known bug #28. The TFTP code no longer assumes a packed struct and thus works reliably on more platforms. --- CHANGES | 4 ++ RELEASE-NOTES | 1 + docs/KNOWN_BUGS | 2 - lib/tftp.c | 111 ++++++++++++++++++++++++++------------------------------ 4 files changed, 57 insertions(+), 61 deletions(-) diff --git a/CHANGES b/CHANGES index c4f0500..03fd1ba 100644 --- a/CHANGES +++ b/CHANGES @@ -6,6 +6,10 @@ Changelog +Daniel (8 May 2006) +- Fixed known bug #28. The TFTP code no longer assumes a packed struct and + thus works reliably on more platforms. + Daniel (5 May 2006) - Roland Blom filed bug report #1481217 (http://curl.haxx.se/bug/view.cgi?id=1481217), with follow-ups by Michele diff --git a/RELEASE-NOTES b/RELEASE-NOTES index f4f2230..208e5b1 100644 --- a/RELEASE-NOTES +++ b/RELEASE-NOTES @@ -20,6 +20,7 @@ This release includes the following changes: This release includes the following bugfixes: + o TFTP works in a more portable fashion (== on more platforms) o WSAGetLastError() is now used (better) on Windows o GnuTLS non-block case that could cause data trashing o deflate code survives lack of zlib header diff --git a/docs/KNOWN_BUGS b/docs/KNOWN_BUGS index 9149b99..ba0bfa4 100644 --- a/docs/KNOWN_BUGS +++ b/docs/KNOWN_BUGS @@ -33,8 +33,6 @@ may have been fixed since this was written! See http://curl.haxx.se/bug/view.cgi?id=1371118 -28. The TFTP code is not portable and will fail on some architectures. - 26. NTLM authentication using SSPI (on Windows) when (lib)curl is running in "system context" will make it use wrong(?) user name - at least when compared to what winhttp does. See http://curl.haxx.se/bug/view.cgi?id=1281867 diff --git a/lib/tftp.c b/lib/tftp.c index d8a23b0..780ccee 100644 --- a/lib/tftp.c +++ b/lib/tftp.c @@ -122,23 +122,7 @@ typedef enum { } tftp_error_t; typedef struct tftp_packet { - unsigned short event; - union { - struct { - unsigned char data[512]; - } request; - struct { - unsigned short block; - unsigned char data[512]; - } data; - struct { - unsigned short block; - } ack; - struct { - unsigned short code; - unsigned char data[512]; - } error; - } u; + unsigned char data[516]; } tftp_packet_t; typedef struct tftp_state_data { @@ -199,7 +183,8 @@ void tftp_set_timeouts(tftp_state_data_t *state) /* Compute the re-start interval to suit the timeout */ state->retry_time = timeout/state->retry_max; - if(state->retry_time<1) state->retry_time=1; + if(state->retry_time<1) + state->retry_time=1; } else { @@ -215,12 +200,16 @@ void tftp_set_timeouts(tftp_state_data_t *state) state->retry_max = timeout/15; } /* But bound the total number */ - if(state->retry_max<3) state->retry_max=3; - if(state->retry_max>50) state->retry_max=50; + if(state->retry_max<3) + state->retry_max=3; + + if(state->retry_max>50) + state->retry_max=50; /* Compute the re-ACK interval to suit the timeout */ state->retry_time = timeout/state->retry_max; - if(state->retry_time<1) state->retry_time=1; + if(state->retry_time<1) + state->retry_time=1; infof(data, "set timeouts for state %d; Total %d, retry %d maxtry %d\n", state->state, (state->max_time-state->start_time), @@ -235,6 +224,29 @@ void tftp_set_timeouts(tftp_state_data_t *state) * **********************************************************/ +static void setpacketevent(tftp_packet_t *packet, unsigned short num) +{ + packet->data[0] = (num >> 8); + packet->data[1] = (num & 0xff); +} + + +static void setpacketblock(tftp_packet_t *packet, unsigned short num) +{ + packet->data[2] = (num >> 8); + packet->data[3] = (num & 0xff); +} + +static unsigned short getrpacketevent(tftp_packet_t *packet) +{ + return (packet->data[0] << 8) | packet->data[1]; +} + +static unsigned short getrpacketblock(tftp_packet_t *packet) +{ + return (packet->data[2] << 8) | packet->data[3]; +} + static void tftp_send_first(tftp_state_data_t *state, tftp_event_t event) { int sbytes; @@ -260,18 +272,18 @@ static void tftp_send_first(tftp_state_data_t *state, tftp_event_t event) if(data->set.upload) { /* If we are uploading, send an WRQ */ - state->spacket.event = htons(TFTP_EVENT_WRQ); + setpacketevent(&state->spacket, TFTP_EVENT_WRQ); filename = curl_easy_unescape(data, filename, 0, NULL); - state->conn->upload_fromhere = (char *)state->spacket.u.data.data; + state->conn->upload_fromhere = (char *)&state->spacket.data[2]; if(data->set.infilesize != -1) Curl_pgrsSetUploadSize(data, data->set.infilesize); } else { /* If we are downloading, send an RRQ */ - state->spacket.event = htons(TFTP_EVENT_RRQ); + setpacketevent(&state->spacket, TFTP_EVENT_RRQ); } - snprintf((char *)state->spacket.u.request.data, - sizeof(state->spacket.u.request.data), + snprintf((char *)&state->spacket.data[2], + 512, "%s%c%s%c", filename, '\0', mode, '\0'); sbytes = 4 + (int)strlen(filename) + (int)strlen(mode); sbytes = sendto(state->sockfd, (void *)&state->spacket, @@ -325,7 +337,7 @@ static void tftp_rx(tftp_state_data_t *state, tftp_event_t event) case TFTP_EVENT_DATA: /* Is this the block we expect? */ - rblock = ntohs(state->rpacket.u.data.block); + rblock = getrpacketblock(&state->rpacket); if ((state->block+1) != rblock) { /* No, log it, up the retry count and fail if over the limit */ infof(data, @@ -340,9 +352,9 @@ static void tftp_rx(tftp_state_data_t *state, tftp_event_t event) /* This is the expected block. Reset counters and ACK it. */ state->block = rblock; state->retries = 0; - state->spacket.event = htons(TFTP_EVENT_ACK); - state->spacket.u.ack.block = htons(state->block); - sbytes = sendto(state->sockfd, (void *)&state->spacket, + 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); @@ -409,7 +421,7 @@ static void tftp_tx(tftp_state_data_t *state, tftp_event_t event) case TFTP_EVENT_ACK: /* Ack the packet */ - rblock = ntohs(state->rpacket.u.data.block); + rblock = getrpacketblock(&state->rpacket); if(rblock != state->block) { /* This isn't the expected block. Log it and up the retry counter */ @@ -428,14 +440,14 @@ static void tftp_tx(tftp_state_data_t *state, tftp_event_t event) block */ state->block++; state->retries = 0; - state->spacket.event = htons(TFTP_EVENT_DATA); - state->spacket.u.ack.block = htons(state->block); + setpacketevent(&state->spacket, TFTP_EVENT_DATA); + setpacketblock(&state->spacket, state->block); if(state->block > 1 && state->sbytes < 512) { state->state = TFTP_STATE_FIN; return; } Curl_fillreadbuffer(state->conn, 512, &state->sbytes); - 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); @@ -529,28 +541,9 @@ CURLcode Curl_tftp_connect(struct connectdata *conn, bool *done) tftp_state_data_t *state; int rc; - /* - * The TFTP code is not portable because it sends C structs directly over - * the wire. Since C gives compiler writers a wide latitude in padding and - * aligning structs, this fails on many architectures (e.g. ARM). - * - * The only portable way to fix this is to copy each struct item into a - * flat buffer and send the flat buffer instead of the struct. The - * alternative, trying to get the compiler to eliminate padding bytes - * within the struct, is a nightmare to maintain (each compiler does it - * differently), and is still not guaranteed to work because some - * architectures can't handle the resulting alignment. - * - * This check can be removed once the code has been fixed. - */ - if(sizeof(struct tftp_packet) != 516) { - failf(conn->data, "tftp not supported on this architecture"); - return CURLE_FAILED_INIT; - } - - if((state = conn->proto.tftp = calloc(sizeof(tftp_state_data_t), 1))==NULL) { + state = conn->proto.tftp = calloc(sizeof(tftp_state_data_t), 1); + if(!state) return CURLE_OUT_OF_MEMORY; - } state->conn = conn; state->sockfd = state->conn->sock[FIRSTSOCKET]; @@ -678,17 +671,17 @@ CURLcode Curl_tftp(struct connectdata *conn, bool *done) } else { /* The event is given by the TFTP packet time */ - event = (tftp_event_t)ntohs(state->rpacket.event); + event = (tftp_event_t)getrpacketevent(&state->rpacket); switch(event) { case TFTP_EVENT_DATA: if (state->rbytes > 4) Curl_client_write(data, CLIENTWRITE_BODY, - (char *)state->rpacket.u.data.data, state->rbytes-4); + (char *)&state->rpacket.data[4], state->rbytes-4); break; case TFTP_EVENT_ERROR: - state->error = (tftp_error_t)ntohs(state->rpacket.u.error.code); - infof(conn->data, "%s\n", (char *)state->rpacket.u.error.data); + state->error = (tftp_error_t)getrpacketblock(&state->rpacket); + infof(conn->data, "%s\n", (char *)&state->rpacket.data[4]); break; case TFTP_EVENT_ACK: break; -- 2.7.4