Skip to content

Commit ba6ba81

Browse files
tony-josi-awstony-josi-aws
authored
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
1 parent bfd8514 commit ba6ba81

File tree

11 files changed

+408
-29
lines changed

11 files changed

+408
-29
lines changed

‎source/FreeRTOS_Sockets.c

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3891,6 +3891,12 @@ void vSocketWakeUpUser( FreeRTOS_Socket_t * pxSocket )
38913891
if( pxParentSocket->u.xTCP.bits.bReuseSocket == pdFALSE_UNSIGNED )
38923892
{
38933893
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+
}
38943900
}
38953901
else
38963902
{
@@ -3899,11 +3905,14 @@ void vSocketWakeUpUser( FreeRTOS_Socket_t * pxSocket )
38993905

39003906
if( pxClientSocket != NULL )
39013907
{
3902-
pxParentSocket->u.xTCP.pxPeerSocket = NULL;
3903-
39043908
/* Is it still not taken ? */
39053909
if( pxClientSocket->u.xTCP.bits.bPassAccept != pdFALSE_UNSIGNED )
39063910
{
3911+
if( pxParentSocket->u.xTCP.pxPeerSocket != NULL )
3912+
{
3913+
pxParentSocket->u.xTCP.pxPeerSocket = NULL;
3914+
}
3915+
39073916
pxClientSocket->u.xTCP.bits.bPassAccept = pdFALSE_UNSIGNED;
39083917
}
39093918
else

‎source/FreeRTOS_TCP_IP.c

Lines changed: 61 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -274,6 +274,36 @@
274274
}
275275
/*-----------------------------------------------------------*/
276276

277+
static BaseType_t vTCPRemoveTCPChild( const FreeRTOS_Socket_t * pxChildSocket )
278+
{
279+
BaseType_t xReturn = pdFALSE;
280+
const ListItem_t * pxEnd = ( ( const ListItem_t * ) &( xBoundTCPSocketsList.xListEnd ) );
281+
282+
/* MISRA Ref 11.3.1 [Misaligned access] */
283+
/* More details at: https://github.com/FreeRTOS/FreeRTOS-Plus-TCP/blob/main/MISRA.md#rule-113 */
284+
/* coverity[misra_c_2012_rule_11_3_violation] */
285+
const ListItem_t * pxIterator = ( const ListItem_t * ) listGET_HEAD_ENTRY( &xBoundTCPSocketsList );
286+
287+
while( pxIterator != pxEnd )
288+
{
289+
FreeRTOS_Socket_t * pxSocket;
290+
pxSocket = ( ( FreeRTOS_Socket_t * ) listGET_LIST_ITEM_OWNER( pxIterator ) );
291+
pxIterator = ( ListItem_t * ) listGET_NEXT( pxIterator );
292+
293+
if( ( pxSocket != pxChildSocket ) && ( pxSocket->usLocalPort == pxChildSocket->usLocalPort ) )
294+
{
295+
if( pxSocket->u.xTCP.pxPeerSocket == pxChildSocket ) /**< for server socket: child, for child socket: parent */
296+
{
297+
pxSocket->u.xTCP.pxPeerSocket = NULL;
298+
xReturn = pdTRUE;
299+
break;
300+
}
301+
}
302+
}
303+
304+
return xReturn;
305+
}
306+
277307
/**
278308
* @brief Changing to a new state. Centralised here to do specific actions such as
279309
* resetting the alive timer, calling the user's OnConnect handler to notify
@@ -440,34 +470,53 @@
440470
if( ( eTCPState == eCLOSED ) ||
441471
( eTCPState == eCLOSE_WAIT ) )
442472
{
473+
BaseType_t xMustClear = pdFALSE;
474+
BaseType_t xHasCleared = pdFALSE;
475+
476+
if( ( xParent == pxSocket ) && ( pxSocket->u.xTCP.pxPeerSocket != NULL ) )
477+
{
478+
xParent = pxSocket->u.xTCP.pxPeerSocket;
479+
}
480+
481+
if( ( xParent->u.xTCP.pxPeerSocket != NULL ) &&
482+
( xParent->u.xTCP.pxPeerSocket == pxSocket ) )
483+
{
484+
xMustClear = pdTRUE;
485+
( void ) xMustClear;
486+
}
487+
443488
/* Socket goes to status eCLOSED because of a RST.
444489
* When nobody owns the socket yet, delete it. */
490+
FreeRTOS_printf( ( "vTCPStateChange: Closing (Queued %d, Accept %d Reuse %d)\n",
491+
pxSocket->u.xTCP.bits.bPassQueued,
492+
pxSocket->u.xTCP.bits.bPassAccept,
493+
pxSocket->u.xTCP.bits.bReuseSocket ) );
494+
FreeRTOS_printf( ( "vTCPStateChange: me %p parent %p peer %p clear %d\n",
495+
( void * ) pxSocket,
496+
( void * ) xParent,
497+
xParent ? ( void * ) xParent->u.xTCP.pxPeerSocket : NULL,
498+
( int ) xMustClear ) );
499+
445500
vTaskSuspendAll();
446501
{
447502
if( ( pxSocket->u.xTCP.bits.bPassQueued != pdFALSE_UNSIGNED ) ||
448503
( pxSocket->u.xTCP.bits.bPassAccept != pdFALSE_UNSIGNED ) )
449504
{
450505
if( pxSocket->u.xTCP.bits.bReuseSocket == pdFALSE_UNSIGNED )
451506
{
507+
xHasCleared = vTCPRemoveTCPChild( pxSocket );
508+
( void ) xHasCleared;
509+
452510
pxSocket->u.xTCP.bits.bPassQueued = pdFALSE_UNSIGNED;
453511
pxSocket->u.xTCP.bits.bPassAccept = pdFALSE_UNSIGNED;
454-
}
455-
456-
( void ) xTaskResumeAll();
457-
458-
FreeRTOS_printf( ( "vTCPStateChange: Closing socket\n" ) );
459-
460-
if( pxSocket->u.xTCP.bits.bReuseSocket == pdFALSE_UNSIGNED )
461-
{
462512
configASSERT( xIsCallingFromIPTask() != pdFALSE );
463513
vSocketCloseNextTime( pxSocket );
464514
}
465515
}
466-
else
467-
{
468-
( void ) xTaskResumeAll();
469-
}
470516
}
517+
( void ) xTaskResumeAll();
518+
FreeRTOS_printf( ( "vTCPStateChange: xHasCleared = %d\n",
519+
( int ) xHasCleared ) );
471520
}
472521

