Fix DHCP race condition (FreeRTOS#1230)

* Initial

* DHCP full code coverage

* Fix formatting

Author: tony-josi-aws

source/FreeRTOS_DHCP.c

@@ -257,7 +257,7 @@
                     pxIterator = NULL;
                 }
 
-                if( pxIterator != NULL )
+                if( ( pxIterator != NULL ) && ( pxIterator->xDHCPData.eDHCPState == pxIterator->xDHCPData.eExpectedState ) )
                 {
                     /* The second parameter pdTRUE tells to check for a UDP message. */
                     vDHCPProcessEndPoint( pdFALSE, pdTRUE, pxIterator );
@@ -269,15 +269,24 @@
                 }
                 else
                 {
-                    /* Target not found, fetch the message and delete it. */
+                    /* Target not found or there is a state mismatch, fetch the message and delete it. */
                     /* PAss the address of a pointer pucUDPPayload, because zero-copy is used. */
                     lBytes = FreeRTOS_recvfrom( EP_DHCPData.xDHCPSocket, &( pucUDPPayload ), 0, FREERTOS_ZERO_COPY, NULL, NULL );
 
                     if( ( lBytes > 0 ) && ( pucUDPPayload != NULL ) )
                     {
                         /* Remove it now, destination not found. */
                         FreeRTOS_ReleaseUDPPayloadBuffer( pucUDPPayload );
-                        FreeRTOS_printf( ( "vDHCPProcess: Removed a %d-byte message: target not found\n", ( int ) lBytes ) );
+
+                        if( pxIterator == NULL )
+                        {
+                            FreeRTOS_printf( ( "vDHCPProcess: Removed a %d-byte message: target not found\n", ( int ) lBytes ) );
+                        }
+                        else
+                        {
+                            FreeRTOS_printf( ( "vDHCPProcess: Wrong state: expected: %d got: %d : ignore\n",
+                                               pxIterator->xDHCPData.eExpectedState, pxIterator->xDHCPData.eDHCPState ) );
+                        }
                     }
                 }
             }
@@ -489,6 +498,11 @@
                 {
                     /* Give up, start again. */
                     EP_DHCPData.eDHCPState = eInitialWait;
+
+                    /* Reset expected state so that DHCP packets from
+                     * different DHCP servers if available already in the DHCP socket can
+                     * be processed */
+                    EP_DHCPData.eExpectedState = eInitialWait;
                 }
             }
         }
@@ -992,6 +1006,11 @@
                         {
                             /* Start again. */
                             EP_DHCPData.eDHCPState = eInitialWait;
+
+                            /* Reset expected state so that DHCP packets from
+                             * different DHCP servers if available already in the DHCP socket can
+                             * be processed */
+                            EP_DHCPData.eExpectedState = eInitialWait;
                         }
                     }
 

test/unit-test/FreeRTOS_DHCP/FreeRTOS_DHCP_stubs.c

@@ -715,4 +715,38 @@ static int32_t FreeRTOS_recvfrom_eWaitingOfferRecvfromSuccess_LocalMACAddrNotMat
 
     return xSizeofUDPBuffer;
 }
+
+static int32_t FreeRTOS_recvfrom_LoopedCall( const ConstSocket_t xSocket,
+                                             void * pvBuffer,
+                                             size_t uxBufferLength,
+                                             BaseType_t xFlags,
+                                             struct freertos_sockaddr * pxSourceAddress,
+                                             socklen_t * pxSourceAddressLength,
+                                             int callbacks )
+{
+    NetworkEndPoint_t * pxIterator = pxNetworkEndPoints;
+    size_t xSizeRetBufferSize = xSizeofUDPBuffer;
+
+    if( callbacks == 2 )
+    {
+        pxNetworkEndPoints->xDHCPData.eDHCPState = eInitialWait;
+    }
+    else if( callbacks == 4 )
+    {
+        xSizeRetBufferSize = 200;
+    }
+
+    if( ( xFlags & FREERTOS_ZERO_COPY ) != 0 )
+    {
+        *( ( uint8_t ** ) pvBuffer ) = pucUDPBuffer;
+    }
+
+    memset( pucUDPBuffer, 0, xSizeofUDPBuffer );
+    /* Put in correct DHCP cookie. */
+    ( ( struct xDHCPMessage_IPv4 * ) pucUDPBuffer )->ulDHCPCookie = dhcpCOOKIE;
+    ( ( struct xDHCPMessage_IPv4 * ) pucUDPBuffer )->ucOpcode = dhcpREPLY_OPCODE;
+    ( ( struct xDHCPMessage_IPv4 * ) pucUDPBuffer )->ulTransactionID = FreeRTOS_htonl( 0x01ABCDEF );
+
+    return xSizeRetBufferSize;
+}
 /*-----------------------------------------------------------*/

test/unit-test/FreeRTOS_DHCP/FreeRTOS_DHCP_utest.c

@@ -1235,6 +1235,49 @@ void test_vDHCPProcess_eLeasedAddress_CorrectState_ValidBytesInMessage( void )
     TEST_ASSERT_EQUAL( eLeasedAddress, pxEndPoint->xDHCPData.eDHCPState );
 }
 
+/**
+ *@brief  This test function ensures that when the DHCP states are mismatching after
+ * initial parsing of response from DHCP server, if a new response from a different DHCP
+ * server will cause a infinite loop inside the vDHCPProcess.
+ */
+void test_vDHCPProcess_eLeasedAddress_InCorrectState_Loop( void )
+{
+    struct xSOCKET xTestSocket;
+    NetworkEndPoint_t xEndPoint = { 0 }, * pxEndPoint = &xEndPoint;
+    uint8_t * pucUDPPayload;
+
+    /* This should remain unchanged. */
+    xDHCPv4Socket = &xTestSocket;
+    xDHCPSocketUserCount = 1;
+    pxEndPoint->xDHCPData.xDHCPSocket = &xTestSocket;
+    /* Put the required state. */
+    pxEndPoint->xDHCPData.eDHCPState = eLeasedAddress;
+    pxEndPoint->xDHCPData.eExpectedState = eLeasedAddress;
+    pxEndPoint->xDHCPData.ulTransactionId = 0x01ABCDEF;
+
+    /* Make sure that the local IP address is uninitialised. */
+    pxEndPoint->ipv4_settings.ulIPAddress = 0;
+    /* Put a verifiable value. */
+    memset( &pxEndPoint->ipv4_settings, 0xAA, sizeof( IPV4Parameters_t ) );
+    /* Put a verifiable value. */
+    memset( &pxEndPoint->ipv4_defaults, 0xBB, sizeof( IPV4Parameters_t ) );
+
+    pxNetworkEndPoints = pxEndPoint;
+
+    /* Expect these arguments. */
+    FreeRTOS_recvfrom_Stub( FreeRTOS_recvfrom_LoopedCall );
+
+    FreeRTOS_ReleaseUDPPayloadBuffer_Expect( pucUDPBuffer );
+
+    FreeRTOS_IsEndPointUp_IgnoreAndReturn( pdFALSE );
+
+    FreeRTOS_ReleaseUDPPayloadBuffer_Ignore();
+
+    vDHCPProcess( pdFALSE, pxEndPoint );
+
+    TEST_ASSERT_EQUAL( eInitialWait, pxEndPoint->xDHCPData.eDHCPState );
+}
+
 void test_vDHCPProcess_eLeasedAddress_CorrectState_ValidBytesInMessage_TransactionIDMismatch( void )
 {
     struct xSOCKET xTestSocket;