Skip to content

Commit 188a9d0

Browse files
htiboschHein TiboschAniruddhaKanhereactions-user
authored
IPv4/single: connect() should return immediately after a protocol error and other things (#559)
* IPv4/single: Let connect() return as soon as socket gets closed * Let both connect() and accept() return after a 'eSOCKET_CLOSED' event * Included hang protection of orphaned socket from PR #545 * Lexicon.txt change * Remove a variable that was not used * Update source/FreeRTOS_Sockets.c Co-authored-by: Aniruddha Kanhere <[email protected]> * Update source/FreeRTOS_TCP_IP.c Co-authored-by: Aniruddha Kanhere <[email protected]> * moved declaration to beginning of block * Update source/FreeRTOS_TCP_IP.c Co-authored-by: Aniruddha Kanhere <[email protected]> * Uncrustify: triggered by comment. * Fix unit test expectations * Avoid a recursive call to vTCPStateChange() * Uncrustify: triggered by comment. * Fix CBMC proof assumptions * Get unit-test coverage up * Fix timers unit-tests * Socket unit-test for closed socket * Fix a unit-test expectations * Fix spell check * Uncrustify: triggered by comment. * Using debug_printf in stead of printf for logging. * Use debug printf instead of printf in 2 locations Co-authored-by: Hein Tibosch <[email protected]> Co-authored-by: Aniruddha Kanhere <[email protected]> Co-authored-by: GitHub Action <[email protected]>
1 parent cb70927 commit 188a9d0

File tree

10 files changed

+474
-52
lines changed

10 files changed

+474
-52
lines changed

‎.github/lexicon.txt

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1392,6 +1392,7 @@ vrxfaultinjection
13921392
vsocketbind
13931393
vsocketclose
13941394
vsocketclosenexttime
1395+
vsocketlistennexttime
13951396
vsocketselect
13961397
vsocketwakeupuser
13971398
vtasklist
@@ -1448,6 +1449,7 @@ xcallbacklist
14481449
xcheckloopback
14491450
xclearonexit
14501451
xclientsocket
1452+
xconnected
14511453
xcount
14521454
xdatalength
14531455
xdatalengthbytes

‎source/FreeRTOS_IP_Timers.c

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -241,6 +241,9 @@ void vCheckNetworkTimers( void )
241241

242242
/* See if any socket was planned to be closed. */
243243
vSocketCloseNextTime( NULL );
244+
245+
/* See if any reusable socket needs to go back to 'eTCP_LISTEN' state. */
246+
vSocketListenNextTime( NULL );
244247
#endif /* ipconfigUSE_TCP == 1 */
245248
}
246249
/*-----------------------------------------------------------*/

‎source/FreeRTOS_Sockets.c

Lines changed: 20 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3102,6 +3102,8 @@ void vSocketWakeUpUser( FreeRTOS_Socket_t * pxSocket )
31023102
/* And wait for the result */
31033103
for( ; ; )
31043104
{
3105+
EventBits_t uxEvents;
3106+
31053107
if( xTimed == pdFALSE )
31063108
{
31073109
/* Only in the first round, check for non-blocking */
@@ -3139,7 +3141,18 @@ void vSocketWakeUpUser( FreeRTOS_Socket_t * pxSocket )
31393141
}
31403142

31413143
/* Go sleeping until we get any down-stream event */
3142-
( void ) xEventGroupWaitBits( pxSocket->xEventGroup, ( EventBits_t ) eSOCKET_CONNECT, pdTRUE /*xClearOnExit*/, pdFALSE /*xWaitAllBits*/, xRemainingTime );
3144+
uxEvents = xEventGroupWaitBits( pxSocket->xEventGroup,
3145+
( EventBits_t ) eSOCKET_CONNECT | ( EventBits_t ) eSOCKET_CLOSED,
3146+
pdTRUE /*xClearOnExit*/,
3147+
pdFALSE /*xWaitAllBits*/,
3148+
xRemainingTime );
3149+
3150+
if( ( uxEvents & eSOCKET_CLOSED ) != 0U )
3151+
{
3152+
xResult = -pdFREERTOS_ERRNO_ENOTCONN;
3153+
FreeRTOS_debug_printf( ( "FreeRTOS_connect() stopped due to an error\n" ) );
3154+
break;
3155+
}
31433156
}
31443157
}
31453158