473522
if( ( eTCPState == eCLOSE_WAIT ) && ( pxSocket->u.xTCP.bits.bReuseSocket == pdTRUE_UNSIGNED ) )

‎source/FreeRTOS_TCP_State_Handling.c

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1021,11 +1021,19 @@
10211021

10221022
pxSocket->u.xTCP.usChildCount++;
10231023

1024-
FreeRTOS_debug_printf( ( "Gain: Socket %u now has %u / %u child%s\n",
1024+
if( pxSocket->u.xTCP.pxPeerSocket == NULL )
1025+
{
1026+
pxSocket->u.xTCP.pxPeerSocket = pxNewSocket;
1027+
}
1028+
1029+
FreeRTOS_debug_printf( ( "Gain: Socket %u now has %u / %u child%s me: %p parent: %p peer: %p\n",
10251030
pxSocket->usLocalPort,
10261031
pxSocket->u.xTCP.usChildCount,
10271032
pxSocket->u.xTCP.usBacklog,
1028-
( pxSocket->u.xTCP.usChildCount == 1U ) ? "" : "ren" ) );
1033+
( pxSocket->u.xTCP.usChildCount == 1U ) ? "" : "ren",
1034+
( void * ) pxNewSocket,
1035+
( void * ) pxSocket,
1036+
pxSocket ? ( void * ) pxSocket->u.xTCP.pxPeerSocket : NULL ) );
10291037

10301038
/* Now bind the child socket to the same port as the listening socket. */
10311039
if( vSocketBind( pxNewSocket, &xAddress, sizeof( xAddress ), pdTRUE ) != 0 )

