weston-embedded/uC-TCP-IP

NetICMP_TxEchoReq() Returns Wrong Error Code

Closed this issue · 3 comments

SeanAlling commented

System Configuration

The following is my configuration.

  1. uc/USB: CDC-ACM
  2. uc/USB: CDC-EEM
  3. uc/Shell: Shell
  4. uc/Shell: Terminal
  5. uc/TCPIP

The terminal is accessed through a serial port which is hosted over the CDC-ACM port. The shell at startup registered all net commands.

Bug Overview

NetICMP_TxEchoReq() function can return the wrong error code. In the following code, if kal_err != KAL_ERR_NONE then an error has occurred. For any error after setting p_err the software will issue a jump to release.

    switch (kal_err) {
        case KAL_ERR_NONE:
             result = DEF_OK;
            *p_err  = NET_ICMP_ERR_NONE;
             goto release;

        case KAL_ERR_TIMEOUT:
             result = DEF_FAIL;
            *p_err  = NET_ICMP_ERR_ECHO_REQ_TIMEOUT;
             goto release;

        case KAL_ERR_ABORT:
        case KAL_ERR_ISR:
        case KAL_ERR_WOULD_BLOCK:
        case KAL_ERR_OS:
        default:
             result = DEF_FAIL;
            *p_err  = NET_ICMP_ERR_ECHO_REQ_SIGNAL_FAULT;
             goto release;
    }
release:
    NetICMP_LockAcquire(p_err);

    KAL_SemDel(sem_handle, &kal_err);

    if (*p_err == NET_ICMP_ERR_NONE) {
        if (data_len > 0) {
            if (p_echo_req_handle_new->DataCmp != DEF_OK) {
                *p_err = NET_ICMP_ERR_ECHO_REPLY_DATA_CMP_FAIL;
            }
        }
    }

Once execution resumes at release, the first function, NetICMP_LockAcquire(p_err), will overwrite p_err resulting in any previous error code being lost.

Reproduction

  1. Start system. System not connected to internet.
  2. Connect to system over serial port (using USB CDC-ACM).
  3. Issue a net_ping command using IP address that is not on local network.
  4. Shell prints error error code 12033 (NET_ICMP_ERR_ECHO_REPLY_DATA_CMP_FAIL).

Shell should return NET_ICMP_ERR_ECHO_REQ_TIMEOUT.

Debugging shows that p_err does get set correctly to NET_ICMP_ERR_ECHO_REQ_TIMEOUT, but is overwritten once code following release starts executing.

Resolution

If the switch statement is changes so that errors use goto exit_kal_fault instead of goto release, the system will not call Mem_DynPoolBlkFree() which results in a memory leak. Since this code uses goto, the following code section

exit_kal_fault:
    Mem_DynPoolBlkFree(&NetICMP_DataPtr->EchoReqPool,
                        p_echo_req_handle_new,
                       &lib_err);


exit_release_lock:
    NetICMP_LockRelease();


exit_return:
    return (result);

could be replaced with

exit_kal_fault:
    Mem_DynPoolBlkFree(&NetICMP_DataPtr->EchoReqPool,
                        p_echo_req_handle_new,
                       &lib_err);

exit_release_lock:
    NetICMP_LockRelease();
    goto exit_return;

exit_kal_fault_with_cleanup:
    Mem_DynPoolBlkFree(&NetICMP_DataPtr->EchoReqPool,
                        p_echo_req_handle_new,
                       &lib_err);

exit_return:
    return (result);

where exit_kal_fault_with_cleanup will be used in the switch case instead of goto release for the fault conditions.

As I can't find a coding standard I do not know what the appropriate solution would be.

yasosa305 commented

Hi SeanAlling,
Thank you for bringing this up. I think a better solution would be to create a separate local variable of type NET_ERR that is passed to the NetICMP_LockAcquire() function calls and use p_err separately.

SeanAlling commented