@@ -3286,8 +3299,12 @@ void vSocketWakeUpUser( FreeRTOS_Socket_t * pxSocket )
32863299
break;
32873300
}
32883301

3289-
/* Go sleeping until we get any down-stream event */
3290-
( void ) xEventGroupWaitBits( pxSocket->xEventGroup, ( EventBits_t ) eSOCKET_ACCEPT, pdTRUE /*xClearOnExit*/, pdFALSE /*xWaitAllBits*/, xRemainingTime );
3302+
/* Put the calling task to 'sleep' until a down-stream event is received. */
3303+
( void ) xEventGroupWaitBits( pxSocket->xEventGroup,
3304+
( EventBits_t ) eSOCKET_ACCEPT,
3305+
pdTRUE /*xClearOnExit*/,
3306+
pdFALSE /*xWaitAllBits*/,
3307+
xRemainingTime );
32913308
}
32923309
}
32933310

‎source/FreeRTOS_TCP_IP.c

Lines changed: 110 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,18 @@
7575
/* MISRA Ref 8.9.1 [File scoped variables] */
7676
/* More details at: https://github.com/FreeRTOS/FreeRTOS-Plus-TCP/blob/main/MISRA.md#rule-89 */
7777
/* coverity[misra_c_2012_rule_8_9_violation] */
78-
static FreeRTOS_Socket_t * xPreviousSocket = NULL;
78+
static FreeRTOS_Socket_t * xSocketToClose = NULL;
79+
80+
/** @brief When a connection is coming in on a reusable socket, and the
81+
* SYN phase times out, the socket must be put back into eTCP_LISTEN
82+
* mode, so it can accept a new connection again.
83+
* This variable can be accessed by the IP task only. Thus, preventing any
84+
* race condition.
85+
*/
86+
/* MISRA Ref 8.9.1 [File scoped variables] */
87+
/* More details at: https://github.com/FreeRTOS/FreeRTOS-Plus-TCP/blob/main/MISRA.md#rule-89 */
88+
/* coverity[misra_c_2012_rule_8_9_violation] */
89+
static FreeRTOS_Socket_t * xSocketToListen = NULL;
7990

8091
/*
8192
* For anti-hang protection and TCP keep-alive messages. Called in two places:
@@ -110,12 +121,28 @@
110121
/* coverity[single_use] */
111122
void vSocketCloseNextTime( FreeRTOS_Socket_t * pxSocket )
112123
{
113-
if( ( xPreviousSocket != NULL ) && ( xPreviousSocket != pxSocket ) )
124+
if( ( xSocketToClose != NULL ) && ( xSocketToClose != pxSocket ) )
114125
{
115-
( void ) vSocketClose( xPreviousSocket );
126+
( void ) vSocketClose( xSocketToClose );
116127
}
117128

118-
xPreviousSocket = pxSocket;
129+
xSocketToClose = pxSocket;
130+
}
131+
/*-----------------------------------------------------------*/
132+
133+
/** @brief Postpone a call to FreeRTOS_listen() to avoid recursive calls.
134+
*
135+
* @param[in] pxSocket: The socket to be checked.
136+
*/
137+
/* coverity[single_use] */
138+
void vSocketListenNextTime( FreeRTOS_Socket_t * pxSocket )
139+
{
140+
if( ( xSocketToListen != NULL ) && ( xSocketToListen != pxSocket ) )
141+
{
142+
( void ) FreeRTOS_listen( ( Socket_t ) xSocketToListen, xSocketToListen->u.xTCP.usBacklog );
143+
}
144+
145+
xSocketToListen = pxSocket;
119146
}
120147
/*-----------------------------------------------------------*/
121148

