• State Resolved
  • Replies 2 replies
  • Subscribers 30 subscribers
  • Views 77 views
  • Users 0 members are here
Support feedback
Options
Options
Related

Original question:

AM2434: AM2434: INDUSTRIAL-COMMUNICATIONS-SDK-AM243X v11.x, EthertNet/IP, bug in drv_uart.c

AM2434: AM2434: INDUSTRIAL-COMMUNICATIONS-SDK-AM243X v11.00.00.xx, EthertNet/IP, bug in drv_uart.c example module

Vit Triska
Prodigy 60 points
Part Number: AM2434


Hello,

I see another bug in function
"void DRV_UART_printf(void* pContext, const char* pFormat, va_list arg);"
present in
"c:\ti\ind_comms_sdk_am243x_11_00_00_13\examples\industrial_comms\ethernetip_adapter_demo\drivers\uart\drv_uart.c" 

This function SHOULD place data from the temporary buffer "tmpString[]" into the circular buffer "aOutStream" atomically, i.e., all or nothing.

In the case of a single write (memcpy), this condition is met. In the case of two writes, it actually happens that only the first part is executed and the rest could not fit, which is a bug.

As a fix, I propose the following solution for the condition of writing to the circular buffer. Please forward this code snippet to your development team.

I request identical behavior of this function, which will behave the same way when the buffer is circular and almost full - place all data or none (anytime)!

Regards,
Vit Triska

ethernetip_example_drv_uart_fixed_DRV_UART_printf.c 

  • 0
    TI__Intellectual 1810 points

    Hello Vit,

    Thank you again for your valuable feedback. The refactoring of the UART driver is included in our roadmap; however, in the upcoming release, we will only address the critical bug related to the usage of the static array inside the DRV_UART_printf function. All other bugs (potential buffer overflow, mutex handling issues, etc.) will be addressed in future releases.

    Regarding your proposed code, while it is indeed a better implementation, there is still one potential bug related to the return value of vsnprintf(). The return value of this function represents the number of characters that would have been written to the buffer if it were large enough (excluding the null terminator). If the return value is greater than or equal to the buffer size, it indicates that the buffer was too small to hold the entire formatted string, which could then result in a buffer overflow during the subsequent memcpy() operations.

    Regards,
    Pourya

  • +1 in reply to Pourya Eskandari
    Prodigy 60 points
    Pourya Eskandari said:
    Regarding your proposed code, while it is indeed a better implementation, there is still one potential bug related to the return value of vsnprintf(). The return value of this function represents the number of characters that would have been written to the buffer if it were large enough (excluding the null terminator). If the return value is greater than or equal to the buffer size, it indicates that the buffer was too small to hold the entire formatted string, which could then result in a buffer overflow during the subsequent memcpy() operations.


    Yes, you are talking about:

        char tmpString[76];  // TODO fix me to 256 (after TI EtherNet/IP library stack overflow fix)
        bytesToWrite = vsnprintf(tmpString, sizeof(tmpString), pFormat, arg);
        if (bytesToWrite > 0)
        { ... }


    Great capture!!!

    The right condition is:
       if (bytesToWrite > 0 && bytesToWrite < sizeof(tmpString))
       { ... }

    I assumed that you had a non-standard implementation of the vsnprintf() method, so I left this matter alone…
    I didn't expect that there were 3 errors in one function in fact :D


    Thank you!