Fix freed memory being reused (FreeRTOS#1148)

tony-josi-aws · web-flow · commit ba6ba81f6487 · 2024-06-04T10:50:55.000+05:30
* Add changes

* Fix build

* TCP API utests

* Fix UTs

* Fix state handling APIs

* Fix CBMC proofs

* Fix formatting

* Updating with review feedback
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 );
 }
 
diff --git a/test/unit-test/FreeRTOS_TCP_IP/FreeRTOS_TCP_IP_utest.c b/test/unit-test/FreeRTOS_TCP_IP/FreeRTOS_TCP_IP_utest.c
diff --git a/test/unit-test/FreeRTOS_TCP_IP/TCP_IP_list_macros.h b/test/unit-test/FreeRTOS_TCP_IP/TCP_IP_list_macros.h
diff --git a/test/unit-test/FreeRTOS_TCP_State_Handling/FreeRTOS_TCP_State_Handling_utest.c b/test/unit-test/FreeRTOS_TCP_State_Handling/FreeRTOS_TCP_State_Handling_utest.c

Original file line number	Diff line number	Diff line change
`@@ -3891,6 +3891,12 @@ void vSocketWakeUpUser( FreeRTOS_Socket_t * pxSocket )`
`3891`	`3891`	`if( pxParentSocket->u.xTCP.bits.bReuseSocket == pdFALSE_UNSIGNED )`
`3892`	`3892`	`{`
`3893`	`3893`	`pxClientSocket = pxParentSocket->u.xTCP.pxPeerSocket;`
	`3894`	`+`
	`3895`	`+ if( pxClientSocket != NULL )`
	`3896`	`+ {`
	`3897`	`+ FreeRTOS_printf( ( "prvAcceptWaitClient: client %p parent %p\n",`
	`3898`	`+ ( void * ) pxClientSocket, ( void * ) pxParentSocket ) );`
	`3899`	`+ }`
`3894`	`3900`	`}`
`3895`	`3901`	`else`
`3896`	`3902`	`{`
`@@ -3899,11 +3905,14 @@ void vSocketWakeUpUser( FreeRTOS_Socket_t * pxSocket )`
`3899`	`3905`
`3900`	`3906`	`if( pxClientSocket != NULL )`
`3901`	`3907`	`{`
`3902`		`- pxParentSocket->u.xTCP.pxPeerSocket = NULL;`
`3903`		`-`
`3904`	`3908`	`/* Is it still not taken ? */`
`3905`	`3909`	`if( pxClientSocket->u.xTCP.bits.bPassAccept != pdFALSE_UNSIGNED )`
`3906`	`3910`	`{`
	`3911`	`+ if( pxParentSocket->u.xTCP.pxPeerSocket != NULL )`
	`3912`	`+ {`
	`3913`	`+ pxParentSocket->u.xTCP.pxPeerSocket = NULL;`
	`3914`	`+ }`
	`3915`	`+`
`3907`	`3916`	`pxClientSocket->u.xTCP.bits.bPassAccept = pdFALSE_UNSIGNED;`
`3908`	`3917`	`}`
`3909`	`3918`	`else`
Original file line number	Diff line number	Diff line change
`@@ -39,6 +39,7 @@`
`39`	`39`	`"OBJS":`
`40`	`40`	`[`
`41`	`41`	`"$(ENTRY)_harness.goto",`
	`42`	`+ "$(FREERTOS_PLUS_TCP)/test/FreeRTOS-Kernel/list.goto",`
`42`	`43`	`"$(FREERTOS_PLUS_TCP)/source/FreeRTOS_TCP_IP.goto",`
`43`	`44`	`"$(FREERTOS_PLUS_TCP)/source/FreeRTOS_TCP_State_Handling.goto",`
`44`	`45`	`"$(FREERTOS_PLUS_TCP)/source/FreeRTOS_TCP_Reception.goto",`
Original file line number	Diff line number	Diff line change
`@@ -91,6 +91,9 @@ void harness()`
`91`	`91`	`FreeRTOS_Socket_t xSck;`
`92`	`92`	`xSocketToListen = &xSck;`
`93`	`93`
	`94`	`+ /* Call to init the socket list. */`
	`95`	`+ vListInitialise( &xBoundTCPSocketsList );`
	`96`	`+`
`94`	`97`	`if( ensure_memory_is_valid( pxNetworkBuffer, bufferSize ) )`
`95`	`98`	`{`
`96`	`99`	`/* Allocates min. buffer size required for the proof */`
Original file line number	Diff line number	Diff line change
`@@ -94,6 +94,9 @@ void harness()`
`94`	`94`
`95`	`95`	`UBaseType_t uxOptionsLength;`
`96`	`96`
	`97`	`+ /* Call to init the socket list. */`
	`98`	`+ vListInitialise( &xBoundTCPSocketsList );`
	`99`	`+`
`97`	`100`	`if( pxSocket )`
`98`	`101`	`{`
`99`	`102`	`publicTCPPrepareSend( pxSocket, &pxNetworkBuffer, uxOptionsLength );`