@@ -272,39 +299,60 @@
272299
void vTCPStateChange( FreeRTOS_Socket_t * pxSocket,
273300
enum eTCP_STATE eTCPState )
274301
{
275-
FreeRTOS_Socket_t * xParent = NULL;
302+
FreeRTOS_Socket_t * xParent = pxSocket;
276303
BaseType_t bBefore = tcpNOW_CONNECTED( ( BaseType_t ) pxSocket->u.xTCP.eTCPState ); /* Was it connected ? */
277304
BaseType_t bAfter = tcpNOW_CONNECTED( ( BaseType_t ) eTCPState ); /* Is it connected now ? */
278305

279-
#if ( ipconfigHAS_DEBUG_PRINTF != 0 )
280-
BaseType_t xPreviousState = ( BaseType_t ) pxSocket->u.xTCP.eTCPState;
281-
#endif
306+
BaseType_t xPreviousState = ( BaseType_t ) pxSocket->u.xTCP.eTCPState;
307+
282308
#if ( ipconfigUSE_CALLBACKS == 1 )
283309
FreeRTOS_Socket_t * xConnected = NULL;
284310
#endif
285311

312+
if( ( ( xPreviousState == eCONNECT_SYN ) ||
313+
( xPreviousState == eSYN_FIRST ) ||
314+
( xPreviousState == eSYN_RECEIVED ) ) &&
315+
( eTCPState == eCLOSE_WAIT ) )
316+
{
317+
/* A socket was in the connecting phase but something
318+
* went wrong and it should be closed. */
319+
FreeRTOS_debug_printf( ( "Move from %s to %s\n",
320+
FreeRTOS_GetTCPStateName( xPreviousState ),
321+
FreeRTOS_GetTCPStateName( eTCPState ) ) );
322+
323+
/* Set the flag to show that it was connected before and that the
324+
* status has changed now. This will cause the control flow to go
325+
* in the below if condition.*/
326+
bBefore = pdTRUE;
327+
}
328+
286329
/* Has the connected status changed? */
287330
if( bBefore != bAfter )
288331
{
332+
/* if bPassQueued is true, this socket is an orphan until it gets connected. */
333+
if( pxSocket->u.xTCP.bits.bPassQueued != pdFALSE_UNSIGNED )
334+
{
335+
/* Find it's parent if the reuse bit is not set. */
336+
if( pxSocket->u.xTCP.bits.bReuseSocket == pdFALSE_UNSIGNED )
337+
{
338+
xParent = pxSocket->u.xTCP.pxPeerSocket;
339+
configASSERT( xParent != NULL );
340+
}
341+
}
342+
289343
/* Is the socket connected now ? */
290344
if( bAfter != pdFALSE )
291345
{
292346
/* if bPassQueued is true, this socket is an orphan until it gets connected. */
293347
if( pxSocket->u.xTCP.bits.bPassQueued != pdFALSE_UNSIGNED )
294348
{
295-
/* Now that it is connected, find it's parent. */
296-
if( pxSocket->u.xTCP.bits.bReuseSocket != pdFALSE_UNSIGNED )
297-
{
298-
xParent = pxSocket;
299-
}
300-
else
301-
{
302-
xParent = pxSocket->u.xTCP.pxPeerSocket;
303-
configASSERT( xParent != NULL );
304-
}
305-
306349
if( xParent != NULL )
307350
{
351+
/* The child socket has got connected. See if the parent
352+
* ( the listening socket ) should be signalled, or if a
353+
* call-back must be made, in which case 'xConnected' will
354+
* be set to the parent socket. */
355+
308356
if( xParent->u.xTCP.pxPeerSocket == NULL )
309357
{
310358
xParent->u.xTCP.pxPeerSocket = pxSocket;
@@ -347,6 +395,9 @@
347395
}
348396
else
349397
{
398+
/* An active connect() has succeeded. In this case there is no
399+
* ( listening ) parent socket. Signal the now connected socket. */
400+
350401
pxSocket->xEventBits |= ( EventBits_t ) eSOCKET_CONNECT;
351402

352403
#if ( ipconfigSUPPORT_SELECT_FUNCTION == 1 )
@@ -361,14 +412,14 @@
361412
}
362413
else /* bAfter == pdFALSE, connection is closed. */
363414
{
364-
/* Notify/wake-up the socket-owner by setting a semaphore. */
365-
pxSocket->xEventBits |= ( EventBits_t ) eSOCKET_CLOSED;
415+
/* Notify/wake-up the socket-owner by setting the event bits. */
416+
xParent->xEventBits |= ( EventBits_t ) eSOCKET_CLOSED;
366417

367418
#if ( ipconfigSUPPORT_SELECT_FUNCTION == 1 )
368419
{
369-
if( ( pxSocket->xSelectBits & ( EventBits_t ) eSELECT_EXCEPT ) != 0U )
420+
if( ( xParent->xSelectBits & ( EventBits_t ) eSELECT_EXCEPT ) != 0U )
370421
{
371-
pxSocket->xEventBits |= ( ( EventBits_t ) eSELECT_EXCEPT ) << SOCKET_EVENT_BIT_COUNT;
422+
xParent->xEventBits |= ( ( EventBits_t ) eSELECT_EXCEPT ) << SOCKET_EVENT_BIT_COUNT;
372423
}
373424
}
374425
#endif
@@ -393,30 +444,51 @@
393444
pxSocket->u.xTCP.usTimeout = 0U;
394445
}
395446
}
396-
else
447+
448+
if( ( eTCPState == eCLOSED ) ||
449+
( eTCPState == eCLOSE_WAIT ) )
397450
{
398-
if( ( eTCPState == eCLOSED ) ||
399-
( eTCPState == eCLOSE_WAIT ) )
451+
/* Socket goes to status eCLOSED because of a RST.
452+
* When nobody owns the socket yet, delete it. */
453+
if( ( pxSocket->u.xTCP.bits.bPassQueued != pdFALSE_UNSIGNED ) ||
454+
( pxSocket->u.xTCP.bits.bPassAccept != pdFALSE_UNSIGNED ) )
400455
{
401-
/* Socket goes to status eCLOSED because of a RST.
402-
* When nobody owns the socket yet, delete it. */
403-
if( ( pxSocket->u.xTCP.bits.bPassQueued != pdFALSE_UNSIGNED ) ||
404-
( pxSocket->u.xTCP.bits.bPassAccept != pdFALSE_UNSIGNED ) )
405-
{
406-
FreeRTOS_debug_printf( ( "vTCPStateChange: Closing socket\n" ) );
456+
FreeRTOS_debug_printf( ( "vTCPStateChange: Closing socket\n" ) );
407457

408-
if( pxSocket->u.xTCP.bits.bReuseSocket == pdFALSE_UNSIGNED )
409-
{
410-
configASSERT( xIsCallingFromIPTask() != pdFALSE );
411-
vSocketCloseNextTime( pxSocket );
412-
}
458+
if( pxSocket->u.xTCP.bits.bReuseSocket == pdFALSE_UNSIGNED )
459+
{
460+
configASSERT( xIsCallingFromIPTask() != pdFALSE );
461+
vSocketCloseNextTime( pxSocket );
413462
}
414463
}
415464
}
416465

417466
/* Fill in the new state. */
418467
pxSocket->u.xTCP.eTCPState = eTCPState;
419468

469+
if( ( eTCPState == eCLOSE_WAIT ) && ( pxSocket->u.xTCP.bits.bReuseSocket == pdTRUE_UNSIGNED ) )
470+
{
471+
switch( xPreviousState )
472+
{
473+
case eSYN_FIRST: /* 3 (server) Just created, must ACK the SYN request */
474+
case eSYN_RECEIVED: /* 4 (server) waiting for a confirming connection request */
475+
FreeRTOS_debug_printf( ( "Restoring a reuse socket port %u\n", pxSocket->usLocalPort ) );
476+
477+
/* Go back into listening mode. Set the TCP status to 'eCLOSED',
478+
* otherwise FreeRTOS_listen() will refuse the action. */
479+
pxSocket->u.xTCP.eTCPState = eCLOSED;
480+
481+
/* vSocketListenNextTime() makes sure that FreeRTOS_listen() will be called
482+
* before the IP-task handles any new message. */
483+
vSocketListenNextTime( pxSocket );
484+
break;
485+
486+
default:
487+
/* Nothing to do. */
488+
break;
489+
}
490+
}
491+
420492
/* Touch the alive timers because moving to another state. */
421493
prvTCPTouchSocket( pxSocket );
422494

@@ -585,7 +657,7 @@
585657
ulLocalIP = FreeRTOS_htonl( pxIPHeader->ulDestinationIPAddress );
586658
ulRemoteIP = FreeRTOS_htonl( pxIPHeader->ulSourceIPAddress );
587659

588-
/* Find the destination socket, and if not found: return a socket listing to
660+
/* Find the destination socket, and if not found: return a socket listening to
589661
* the destination PORT. */
590662
pxSocket = ( FreeRTOS_Socket_t * ) pxTCPSocketLookup( ulLocalIP, usLocalPort, ulRemoteIP, usRemotePort );
591663

‎source/include/FreeRTOS_IP_Private.h

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -703,6 +703,11 @@ typedef struct xSOCKET
703703
*/
704704
void vSocketCloseNextTime( FreeRTOS_Socket_t * pxSocket );
705705

706+
/*
707+
* Postpone a call to listen() by the IP-task.
708+
*/
709+
void vSocketListenNextTime( FreeRTOS_Socket_t * pxSocket );
710+
706711
/*
707712
* Lookup a TCP socket, using a multiple matching: both port numbers and
708713
* return IP address.

‎test/cbmc/proofs/parsing/ProcessReceivedTCPPacket/ProcessReceivedTCPPacket_harness.c

Lines changed: 18 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,16 @@ FreeRTOS_Socket_t * pxTCPSocketLookup( uint32_t ulLocalIP,
6565
xRetSocket->u.xTCP.txStream = safeMalloc( sizeof( StreamBuffer_t ) );
6666
xRetSocket->u.xTCP.pxPeerSocket = safeMalloc( sizeof( StreamBuffer_t ) );
6767

68+
/* This bit depicts whether the socket was supposed to be reused or not. */
69+
if( xRetSocket->u.xTCP.pxPeerSocket == NULL )
70+
{
71+
xRetSocket->u.xTCP.bits.bReuseSocket = pdTRUE_UNSIGNED;
72+
}
73+
else
74+
{
75+
xRetSocket->u.xTCP.bits.bReuseSocket = pdFALSE_UNSIGNED;
76+
}
77+
6878
if( xIsCallingFromIPTask() == pdFALSE )
6979
{
7080
xRetSocket->u.xTCP.bits.bPassQueued = pdFALSE_UNSIGNED;
@@ -94,13 +104,13 @@ void harness()
94104
{
95105
NetworkBufferDescriptor_t * pxNetworkBuffer = safeMalloc( sizeof( NetworkBufferDescriptor_t ) );
96106

97-
if( pxNetworkBuffer )
98-
{
99-
pxNetworkBuffer->pucEthernetBuffer = safeMalloc( sizeof( TCPPacket_t ) );
100-
}
107+
/* To avoid asserting on the network buffer being NULL. */
108+
__CPROVER_assume( pxNetworkBuffer != NULL );
101109

102-
if( pxNetworkBuffer && pxNetworkBuffer->pucEthernetBuffer )
103-
{
104-
xProcessReceivedTCPPacket( pxNetworkBuffer );
105-
}
110+
pxNetworkBuffer->pucEthernetBuffer = safeMalloc( sizeof( TCPPacket_t ) );
111+
112+
/* To avoid asserting on the ethernet buffer being NULL. */
113+
__CPROVER_assume( pxNetworkBuffer->pucEthernetBuffer != NULL );
114+
115+
xProcessReceivedTCPPacket( pxNetworkBuffer );
106116
}

0 commit comments

Comments
 (0)