In the case where KAL_SemPend() has failed, should ``release:even be executed? My assumption was that in the case whereKAL_SemPend()```, there is a known error so the code should immediately release the allocated memory and return.

If I understand your intent, then even when KAL_SemPend() fails, then the NetICMP_LockAcquire() should still be run.

An alternative to what I proposed could be the following

if(*p_err == KAL_ERR_NONE) {
    NetICMP_LockAcquire(p_err);

    KAL_SemDel(sem_handle, &kal_err);

    if (*p_err == NET_ICMP_ERR_NONE) {
        if (data_len > 0) {
            if (p_echo_req_handle_new->DataCmp != DEF_OK) {
                *p_err = NET_ICMP_ERR_ECHO_REPLY_DATA_CMP_FAIL;
            }
        }
    }

    if ((NetICMP_DataPtr->EchoReqHandleListStartPtr == p_echo_req_handle_new) &&
        (NetICMP_DataPtr->EchoReqHandleListEndPtr   == p_echo_req_handle_new)) {

        NetICMP_DataPtr->EchoReqHandleListStartPtr = DEF_NULL;
        NetICMP_DataPtr->EchoReqHandleListEndPtr   = DEF_NULL;

    } else if (NetICMP_DataPtr->EchoReqHandleListStartPtr == p_echo_req_handle_new) {
        NetICMP_DataPtr->EchoReqHandleListStartPtr = p_echo_req_handle_new->NextPtr;

    } else if (NetICMP_DataPtr->EchoReqHandleListEndPtr == p_echo_req_handle_new) {
        NetICMP_DataPtr->EchoReqHandleListEndPtr = p_echo_req_handle_new->PrevPtr;

    } else {
        p_echo_req_handle_end          = p_echo_req_handle_new->PrevPtr;
        p_echo_req_handle_end->NextPtr = p_echo_req_handle_new->NextPtr;
    }

    p_echo_req_handle_new->NextPtr = DEF_NULL;
    p_echo_req_handle_new->PrevPtr = DEF_NULL;
}

exit_kal_fault:
    Mem_DynPoolBlkFree(&NetICMP_DataPtr->EchoReqPool,
                        p_echo_req_handle_new,
                       &lib_err);


exit_release_lock:
    if(*p_err == KAL_ERR_NONE) {
        NetICMP_LockRelease();
    }

The above would make the entire release branch and the exit_release_lock branch of code not execute when KAL_SemPend() returned with an error.

The if around NetICMP_LockRelease() but I like to keep the number of lock and release calls equal, and with out the f condition on the exit_release_lock then an additional LockRelease would execute.

This alternative proposed method also wont need an additional NET_ERR variable created and also does not need any extra code written for making sure the correct error code is set in p_err.

SeanAlling commented

After some more testing of this today, I realized my last remark had a memory leak. KAL_SemDel() would not be called when an error occurred which would prevent the created semaphore from being deleted.

The fix is a small change to the if statement.

release:
    if(p_err == NET_ICMP_ERR_NONE) {

        NetICMP_LockAcquire(p_err);

        KAL_SemDel(sem_handle, &kal_err);

        if (*p_err == NET_ICMP_ERR_NONE) {
            if (data_len > 0) {
                if (p_echo_req_handle_new->DataCmp != DEF_OK) {
                    *p_err = NET_ICMP_ERR_ECHO_REPLY_DATA_CMP_FAIL;
                }
            }
        }

        if ((NetICMP_DataPtr->EchoReqHandleListStartPtr == p_echo_req_handle_new) &&
            (NetICMP_DataPtr->EchoReqHandleListEndPtr   == p_echo_req_handle_new)) {

            NetICMP_DataPtr->EchoReqHandleListStartPtr = DEF_NULL;
            NetICMP_DataPtr->EchoReqHandleListEndPtr   = DEF_NULL;

        } else if (NetICMP_DataPtr->EchoReqHandleListStartPtr == p_echo_req_handle_new) {
            NetICMP_DataPtr->EchoReqHandleListStartPtr = p_echo_req_handle_new->NextPtr;

        } else if (NetICMP_DataPtr->EchoReqHandleListEndPtr == p_echo_req_handle_new) {
            NetICMP_DataPtr->EchoReqHandleListEndPtr = p_echo_req_handle_new->PrevPtr;

        } else {
            p_echo_req_handle_end          = p_echo_req_handle_new->PrevPtr;
            p_echo_req_handle_end->NextPtr = p_echo_req_handle_new->NextPtr;
        }

        p_echo_req_handle_new->NextPtr = DEF_NULL;
        p_echo_req_handle_new->PrevPtr = DEF_NULL;
    }
    else {
        KAL_SemDel(sem_handle, &kal_err); 
    }

Same as my prior recommendation, the block of code between the release tag and the exit_kal_fault will now not execute if a fault is detected. The code will then execute the else block, deleting the semaphore, and then continue at exit_kal_fault tag. If no error is detected, the code will run the same as before.