Menu

#818 TFTP error scenario not closing connection in curl 7.19.4

closed-fixed
TFTP (14)
5
2013-06-21
2009-03-31
Vijay G
No

In tftp.c in the curl lib 7.19.4, whenever there is any error in TFTP, it is better to close the
connection. In ftp.c, its being done in the same way. "conn->bits.close =
TRUE; " should be called in tftp_do() in if(state->error != TFTP_ERR_NONE)
to resolve this issue.

Discussion

  • Vijay G

    Vijay G - 2009-03-31

    The below patch will fix this problem :

    /**********************************************************
    *
    * tftp
    *
    * The do callback
    *
    * This callback handles the entire TFTP transfer
    *
    **********************************************************/

    static CURLcode tftp_do(struct connectdata *conn, bool *done)
    {
    struct SessionHandle *data = conn->data;
    tftp_state_data_t *state;
    tftp_event_t event;
    CURLcode code;
    int rc;
    struct Curl_sockaddr_storage fromaddr;
    socklen_t fromlen;
    int check_time = 0;
    struct SingleRequest *k = &data->req;

    *done = TRUE;

    /*
    Since connections can be re-used between SessionHandles, this might be a
    connection already existing but on a fresh SessionHandle struct so we must
    make sure we have a good 'struct TFTP' to play with. For new connections,
    the struct TFTP is allocated and setup in the tftp_connect() function.
    */
    Curl_reset_reqproto(conn);

    if(!conn->proto.tftpc) {
    code = tftp_connect(conn, done);
    if(code)
    return code;
    }
    state = (tftp_state_data_t *)conn->proto.tftpc;

    /* Run the TFTP State Machine */
    for(code=tftp_state_machine(state, TFTP_EVENT_INIT);
    (state->state != TFTP_STATE_FIN) && (code == CURLE_OK);
    code=tftp_state_machine(state, event) ) {

    /* Wait until ready to read or timeout occurs */
    rc=Curl_socket_ready(state->sockfd, CURL_SOCKET_BAD,
    state->retry_time * 1000);

    if(rc == -1) {
    /* bail out */
    int error = SOCKERRNO;
    failf(data, "%s", Curl_strerror(conn, error));
    event = TFTP_EVENT_ERROR;
    }
    else if(rc==0) {
    /* A timeout occured */
    event = TFTP_EVENT_TIMEOUT;

    /* Force a look at transfer timeouts */
    check_time = 0;

    }
    else {

    /* Receive the packet */
    fromlen = sizeof(fromaddr);
    state->rbytes = (ssize_t)recvfrom(state->sockfd,
    (void *)state->rpacket.data,
    state->blksize+4,
    0,
    (struct sockaddr *)&fromaddr,
    &fromlen);
    if(state->remote_addrlen==0) {
    memcpy(&state->remote_addr, &fromaddr, fromlen);
    state->remote_addrlen = fromlen;
    }

    /* Sanity check packet length */
    if(state->rbytes < 4) {
    failf(data, "Received too short packet");
    /* Not a timeout, but how best to handle it? */
    event = TFTP_EVENT_TIMEOUT;
    }
    else {

    /* The event is given by the TFTP packet time */
    event = (tftp_event_t)getrpacketevent(&state->rpacket);

    switch(event) {
    case TFTP_EVENT_DATA:
    /* Don't pass to the client empty or retransmitted packets */
    if(state->rbytes > 4 &&
    ((state->block+1) == getrpacketblock(&state->rpacket))) {
    code = Curl_client_write(conn, CLIENTWRITE_BODY,
    (char *)state->rpacket.data+4,
    state->rbytes-4);
    if(code)
    return code;
    k->bytecount += state->rbytes-4;
    Curl_pgrsSetDownloadCounter(data, (curl_off_t) k->bytecount);
    }
    break;
    case TFTP_EVENT_ERROR:
    state->error = (tftp_error_t)getrpacketblock(&state->rpacket);
    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:
    failf(data, "%s", "Internal error: Unexpected packet");
    break;
    }

    /* Update the progress meter */
    if(Curl_pgrsUpdate(conn))
    return CURLE_ABORTED_BY_CALLBACK;
    }
    }

    /* Check for transfer timeout every 10 blocks, or after timeout */
    if(check_time%10==0) {
    time_t current;
    time(&current);
    if(current>state->max_time) {
    DEBUGF(infof(data, "timeout: %d > %d\n",
    current, state->max_time));
    state->error = TFTP_ERR_TIMEOUT;
    state->state = TFTP_STATE_FIN;
    }
    }

    }
    if(code)
    return code;

    /* Tell curl we're done */
    code = Curl_setup_transfer(conn, -1, -1, FALSE, NULL, -1, NULL);
    if(code)
    return code;

    /* If we have encountered an error */
    if(state->error != TFTP_ERR_NONE) {
    conn->bits.close = TRUE; /*marked for closure */
    /* Translate internal error codes to curl error codes */
    switch(state->error) {
    case TFTP_ERR_NOTFOUND:
    code = CURLE_TFTP_NOTFOUND;
    break;
    case TFTP_ERR_PERM:
    code = CURLE_TFTP_PERM;
    break;
    case TFTP_ERR_DISKFULL:
    code = CURLE_REMOTE_DISK_FULL;
    break;
    case TFTP_ERR_UNDEF:
    case TFTP_ERR_ILLEGAL:
    code = CURLE_TFTP_ILLEGAL;
    break;
    case TFTP_ERR_UNKNOWNID:
    code = CURLE_TFTP_UNKNOWNID;
    break;
    case TFTP_ERR_EXISTS:
    code = CURLE_REMOTE_FILE_EXISTS;
    break;
    case TFTP_ERR_NOSUCHUSER:
    code = CURLE_TFTP_NOSUCHUSER;
    break;
    case TFTP_ERR_TIMEOUT:
    code = CURLE_OPERATION_TIMEDOUT;
    break;
    case TFTP_ERR_NORESPONSE:
    code = CURLE_COULDNT_CONNECT;
    break;
    default:
    code= CURLE_ABORTED_BY_CALLBACK;
    break;
    }
    }
    else
    code = CURLE_OK;
    return code;
    }

     
  • Vijay G

    Vijay G - 2009-04-01

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

    --- tftp_1.80.c 2009-04-01 15:08:58.939660400 +0530
    +++ tftp.c 2009-04-01 15:09:42.954722000 +0530
    @@ -1109,6 +1109,7 @@

    /* If we have encountered an error */
    if(state->error != TFTP_ERR_NONE) {
    + conn->bits.close = TRUE; /*marked for closure */

    /* Translate internal error codes to curl error codes */
    switch(state->error) {

     
  • Vijay G

    Vijay G - 2009-04-01
     
  • Vijay G

    Vijay G - 2009-04-10

    Daniel,

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

     
  • Daniel Stenberg

    Daniel Stenberg - 2009-04-13

    You can help getting this added faster by providing test cases that repeat the problem(s) and thus showing how the proposed patch(es) work.

    For this close case, shouldn't we rather only mark the connection for closure for the specific problems that can be considered to be that "fatal" ?

    I mean, TFTP_ERR_NOTFOUND is just a bad file name. Not a really good reason to force a closure of the connection methinks. TFTP_ERR_NORESPONSE however sounds like a more serious problem that probably deserves a complete close.

     
  • Vijay G

    Vijay G - 2009-04-17

    In my application, if a file is not present in the server, I was not able to receive any other file also in the same connection. Thats the reason why I was closing the connection.

     
  • Daniel Stenberg

    Daniel Stenberg - 2009-04-17

    Wow. That description makes it sound like this suggested fix is rather painting over the real problem instead of addressing it. It should of course not matter if the connection was re-used or not.

    Can you adjust an existing test cast to make it repeat your problem?

     
  • Daniel Stenberg

    Daniel Stenberg - 2009-04-29

    Thanks for the report. I instead decided to simply not re-use connections at all for TFTP and thus this problem doesn't occur anymore. Test case 1099 verifies this problem. Fix committed just now. Case closed.

     
  • Daniel Stenberg

    Daniel Stenberg - 2009-04-29
    • status: open --> closed-fixed