‎test/cbmc/proofs/TCP/prvTCPHandleState/Makefile.json

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@
3939
"OBJS":
4040
[
4141
"$(ENTRY)_harness.goto",
42+
"$(FREERTOS_PLUS_TCP)/test/FreeRTOS-Kernel/list.goto",
4243
"$(FREERTOS_PLUS_TCP)/source/FreeRTOS_TCP_IP.goto",
4344
"$(FREERTOS_PLUS_TCP)/source/FreeRTOS_TCP_State_Handling.goto",
4445
"$(FREERTOS_PLUS_TCP)/source/FreeRTOS_TCP_Reception.goto",

‎test/cbmc/proofs/TCP/prvTCPHandleState/TCPHandleState_harness.c

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -91,6 +91,9 @@ void harness()
9191
FreeRTOS_Socket_t xSck;
9292
xSocketToListen = &xSck;
9393

94+
/* Call to init the socket list. */
95+
vListInitialise( &xBoundTCPSocketsList );
96+
9497
if( ensure_memory_is_valid( pxNetworkBuffer, bufferSize ) )
9598
{
9699
/* Allocates min. buffer size required for the proof */

‎test/cbmc/proofs/TCP/prvTCPPrepareSend/Makefile.json

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,15 +31,21 @@
3131
"CBMCFLAGS":
3232
[
3333
"--unwind 1",
34+
"--unwindset __CPROVER_file_local_FreeRTOS_TCP_IP_c_vTCPRemoveTCPChild.0:1",
3435
"--nondet-static"
3536
],
3637
"OBJS":
3738
[
3839
"$(ENTRY)_harness.goto",
40+
"$(FREERTOS_PLUS_TCP)/test/FreeRTOS-Kernel/list.goto",
3941
"$(FREERTOS_PLUS_TCP)/source/FreeRTOS_IP.goto",
4042
"$(FREERTOS_PLUS_TCP)/source/FreeRTOS_TCP_IP.goto",
4143
"$(FREERTOS_PLUS_TCP)/source/FreeRTOS_TCP_Transmission.goto"
4244
],
45+
"OPT":
46+
[
47+
"--export-file-local-symbols"
48+
],
4349
"DEF":
4450
[
4551
"FREERTOS_TCP_ENABLE_VERIFICATION"

‎test/cbmc/proofs/TCP/prvTCPPrepareSend/TCPPrepareSend_harness.c

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,9 @@ void harness()
9494

9595
UBaseType_t uxOptionsLength;
9696

97+
/* Call to init the socket list. */
98+
vListInitialise( &xBoundTCPSocketsList );
99+
97100
if( pxSocket )
98101
{
99102
publicTCPPrepareSend( pxSocket, &pxNetworkBuffer, uxOptionsLength );

‎test/unit-test/FreeRTOS_Sockets/FreeRTOS_Sockets_TCP_API_utest.c

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -128,6 +128,34 @@ void test_FreeRTOS_accept_ClientSocketTaken( void )
128128

129129
pxReturn = FreeRTOS_accept( &xServerSocket, &xAddress, &xAddressLength );
130130
TEST_ASSERT_EQUAL( NULL, pxReturn );
131+
}
132+
133+
/**
134+
* @brief Client socket is already taken.
135+
*/
136+
void test_FreeRTOS_accept_PeerSocketNullWithReuseSocket( void )
137+
{
138+
FreeRTOS_Socket_t xServerSocket, * pxReturn, xPeerSocket;
139+
struct freertos_sockaddr xAddress;
140+
socklen_t xAddressLength;
141+
142+
memset( &xServerSocket, 0, sizeof( xServerSocket ) );
143+
memset( &xPeerSocket, 0, sizeof( xPeerSocket ) );
144+
145+
/* Invalid Protocol */
146+
listLIST_ITEM_CONTAINER_ExpectAnyArgsAndReturn( &xBoundTCPSocketsList );
147+
xServerSocket.ucProtocol = FREERTOS_IPPROTO_TCP;
148+
xServerSocket.u.xTCP.eTCPState = eTCP_LISTEN;
149+
150+
xServerSocket.u.xTCP.pxPeerSocket = NULL;
151+
xServerSocket.u.xTCP.bits.bPassAccept = pdTRUE_UNSIGNED;
152+
xServerSocket.u.xTCP.bits.bReuseSocket = pdTRUE_UNSIGNED;
153+
154+
vTaskSuspendAll_Expect();
155+
xTaskResumeAll_ExpectAndReturn( pdFALSE );
156+
157+
pxReturn = FreeRTOS_accept( &xServerSocket, &xAddress, &xAddressLength );
158+
TEST_ASSERT_EQUAL( &xServerSocket, pxReturn );
131159
TEST_ASSERT_EQUAL( NULL, xServerSocket.u.xTCP.pxPeerSocket );
132160
}
133161

0 commit comments

Comments
 (0)