Fix freed memory being reused (#1148)

* Add changes * Fix build * TCP API utests * Fix UTs * Fix state handling APIs * Fix CBMC proofs * Fix formatting * Updating with review feedback
FreeRTOS · Jun 4, 2024 · ba6ba81 · ba6ba81
1 parent bfd8514
commit ba6ba81
Show file tree

Hide file tree

Showing 11 changed files with 408 additions and 29 deletions.
diff --git a/source/FreeRTOS_Sockets.c b/source/FreeRTOS_Sockets.c
@@ -3891,6 +3891,12 @@ void vSocketWakeUpUser( FreeRTOS_Socket_t * pxSocket )
             if( pxParentSocket->u.xTCP.bits.bReuseSocket == pdFALSE_UNSIGNED )
             {
                 pxClientSocket = pxParentSocket->u.xTCP.pxPeerSocket;
+
+                if( pxClientSocket != NULL )
+                {
+                    FreeRTOS_printf( ( "prvAcceptWaitClient: client %p parent %p\n",
+                                       ( void * ) pxClientSocket, ( void * ) pxParentSocket ) );
+                }
             }
             else
             {
@@ -3899,11 +3905,14 @@ void vSocketWakeUpUser( FreeRTOS_Socket_t * pxSocket )
 
             if( pxClientSocket != NULL )
             {
-                pxParentSocket->u.xTCP.pxPeerSocket = NULL;
-
                 /* Is it still not taken ? */
                 if( pxClientSocket->u.xTCP.bits.bPassAccept != pdFALSE_UNSIGNED )
                 {
+                    if( pxParentSocket->u.xTCP.pxPeerSocket != NULL )
+                    {
+                        pxParentSocket->u.xTCP.pxPeerSocket = NULL;
+                    }
+
                     pxClientSocket->u.xTCP.bits.bPassAccept = pdFALSE_UNSIGNED;
                 }
                 else

diff --git a/source/FreeRTOS_TCP_IP.c b/source/FreeRTOS_TCP_IP.c
@@ -274,6 +274,36 @@
     }
     /*-----------------------------------------------------------*/
 
+    static BaseType_t vTCPRemoveTCPChild( const FreeRTOS_Socket_t * pxChildSocket )
+    {
+        BaseType_t xReturn = pdFALSE;
+        const ListItem_t * pxEnd = ( ( const ListItem_t * ) &( xBoundTCPSocketsList.xListEnd ) );
+
+        /* MISRA Ref 11.3.1 [Misaligned access] */
+        /* More details at: https://github.com/FreeRTOS/FreeRTOS-Plus-TCP/blob/main/MISRA.md#rule-113 */
+        /* coverity[misra_c_2012_rule_11_3_violation] */
+        const ListItem_t * pxIterator = ( const ListItem_t * ) listGET_HEAD_ENTRY( &xBoundTCPSocketsList );
+
+        while( pxIterator != pxEnd )
+        {
+            FreeRTOS_Socket_t * pxSocket;
+            pxSocket = ( ( FreeRTOS_Socket_t * ) listGET_LIST_ITEM_OWNER( pxIterator ) );
+            pxIterator = ( ListItem_t * ) listGET_NEXT( pxIterator );
+
+            if( ( pxSocket != pxChildSocket ) && ( pxSocket->usLocalPort == pxChildSocket->usLocalPort ) )
+            {
+                if( pxSocket->u.xTCP.pxPeerSocket == pxChildSocket ) /**< for server socket: child, for child socket: parent */
+                {
+                    pxSocket->u.xTCP.pxPeerSocket = NULL;
+                    xReturn = pdTRUE;
+                    break;
+                }
+            }
+        }
+
+        return xReturn;
+    }
+
 /**
  * @brief Changing to a new state. Centralised here to do specific actions such as
  *        resetting the alive timer, calling the user's OnConnect handler to notify
@@ -440,34 +470,53 @@
         if( ( eTCPState == eCLOSED ) ||
             ( eTCPState == eCLOSE_WAIT ) )
         {
+            BaseType_t xMustClear = pdFALSE;
+            BaseType_t xHasCleared = pdFALSE;
+
+            if( ( xParent == pxSocket ) && ( pxSocket->u.xTCP.pxPeerSocket != NULL ) )
+            {
+                xParent = pxSocket->u.xTCP.pxPeerSocket;
+            }
+
+            if( ( xParent->u.xTCP.pxPeerSocket != NULL ) &&
+                ( xParent->u.xTCP.pxPeerSocket == pxSocket ) )
+            {
+                xMustClear = pdTRUE;
+                ( void ) xMustClear;
+            }
+
             /* Socket goes to status eCLOSED because of a RST.
              * When nobody owns the socket yet, delete it. */
+            FreeRTOS_printf( ( "vTCPStateChange: Closing (Queued %d, Accept %d Reuse %d)\n",
+                               pxSocket->u.xTCP.bits.bPassQueued,
+                               pxSocket->u.xTCP.bits.bPassAccept,
+                               pxSocket->u.xTCP.bits.bReuseSocket ) );
+            FreeRTOS_printf( ( "vTCPStateChange: me %p parent %p peer %p clear %d\n",
+                               ( void * ) pxSocket,
+                               ( void * ) xParent,
+                               xParent ? ( void * ) xParent->u.xTCP.pxPeerSocket : NULL,
+                               ( int ) xMustClear ) );
+
             vTaskSuspendAll();
             {
                 if( ( pxSocket->u.xTCP.bits.bPassQueued != pdFALSE_UNSIGNED ) ||
                     ( pxSocket->u.xTCP.bits.bPassAccept != pdFALSE_UNSIGNED ) )
                 {
                     if( pxSocket->u.xTCP.bits.bReuseSocket == pdFALSE_UNSIGNED )
                     {
+                        xHasCleared = vTCPRemoveTCPChild( pxSocket );
+                        ( void ) xHasCleared;
+
                         pxSocket->u.xTCP.bits.bPassQueued = pdFALSE_UNSIGNED;
                         pxSocket->u.xTCP.bits.bPassAccept = pdFALSE_UNSIGNED;
-                    }
-
-                    ( void ) xTaskResumeAll();
-
-                    FreeRTOS_printf( ( "vTCPStateChange: Closing socket\n" ) );
-
-                    if( pxSocket->u.xTCP.bits.bReuseSocket == pdFALSE_UNSIGNED )
-                    {
                         configASSERT( xIsCallingFromIPTask() != pdFALSE );
                         vSocketCloseNextTime( pxSocket );
                     }
                 }
-                else
-                {
-                    ( void ) xTaskResumeAll();
-                }
             }
+            ( void ) xTaskResumeAll();
+            FreeRTOS_printf( ( "vTCPStateChange: xHasCleared = %d\n",
+                               ( int ) xHasCleared ) );
         }
 
         if( ( eTCPState == eCLOSE_WAIT ) && ( pxSocket->u.xTCP.bits.bReuseSocket == pdTRUE_UNSIGNED ) )

diff --git a/source/FreeRTOS_TCP_State_Handling.c b/source/FreeRTOS_TCP_State_Handling.c
@@ -1021,11 +1021,19 @@
 
         pxSocket->u.xTCP.usChildCount++;
 
-        FreeRTOS_debug_printf( ( "Gain: Socket %u now has %u / %u child%s\n",
+        if( pxSocket->u.xTCP.pxPeerSocket == NULL )
+        {
+            pxSocket->u.xTCP.pxPeerSocket = pxNewSocket;
+        }
+
+        FreeRTOS_debug_printf( ( "Gain: Socket %u now has %u / %u child%s me: %p parent: %p peer: %p\n",
                                  pxSocket->usLocalPort,
                                  pxSocket->u.xTCP.usChildCount,
                                  pxSocket->u.xTCP.usBacklog,
-                                 ( pxSocket->u.xTCP.usChildCount == 1U ) ? "" : "ren" ) );
+                                 ( pxSocket->u.xTCP.usChildCount == 1U ) ? "" : "ren",
+                                 ( void * ) pxNewSocket,
+                                 ( void * ) pxSocket,
+                                 pxSocket ? ( void * ) pxSocket->u.xTCP.pxPeerSocket : NULL ) );
 
         /* Now bind the child socket to the same port as the listening socket. */
         if( vSocketBind( pxNewSocket, &xAddress, sizeof( xAddress ), pdTRUE ) != 0 )

diff --git a/test/cbmc/proofs/TCP/prvTCPHandleState/Makefile.json b/test/cbmc/proofs/TCP/prvTCPHandleState/Makefile.json
@@ -39,6 +39,7 @@
   "OBJS":
   [
     "$(ENTRY)_harness.goto",
+    "$(FREERTOS_PLUS_TCP)/test/FreeRTOS-Kernel/list.goto",
     "$(FREERTOS_PLUS_TCP)/source/FreeRTOS_TCP_IP.goto",
     "$(FREERTOS_PLUS_TCP)/source/FreeRTOS_TCP_State_Handling.goto",
     "$(FREERTOS_PLUS_TCP)/source/FreeRTOS_TCP_Reception.goto",

diff --git a/test/cbmc/proofs/TCP/prvTCPHandleState/TCPHandleState_harness.c b/test/cbmc/proofs/TCP/prvTCPHandleState/TCPHandleState_harness.c
@@ -91,6 +91,9 @@ void harness()
     FreeRTOS_Socket_t xSck;
     xSocketToListen = &xSck;
 
+    /* Call to init the socket list. */
+    vListInitialise( &xBoundTCPSocketsList );
+
     if( ensure_memory_is_valid( pxNetworkBuffer, bufferSize ) )
     {
         /* Allocates min. buffer size required for the proof */

diff --git a/test/cbmc/proofs/TCP/prvTCPPrepareSend/Makefile.json b/test/cbmc/proofs/TCP/prvTCPPrepareSend/Makefile.json
@@ -31,15 +31,21 @@
   "CBMCFLAGS":
   [
     "--unwind 1",
+    "--unwindset __CPROVER_file_local_FreeRTOS_TCP_IP_c_vTCPRemoveTCPChild.0:1",
     "--nondet-static"
   ],
   "OBJS":
   [
     "$(ENTRY)_harness.goto",
+    "$(FREERTOS_PLUS_TCP)/test/FreeRTOS-Kernel/list.goto",
     "$(FREERTOS_PLUS_TCP)/source/FreeRTOS_IP.goto",
     "$(FREERTOS_PLUS_TCP)/source/FreeRTOS_TCP_IP.goto",
     "$(FREERTOS_PLUS_TCP)/source/FreeRTOS_TCP_Transmission.goto"
   ],
+  "OPT":
+  [
+    "--export-file-local-symbols"
+  ],
   "DEF":
   [
     "FREERTOS_TCP_ENABLE_VERIFICATION"

diff --git a/test/cbmc/proofs/TCP/prvTCPPrepareSend/TCPPrepareSend_harness.c b/test/cbmc/proofs/TCP/prvTCPPrepareSend/TCPPrepareSend_harness.c
@@ -94,6 +94,9 @@ void harness()
 
     UBaseType_t uxOptionsLength;
 
+    /* Call to init the socket list. */
+    vListInitialise( &xBoundTCPSocketsList );
+
     if( pxSocket )
     {
         publicTCPPrepareSend( pxSocket, &pxNetworkBuffer, uxOptionsLength );

diff --git a/test/unit-test/FreeRTOS_Sockets/FreeRTOS_Sockets_TCP_API_utest.c b/test/unit-test/FreeRTOS_Sockets/FreeRTOS_Sockets_TCP_API_utest.c
@@ -128,6 +128,34 @@ void test_FreeRTOS_accept_ClientSocketTaken( void )
 
     pxReturn = FreeRTOS_accept( &xServerSocket, &xAddress, &xAddressLength );
     TEST_ASSERT_EQUAL( NULL, pxReturn );
+}
+
+/**
+ * @brief Client socket is already taken.
+ */
+void test_FreeRTOS_accept_PeerSocketNullWithReuseSocket( void )
+{
+    FreeRTOS_Socket_t xServerSocket, * pxReturn, xPeerSocket;
+    struct freertos_sockaddr xAddress;
+    socklen_t xAddressLength;
+
+    memset( &xServerSocket, 0, sizeof( xServerSocket ) );
+    memset( &xPeerSocket, 0, sizeof( xPeerSocket ) );
+
+    /* Invalid Protocol */
+    listLIST_ITEM_CONTAINER_ExpectAnyArgsAndReturn( &xBoundTCPSocketsList );
+    xServerSocket.ucProtocol = FREERTOS_IPPROTO_TCP;
+    xServerSocket.u.xTCP.eTCPState = eTCP_LISTEN;
+
+    xServerSocket.u.xTCP.pxPeerSocket = NULL;
+    xServerSocket.u.xTCP.bits.bPassAccept = pdTRUE_UNSIGNED;
+    xServerSocket.u.xTCP.bits.bReuseSocket = pdTRUE_UNSIGNED;
+
+    vTaskSuspendAll_Expect();
+    xTaskResumeAll_ExpectAndReturn( pdFALSE );
+
+    pxReturn = FreeRTOS_accept( &xServerSocket, &xAddress, &xAddressLength );
+    TEST_ASSERT_EQUAL( &xServerSocket, pxReturn );
     TEST_ASSERT_EQUAL( NULL, xServerSocket.u.xTCP.pxPeerSocket );
 }