Risk of buffer overflow in nxd_bsd.c

[garin1000]

Discovered as warning when compiling with arm-none-eabi-gcc 13.3Rel1, compile options -Wall -Wextra -Werror:
netxduo/addons/BSD/nxd_bsd.c:5601:18: error: writing 1 byte into a region of size 0 [-Werror=stringop-overflow=]

(in my project, an older version of nxd is used, the bug is still present in the current version)

Reading the code confirms the compiler is right:

In nxd_bsd.c line 5893 (in function bsd_number_convert()), the assignment

string[size] = (CHAR) NX_NULL;

assigns 0 to the byte after the end of CHAR *string, if the generated string exactly fills the buffer.

Rationale:

The while loop (line 5861) exits after size has been incremented to buffer_len by the increment in line 5885. Line 5893 then is equivalent to string[buffer_len]=0, which is a write access to the byte after the string buffer.

[yuxinzhou5]

@garin1000, this is correct if the caller doesn't supply a buffer sufficiently big for the trailing NULL.

[garin1000]

Well, if supplying a pointer to a buffer and a buffer length to a function, one would expect that you have to give length and not length-1 as parameter. In my opinion that definitely violates the principle of least surprise.

[hwmaier]

I would agree that this has potential for a buffer overflow and implementation should check for that, rather writing NX_NULL potentially into arbitrary memory.

A suggested implementation would replace

    /* Make the string NULL terminated.  */
    string[size] =  (CHAR) NX_NULL;

    /* Determine if there is an overflow error.  */
    if (number)
    {

        /* Error, return bad values to user.  */
        size =  0;
        string[0] = '0';
    }

with

    /* Determine if there is an overflow error.  */
    if (number || (size >= buffer_len))
    {

        /* Error, return bad values to user.  */
        size =  0;
        string[0] = '0';
    }

    /* Make the string NULL terminated.  */
    string[size] =  (CHAR) NX_NULL;

[garin1000]

I agree, I would have made the same change.