[BUG] prvTCPReturnPacket_IPV4() dereferences pxNetworkBuffer outside its null pointer check

[anordal]

This applies to the latest release (V4.1.0) as well as origin/HEAD as of writing.

Clang-tidy (16 and 18) complains about a couple of possible null pointer dereferences in this repository, and I think it is right about one of them. They are of the form of a null pointer check (hinting that it can be NULL) and a dereference outside the check that dereferences it anyway.

function prvTCPReturnPacket_IPV4(), variable pxNetworkBuffer:

source/FreeRTOS_TCP_Transmission_IPv4.c:147:44: warning: Access to field 'pucEthernetBuffer' results in a dereference of a null pointer (loaded from variable 'pxNetworkBuffer'

This is the one I think clang-tidy is right about. Here, the unguarded dereference is accompanied by null pointer checks both before and after:

#if ( ipconfigZERO_COPY_TX_DRIVER != 0 )
{
    if( xDoRelease == pdFALSE )
    {
        /* A zero-copy network driver wants to pass the packet buffer
         * to DMA, so a new buffer must be created. */
        pxNetworkBuffer = pxDuplicateNetworkBufferWithDescriptor( pxNetworkBuffer, ( size_t ) pxNetworkBuffer->xDataLength );

        if( pxNetworkBuffer != NULL )
        {
            xDoRelease = pdTRUE;
        }
        else
        {
            FreeRTOS_debug_printf( ( "prvTCPReturnPacket: duplicate failed\n" ) );
        }
    }
}
#endif /* ipconfigZERO_COPY_TX_DRIVER */

// …

pxIPHeader = ( ( IPHeader_t * ) &( pxNetworkBuffer->pucEthernetBuffer[ ipSIZE_OF_ETH_HEADER ] ) );

#ifndef __COVERITY__
    if( pxNetworkBuffer != NULL ) /* LCOV_EXCL_BR_LINE the 2nd branch will never be reached */
#endif

Interestingly, the section above this would have made sure it isn't a null pointer if it wasn't for assigning it again here. If the assignment here (in the ipconfigZERO_COPY_TX_DRIVER section) is to uphold the same guarrantee, it is missing some error handling.

function prvSendDHCPRequest(), variable EP_DHCPData.xDHCPSocket:

This must be a false positive, but I'll include it.
Clang is looking across 3 funciton calls to build a case for how this pointer can be null at the place of dereference,
but ignores the possibility that xSocketValid() can contain the crucial null pointer check,
which it does.

In function vDHCPProcess:

source/FreeRTOS_DHCP.c:199:15: note: Assuming field 'eDHCPState' is equal to field 'eExpectedState'
source/FreeRTOS_DHCP.c:206:18: note: Assuming field 'xDHCPSocket' is equal to NULL
source/FreeRTOS_DHCP.c:290:13: note: 'xDoProcess' is not equal to pdFALSE
source/FreeRTOS_DHCP.c:293:13: note: Calling 'vDHCPProcessEndPoint'

In function vDHCPProcessEndPoint:

source/FreeRTOS_DHCP.c:679:13: note: 'xReset' is equal to pdFALSE
source/FreeRTOS_DHCP.c:684:27: note: Field 'eDHCPState' is equal to field 'eExpectedState'
source/FreeRTOS_DHCP.c:703:13: note: Control jumps to 'case eSendDHCPRequest:'  at line 720
source/FreeRTOS_DHCP.c:722:25: note: Calling 'prvSendDHCPRequest'

In function prvSendDHCPRequest:

source/FreeRTOS_DHCP.c:1542:49: note: Access to field 'pxEndPoint' results in a dereference of a null pointer (loaded from field 'xDHCPSocket')

[xuelix]

Thanks for reporting. We'll have the team go through the code flow you mentioned above and see if any action needs to be taken

[tony-josi-aws]

@anordal

Thanks for sharing this detailed report.

Your assessment regarding prvTCPReturnPacket_IPV4 seems to be correct when ipconfigZERO_COPY_TX_DRIVER is enabled. The line pxIPHeader = ( ( IPHeader_t * ) &( pxNetworkBuffer->pucEthernetBuffer[ ipSIZE_OF_ETH_HEADER ] ) ); should be performed inside the if( pxNetworkBuffer != NULL ) check.

Regarding prvSendDHCPRequest, vDHCPProcess is called in 2 places:

• prvProcessNetworkDownEvent: NULL pointer check is performed on pxEndPoint before the call to vDHCPProcess here.
• prvCallDHCP_RA_Handler: This function is called when there is a eDHCPEvent received in the IP task. DHCP event is sent from the following places:
  • vCheckNetworkTimers: NULL pointer check.
  • xProcessReceivedUDPPacket_IPv4 \ xProcessReceivedUDPPacket_IPv6: Before processing any received ethernet packet both interface and endpoint of the packet is checked to be not NULL here.

Hence I believe the 2nd case is indeed a false positive as you suggested.

[anordal]

Thanks!

prvTCPReturnPacket_IPV4: Yes, I don't know why I didn't see that solution – pxIPHeader is indeed only used in that scope – its scope can be reduced. It looks tempting to move most of the variable declarations into that scope. The single NetworkInterface_t * pxInterface; declaration there looks so lonely ;-)

prvProcessNetworkDownEvent: Hm, maybe you see something I don't, but note that the supposed null pointer is xDHCPSocket – in the left side of the assignment. Here it is inside the xSocketValid check:

if( ( xSocketValid( EP_DHCPData.xDHCPSocket ) == pdTRUE ) && /* … */ )
{
    // …

    EP_DHCPData.xDHCPSocket->pxEndPoint = pxEndPoint;

My thinking was that it is enough to observe that xSocketValid() only returns pdTRUE if its argument is non-null.

Clang-tidy can't see into xSocketValid() (because it's defined in a different translation unit, and clang-tidy is essentially a compiler). If I define a prvSocketValid() in the same file that does the same, and replace the call to xSocketValid with a call to it, clang-tidy is indeed happy. It is (arguably) a compiler bug that it allows itself to make an invalid assumption and raise a false accusation when it's unable to prove something.

[tony-josi-aws]

Will you be interested in creating a PR to fix this issue, or would you prefer that we create one?

[anordal]

I would be delighted! I'm writing a commit. It looks like the corresponding *_IPv6 function has the same problem. I'll check if it does and then include that too.

I can also move the variables into the scope. In a separate commit.

[xuelix]

With merged PR, I am closing this issue.
Thanks for the PR.