Use of uninitialized value in TM4C NetworkInterface.c

[jonathangjertsen]

I got a warning when trying to port a project from TI CCS to the GNU ARM toolchain. Specifically on this line:

FreeRTOS-Plus-TCP/source/portable/NetworkInterface/TM4C/NetworkInterface.c

Line 485 in 5e55153

_rx_descriptors[ i ].ui32Count = DES1_RX_CTRL_CHAINED | ( ( buffer_size_requested << DES1_TX_CTRL_BUFF1_SIZE_S ) & DES1_TX_CTRL_BUFF1_SIZE_M );

GCC warns that 'buffer_size_requested' may be used uninitialized. Indeed, it is declared here and never initialized:

FreeRTOS-Plus-TCP/source/portable/NetworkInterface/TM4C/NetworkInterface.c

Line 450 in 5e55153

size_t buffer_size_requested;

Note that the port works great despite this apparent bug. But it seems like this should cause issues with the DMA. Tagging @jscott-hotstart in case you remember what buffer_size_requested should be.

[htibosch]

Hi @jonathangjertsen , Github doesn't let me see mentioned pages, it shows a big number 500. And even TM4C/NetworkInterface.c doesn't load, where README.md loads without a problem.
Of cause buffer_size_requested should be initialised. My guess is that it indicates the maximum size of the RX buffers, something like 1514 byte.
Here you see that the maximum RX size is 8191 bytes:

#define DES1_RX_CTRL_DISABLE_INT            0x80000000
#define DES1_RX_CTRL_BUFF2_SIZE_M           0x1FFF0000
#define DES1_RX_CTRL_BUFF2_SIZE_S           16
#define DES1_RX_CTRL_END_OF_RING            0x00008000
#define DES1_RX_CTRL_CHAINED                0x00004000
#define DES1_RX_CTRL_BUFF1_SIZE_M           0x00001FFF   /* 8191 bytes */
#define DES1_RX_CTRL_BUFF1_SIZE_S           0

Later on, the word is loaded again:

    /*Set up the DMA descriptor for the next receive transaction */
    dma_descriptor->ui32Count =
        DES1_RX_CTRL_CHAINED | ipTOTAL_ETHERNET_FRAME_SIZE;

It looks like the TM4C driver still needs some work.

[jonathangjertsen]

I guess ipTOTAL_ETHERNET_FRAME_SIZE (or BUFFER_SIZE, or BUFFER_SIZE_ROUNDED_UP 🤷 ) would work. I'll have a look at the reference manual for the chip and try for a fix next time I'm at the desk with a board

[htibosch]

These are the (local) definitions that you mention:

#define BUFFER_SIZE             ( ipTOTAL_ETHERNET_FRAME_SIZE + ipBUFFER_PADDING )
#define BUFFER_SIZE_ROUNDED_UP  ( ( BUFFER_SIZE + 7 ) & ~0x7UL )

where ipTOTAL_ETHERNET_FRAME_SIZE is the sum of:

    ipconfigNETWORK_MTU        // Normally 1500 bytes, defined by the user in FreeRTOS_IPConfig.h
    ipSIZE_OF_ETH_HEADER       // The 14-byte Ethernet header
    ipSIZE_OF_ETH_CRC_BYTES    // The hardware CRC is not included
    ipSIZE_OF_ETH_OPTIONAL_802_1Q_TAG_BYTES  // VLAN not yet supported

So I would propose to reserve DMA buffer of ipconfigNETWORK_MTU + ipSIZE_OF_ETH_HEADER bytes.

#define BUFFER_SIZE             ( ipconfigNETWORK_MTU + ipSIZE_OF_ETH_HEADER + ipBUFFER_PADDING )
#define BUFFER_SIZE_ROUNDED_UP  ( ( BUFFER_SIZE + 7 ) & ~0x7UL )

And define :

	const size_t buffer_size_requested = ipconfigNETWORK_MTU + ipSIZE_OF_ETH_HEADER;

And use it like this in the two locations:

        _rx_descriptors[ i ].ui32Count = DES1_RX_CTRL_CHAINED |
		    ( ( buffer_size_requested << DES1_RX_CTRL_BUFF1_SIZE_S ) & DES1_RX_CTRL_BUFF1_SIZE_M );
        dma_descriptor->ui32Count = DES1_RX_CTRL_CHAINED |
		    ( ( buffer_size_requested << DES1_RX_CTRL_BUFF1_SIZE_S ) & DES1_RX_CTRL_BUFF1_SIZE_M );

A network buffer is preceded by 10 hidden bytes, called ipBUFFER_PADDING. These are needed to store the address of the owning NetworkBufferDescriptor_t.
The rounding up is necessary to make sure that each buffer is well aligned.

/* This total buffer is 1528 bytes long:
0..9       Buffer padding
10..1523   DMA buffer
1524..1527 Round up space

[kstribrnAmzn]

@htibosch Thank you for taking a look at this. I completely agree with your justification. I'll see about getting this updated so that this default uninitialized variable doesn't bite anyone else using this port in the future.

[htibosch]

@kstribrnAmzn wrote:

Thank you for taking a look at this

No problem.

Here is a picture that may help to get a good understanding:

I like a BUFFER_SIZE_ROUNDED_UP of 1536 because that is super aligned, even when caching is active. When caching is not active, 1528 is the smallest size that can be used in case MTU = 1500.

[moninom1]

Thank you for raising change @archigup

@jonathangjertsen If you have some time can you please help in validating the change once, before we merge it. Thank you

[jonathangjertsen]

@moninom1 @archigup I've tested change in #1028. Everything still works in our application, which uses MQTT over TCP and UDP packets with up to 1024 bytes of payload. The diff looks OK as well.

[moninom1]

@jonathangjertsen Thank you for the quick update. Will we review it and merge it. Thanks again for reporting the issue.

[moninom1]

The fix got merged. Closing the issue. Thank you