From 04e1c603629a279ea2c218f00b69b42f00b47f26 Mon Sep 17 00:00:00 2001 From: Hein Tibosch Date: Fri, 8 Nov 2019 17:23:35 +0800 Subject: [PATCH 1/4] FreeRTOS+TCP: Let xDataLength always mean the same, total number of bytes in a packet --- .../freertos_plus_tcp/source/FreeRTOS_IP.c | 3 ++- .../source/FreeRTOS_Sockets.c | 3 ++- .../source/FreeRTOS_UDP_IP.c | 20 ++++++++++++++----- 3 files changed, 19 insertions(+), 7 deletions(-) diff --git a/libraries/freertos_plus/standard/freertos_plus_tcp/source/FreeRTOS_IP.c b/libraries/freertos_plus/standard/freertos_plus_tcp/source/FreeRTOS_IP.c index 1c45cbd314c..02c1179cee0 100644 --- a/libraries/freertos_plus/standard/freertos_plus_tcp/source/FreeRTOS_IP.c +++ b/libraries/freertos_plus/standard/freertos_plus_tcp/source/FreeRTOS_IP.c @@ -1122,7 +1122,8 @@ void FreeRTOS_SetAddressConfiguration( const uint32_t *pulIPAddress, const uint3 pxNetworkBuffer->pucEthernetBuffer[ ipSOCKET_OPTIONS_OFFSET ] = FREERTOS_SO_UDPCKSUM_OUT; pxNetworkBuffer->ulIPAddress = ulIPAddress; pxNetworkBuffer->usPort = ipPACKET_CONTAINS_ICMP_DATA; - pxNetworkBuffer->xDataLength = xNumberOfBytesToSend + sizeof( ICMPHeader_t ); + /* xDataLength is the size of the total packet, including the Ethernet header. */ + pxNetworkBuffer->xDataLength = xNumberOfBytesToSend + sizeof( ICMPPacket_t ); /* Send to the stack. */ xStackTxEvent.pvData = pxNetworkBuffer; diff --git a/libraries/freertos_plus/standard/freertos_plus_tcp/source/FreeRTOS_Sockets.c b/libraries/freertos_plus/standard/freertos_plus_tcp/source/FreeRTOS_Sockets.c index 9d6ce5bbdb6..c51878ad963 100644 --- a/libraries/freertos_plus/standard/freertos_plus_tcp/source/FreeRTOS_Sockets.c +++ b/libraries/freertos_plus/standard/freertos_plus_tcp/source/FreeRTOS_Sockets.c @@ -833,7 +833,8 @@ FreeRTOS_Socket_t *pxSocket; if( pxNetworkBuffer != NULL ) { - pxNetworkBuffer->xDataLength = xTotalDataLength; + /* xDataLength is the size of the total packet, including the Ethernet header. */ + pxNetworkBuffer->xDataLength = xTotalDataLength + sizeof( UDPPacket_t ); pxNetworkBuffer->usPort = pxDestinationAddress->sin_port; pxNetworkBuffer->usBoundPort = ( uint16_t ) socketGET_SOCKET_PORT( pxSocket ); pxNetworkBuffer->ulIPAddress = pxDestinationAddress->sin_addr; diff --git a/libraries/freertos_plus/standard/freertos_plus_tcp/source/FreeRTOS_UDP_IP.c b/libraries/freertos_plus/standard/freertos_plus_tcp/source/FreeRTOS_UDP_IP.c index 0458fdf3a16..7ef37ef2d54 100644 --- a/libraries/freertos_plus/standard/freertos_plus_tcp/source/FreeRTOS_UDP_IP.c +++ b/libraries/freertos_plus/standard/freertos_plus_tcp/source/FreeRTOS_UDP_IP.c @@ -78,10 +78,22 @@ UDPPacket_t *pxUDPPacket; IPHeader_t *pxIPHeader; eARPLookupResult_t eReturned; uint32_t ulIPAddress = pxNetworkBuffer->ulIPAddress; +size_t uxPayloadSize; /* Map the UDP packet onto the start of the frame. */ pxUDPPacket = ( UDPPacket_t * ) pxNetworkBuffer->pucEthernetBuffer; +#if ipconfigSUPPORT_OUTGOING_PINGS == 1 + if( pxNetworkBuffer->usPort == ipPACKET_CONTAINS_ICMP_DATA ) + { + uxPayloadSize = pxNetworkBuffer->xDataLength - sizeof( ICMPPacket_t ); + } + else +#endif + { + uxPayloadSize = pxNetworkBuffer->xDataLength - sizeof( UDPPacket_t ); + } + /* Determine the ARP cache status for the requested IP address. */ eReturned = eARPGetCacheEntry( &( ulIPAddress ), &( pxUDPPacket->xEthernetHeader.xDestinationAddress ) ); @@ -109,7 +121,7 @@ uint32_t ulIPAddress = pxNetworkBuffer->ulIPAddress; pxUDPHeader->usDestinationPort = pxNetworkBuffer->usPort; pxUDPHeader->usSourcePort = pxNetworkBuffer->usBoundPort; - pxUDPHeader->usLength = ( uint16_t ) ( pxNetworkBuffer->xDataLength + sizeof( UDPHeader_t ) ); + pxUDPHeader->usLength = ( uint16_t ) ( uxPayloadSize + sizeof( UDPHeader_t ) ); pxUDPHeader->usLength = FreeRTOS_htons( pxUDPHeader->usLength ); pxUDPHeader->usChecksum = 0u; } @@ -143,16 +155,14 @@ uint32_t ulIPAddress = pxNetworkBuffer->ulIPAddress; if( pxNetworkBuffer->usPort == ipPACKET_CONTAINS_ICMP_DATA ) { pxIPHeader->ucProtocol = ipPROTOCOL_ICMP; - pxIPHeader->usLength = ( uint16_t ) ( pxNetworkBuffer->xDataLength + sizeof( IPHeader_t ) ); + pxIPHeader->usLength = ( uint16_t ) ( uxPayloadSize + sizeof( IPHeader_t ) + sizeof( ICMPHeader_t ) ); } else #endif /* ipconfigSUPPORT_OUTGOING_PINGS */ { - pxIPHeader->usLength = ( uint16_t ) ( pxNetworkBuffer->xDataLength + sizeof( IPHeader_t ) + sizeof( UDPHeader_t ) ); + pxIPHeader->usLength = ( uint16_t ) ( uxPayloadSize + sizeof( IPHeader_t ) + sizeof( UDPHeader_t ) ); } - /* The total transmit size adds on the Ethernet header. */ - pxNetworkBuffer->xDataLength = pxIPHeader->usLength + sizeof( EthernetHeader_t ); pxIPHeader->usLength = FreeRTOS_htons( pxIPHeader->usLength ); /* HT:endian: changed back to network endian */ pxIPHeader->ulDestinationIPAddress = pxNetworkBuffer->ulIPAddress; From 3248a18c72d37f63305340314359df16023da46f Mon Sep 17 00:00:00 2001 From: Hein Tibosch Date: Sat, 9 Nov 2019 16:52:55 +0800 Subject: [PATCH 2/4] NetworkBuffer.xDataLength also corrected for the received UDP packets --- .../freertos_plus_tcp/source/FreeRTOS_ARP.c | 10 ++++----- .../freertos_plus_tcp/source/FreeRTOS_DNS.c | 22 ++++++++----------- .../freertos_plus_tcp/source/FreeRTOS_IP.c | 11 +++++----- .../source/FreeRTOS_Sockets.c | 2 +- 4 files changed, 21 insertions(+), 24 deletions(-) diff --git a/libraries/freertos_plus/standard/freertos_plus_tcp/source/FreeRTOS_ARP.c b/libraries/freertos_plus/standard/freertos_plus_tcp/source/FreeRTOS_ARP.c index b91f87d1ec4..e35e3d72148 100644 --- a/libraries/freertos_plus/standard/freertos_plus_tcp/source/FreeRTOS_ARP.c +++ b/libraries/freertos_plus/standard/freertos_plus_tcp/source/FreeRTOS_ARP.c @@ -605,11 +605,11 @@ void vARPGenerateRequestPacket( NetworkBufferDescriptor_t * const pxNetworkBuffe { ARPPacket_t *pxARPPacket; - /* Buffer allocation ensures that buffers always have space - for an ARP packet. See buffer allocation implementations 1 - and 2 under portable/BufferManagement. */ - configASSERT( pxNetworkBuffer ); - configASSERT( pxNetworkBuffer->xDataLength >= sizeof(ARPPacket_t) ); + /* Buffer allocation ensures that buffers always have space + for an ARP packet. See buffer allocation implementations 1 + and 2 under portable/BufferManagement. */ + configASSERT( pxNetworkBuffer ); + configASSERT( pxNetworkBuffer->xDataLength >= sizeof(ARPPacket_t) ); pxARPPacket = ( ARPPacket_t * ) pxNetworkBuffer->pucEthernetBuffer; diff --git a/libraries/freertos_plus/standard/freertos_plus_tcp/source/FreeRTOS_DNS.c b/libraries/freertos_plus/standard/freertos_plus_tcp/source/FreeRTOS_DNS.c index e511ca32433..2f255c794bd 100644 --- a/libraries/freertos_plus/standard/freertos_plus_tcp/source/FreeRTOS_DNS.c +++ b/libraries/freertos_plus/standard/freertos_plus_tcp/source/FreeRTOS_DNS.c @@ -912,15 +912,16 @@ for testing purposes, by the module iot_test_freertos_tcp.c uint32_t ulDNSHandlePacket( NetworkBufferDescriptor_t *pxNetworkBuffer ) { DNSMessage_t *pxDNSMessageHeader; +size_t uxPayloadSize = pxNetworkBuffer->xDataLength - sizeof( UDPPacket_t ); - if( pxNetworkBuffer->xDataLength >= sizeof( DNSMessage_t ) ) + if( uxPayloadSize >= sizeof( DNSMessage_t ) ) { pxDNSMessageHeader = ( DNSMessage_t * ) ( pxNetworkBuffer->pucEthernetBuffer + sizeof( UDPPacket_t ) ); /* The parameter pdFALSE indicates that the reply was not expected. */ prvParseDNSReply( ( uint8_t * ) pxDNSMessageHeader, - pxNetworkBuffer->xDataLength, + uxPayloadSize, pdFALSE ); } @@ -935,11 +936,12 @@ DNSMessage_t *pxDNSMessageHeader; { UDPPacket_t *pxUDPPacket = ( UDPPacket_t * ) pxNetworkBuffer->pucEthernetBuffer; uint8_t *pucUDPPayloadBuffer = pxNetworkBuffer->pucEthernetBuffer + sizeof( UDPPacket_t ); + size_t uxPayloadSize = pxNetworkBuffer->xDataLength - sizeof( UDPPacket_t ); /* The network buffer data length has already been set to the length of the UDP payload. */ prvTreatNBNS( pucUDPPayloadBuffer, - pxNetworkBuffer->xDataLength, + uxPayloadSize, pxUDPPacket->xIPHeader.ulSourceIPAddress ); /* The packet was not consumed. */ @@ -1168,9 +1170,7 @@ BaseType_t xDoStore = xExpected; { BaseType_t xDataLength = uxBufferLength + sizeof( UDPHeader_t ) + sizeof( EthernetHeader_t ) + sizeof( IPHeader_t ); - /* The field xDataLength was set to the length of the UDP payload. - The answer (reply) will be longer than the request, so the packet - must be duplicaed into a bigger buffer */ + /* Set the size of the outgoing packet. */ pxNetworkBuffer->xDataLength = xDataLength; pxNewBuffer = pxDuplicateNetworkBufferWithDescriptor( pxNetworkBuffer, xDataLength + 16 ); @@ -1326,14 +1326,10 @@ BaseType_t xDoStore = xExpected; if( ( xBufferAllocFixedSize == pdFALSE ) && ( pxNetworkBuffer != NULL ) ) { NetworkBufferDescriptor_t *pxNewBuffer; - BaseType_t xDataLength = pxNetworkBuffer->xDataLength + sizeof( UDPHeader_t ) + - sizeof( EthernetHeader_t ) + sizeof( IPHeader_t ); - /* The field xDataLength was set to the length of the UDP payload. - The answer (reply) will be longer than the request, so the packet - must be duplicated into a bigger buffer */ - pxNetworkBuffer->xDataLength = xDataLength; - pxNewBuffer = pxDuplicateNetworkBufferWithDescriptor( pxNetworkBuffer, xDataLength + 16 ); + /* The field xDataLength was set to the total length of the UDP packet, + i.e. the payload size plus sizeof( UDPPacket_t ). */ + pxNewBuffer = pxDuplicateNetworkBufferWithDescriptor( pxNetworkBuffer, pxNetworkBuffer->xDataLength + 16 ); if( pxNewBuffer != NULL ) { diff --git a/libraries/freertos_plus/standard/freertos_plus_tcp/source/FreeRTOS_IP.c b/libraries/freertos_plus/standard/freertos_plus_tcp/source/FreeRTOS_IP.c index 02c1179cee0..23d1b66fc9a 100644 --- a/libraries/freertos_plus/standard/freertos_plus_tcp/source/FreeRTOS_IP.c +++ b/libraries/freertos_plus/standard/freertos_plus_tcp/source/FreeRTOS_IP.c @@ -1612,8 +1612,9 @@ uint8_t ucProtocol; /* Only proceed if the payload length indicated in the header appears to be valid. */ - if ( pxNetworkBuffer->xDataLength >= sizeof( UDPPacket_t ) ) + if ( ( pxNetworkBuffer->xDataLength >= sizeof( UDPPacket_t ) ) && ( FreeRTOS_ntohs( pxUDPPacket->xUDPHeader.usLength ) >= sizeof( UDPHeader_t ) ) ) { + size_t uxPayloadSize_1, uxPayloadSize_2; /* Ensure that downstream UDP packet handling has the lesser * of: the actual network buffer Ethernet frame length, or * the sender's UDP packet header payload length, minus the @@ -1624,11 +1625,11 @@ uint8_t ucProtocol; * the IP header, and the size of the UDP header. */ - pxNetworkBuffer->xDataLength -= sizeof( UDPPacket_t ); - if( ( FreeRTOS_ntohs( pxUDPPacket->xUDPHeader.usLength ) - sizeof( UDPHeader_t ) ) < - pxNetworkBuffer->xDataLength ) + uxPayloadSize_1 = pxNetworkBuffer->xDataLength - sizeof( UDPPacket_t ); + uxPayloadSize_2 = FreeRTOS_ntohs( pxUDPPacket->xUDPHeader.usLength ) - sizeof( UDPHeader_t ); + if( uxPayloadSize_1 > uxPayloadSize_2 ) { - pxNetworkBuffer->xDataLength = FreeRTOS_ntohs( pxUDPPacket->xUDPHeader.usLength ) - sizeof( UDPHeader_t ); + pxNetworkBuffer->xDataLength = uxPayloadSize_2 + sizeof( UDPPacket_t ); } /* Fields in pxNetworkBuffer (usPort, ulIPAddress) are network order. */ diff --git a/libraries/freertos_plus/standard/freertos_plus_tcp/source/FreeRTOS_Sockets.c b/libraries/freertos_plus/standard/freertos_plus_tcp/source/FreeRTOS_Sockets.c index c51878ad963..9d70cee1738 100644 --- a/libraries/freertos_plus/standard/freertos_plus_tcp/source/FreeRTOS_Sockets.c +++ b/libraries/freertos_plus/standard/freertos_plus_tcp/source/FreeRTOS_Sockets.c @@ -704,7 +704,7 @@ EventBits_t xEventBits = ( EventBits_t ) 0; /* The returned value is the data length, which may have been capped to the receive buffer size. */ - lReturn = ( int32_t ) pxNetworkBuffer->xDataLength; + lReturn = ( int32_t ) pxNetworkBuffer->xDataLength - sizeof( UDPPacket_t ); if( pxSourceAddress != NULL ) { From 0fa525cc32317f7041e229632239b2d32fa7ac45 Mon Sep 17 00:00:00 2001 From: Hein Tibosch Date: Tue, 12 Nov 2019 17:09:25 +0800 Subject: [PATCH 3/4] Replaced 2 magic numbers with a sizeof statement --- .../standard/freertos_plus_tcp/source/FreeRTOS_DNS.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/libraries/freertos_plus/standard/freertos_plus_tcp/source/FreeRTOS_DNS.c b/libraries/freertos_plus/standard/freertos_plus_tcp/source/FreeRTOS_DNS.c index 2f255c794bd..c2f2e0216a6 100644 --- a/libraries/freertos_plus/standard/freertos_plus_tcp/source/FreeRTOS_DNS.c +++ b/libraries/freertos_plus/standard/freertos_plus_tcp/source/FreeRTOS_DNS.c @@ -1172,7 +1172,7 @@ BaseType_t xDoStore = xExpected; /* Set the size of the outgoing packet. */ pxNetworkBuffer->xDataLength = xDataLength; - pxNewBuffer = pxDuplicateNetworkBufferWithDescriptor( pxNetworkBuffer, xDataLength + 16 ); + pxNewBuffer = pxDuplicateNetworkBufferWithDescriptor( pxNetworkBuffer, xDataLength + sizeof( LLMNRAnswer_t ) ); if( pxNewBuffer != NULL ) { @@ -1329,7 +1329,7 @@ BaseType_t xDoStore = xExpected; /* The field xDataLength was set to the total length of the UDP packet, i.e. the payload size plus sizeof( UDPPacket_t ). */ - pxNewBuffer = pxDuplicateNetworkBufferWithDescriptor( pxNetworkBuffer, pxNetworkBuffer->xDataLength + 16 ); + pxNewBuffer = pxDuplicateNetworkBufferWithDescriptor( pxNetworkBuffer, pxNetworkBuffer->xDataLength + sizeof( NBNSAnswer_t ) ); if( pxNewBuffer != NULL ) { From 67e86d24e0f282bd21089b12d89347f90b4a8712 Mon Sep 17 00:00:00 2001 From: Hein Tibosch Date: Tue, 12 Nov 2019 17:41:03 +0800 Subject: [PATCH 4/4] Changes comments about xDataLength in downstream UDP packets --- .../freertos_plus_tcp/source/FreeRTOS_IP.c | 16 ++++++++-------- .../freertos_plus_tcp/source/FreeRTOS_Sockets.c | 8 +++++--- 2 files changed, 13 insertions(+), 11 deletions(-) diff --git a/libraries/freertos_plus/standard/freertos_plus_tcp/source/FreeRTOS_IP.c b/libraries/freertos_plus/standard/freertos_plus_tcp/source/FreeRTOS_IP.c index 23d1b66fc9a..878d3c777af 100644 --- a/libraries/freertos_plus/standard/freertos_plus_tcp/source/FreeRTOS_IP.c +++ b/libraries/freertos_plus/standard/freertos_plus_tcp/source/FreeRTOS_IP.c @@ -1615,14 +1615,14 @@ uint8_t ucProtocol; if ( ( pxNetworkBuffer->xDataLength >= sizeof( UDPPacket_t ) ) && ( FreeRTOS_ntohs( pxUDPPacket->xUDPHeader.usLength ) >= sizeof( UDPHeader_t ) ) ) { size_t uxPayloadSize_1, uxPayloadSize_2; - /* Ensure that downstream UDP packet handling has the lesser - * of: the actual network buffer Ethernet frame length, or - * the sender's UDP packet header payload length, minus the - * size of the UDP header. - * - * The size of the UDP packet structure in this implementation - * includes the size of the Ethernet header, the size of - * the IP header, and the size of the UDP header. + /* The UDP payload size can be calculated by subtracting the + * header size from `xDataLength`. + * However, the `xDataLength` may be longer that expected, + * e.g. when a small packet is padded with zero's. + * The UDP header contains a field `usLength` reflecting + * the payload size plus the UDP header ( 8 bytes ). + * Set `xDataLength` to the size of the headers, + * plus the lower of the two calculated payload sizes. */ uxPayloadSize_1 = pxNetworkBuffer->xDataLength - sizeof( UDPPacket_t ); diff --git a/libraries/freertos_plus/standard/freertos_plus_tcp/source/FreeRTOS_Sockets.c b/libraries/freertos_plus/standard/freertos_plus_tcp/source/FreeRTOS_Sockets.c index 9d70cee1738..6873af16b35 100644 --- a/libraries/freertos_plus/standard/freertos_plus_tcp/source/FreeRTOS_Sockets.c +++ b/libraries/freertos_plus/standard/freertos_plus_tcp/source/FreeRTOS_Sockets.c @@ -702,9 +702,11 @@ EventBits_t xEventBits = ( EventBits_t ) 0; } taskEXIT_CRITICAL(); - /* The returned value is the data length, which may have been capped to - the receive buffer size. */ - lReturn = ( int32_t ) pxNetworkBuffer->xDataLength - sizeof( UDPPacket_t ); + /* The returned value is the length of the payload data, which is + calculated at the total packet size minus the headers. + The validity of `xDataLength` prvProcessIPPacket has been confirmed + in 'prvProcessIPPacket()'. */ + lReturn = ( int32_t ) ( pxNetworkBuffer->xDataLength - sizeof( UDPPacket_t ) ); if( pxSourceAddress != NULL ) {