FreeRTOS+TCP: Let xDataLength always mean "total number of bytes"

[htibosch]

Description

A network buffer (NetworkBufferDescriptor_t) has a field xDataLength. This is the total length of the buffer. It includes the 14-byte Ethernet header and everything after that.

Just recently, @aggarg wanted to add a length check in the function vARPGenerateRequestPacket(), something like:

	configASSERT( pxNetworkBuffer->xDataLength >= sizeof(ARPPacket_t) )

This function failed because there are two cases in which xDataLength has a different meaning.

• In an outgoing ICMP/ping packet, it means the number of bytes to send plus the size of an ICMP header.
• In an outgoing UDP packet, it is the payload size ( without the UDP header ).

In order to avoid confusion, I'd like to correct that: xDataLength will always contain the total number of bytes.

Checklist:

• I have tested my changes. No regression in existing tests.
• My code is Linted.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[lundinc2]

/bot run checks

[htibosch]

When UDP packets are sent or received, the field xDataLength still contained the size of the nett payload. I believe that this was the ultimate situation in which xDataLength referred to a payload size.

A simple change in FreeRTOS_Sockets.c in FreeRTOS_recvfrom():

-	lReturn = ( int32_t ) pxNetworkBuffer->xDataLength;
+	lReturn = ( int32_t ) pxNetworkBuffer->xDataLength - sizeof( UDPPacket_t );

In FreeRTOS_sendto() there is this change:

-	pxNetworkBuffer->xDataLength = xTotalDataLength;
+	pxNetworkBuffer->xDataLength = xTotalDataLength + sizeof( UDPPacket_t );

In FreeRTOS_DNS.c, added a variable uxPayloadSize in a few occasions:

-	if( pxNetworkBuffer->xDataLength >= sizeof( DNSMessage_t ) )
+	size_t uxPayloadSize = pxNetworkBuffer->xDataLength - sizeof( UDPPacket_t );
+	if( uxPayloadSize >= sizeof( DNSMessage_t ) )

In FreeRTOS_IP.c, there is an extra sanity check on the UDP field usLength:

if ( ... && FreeRTOS_ntohs( pxUDPPacket->xUDPHeader.usLength ) >= sizeof( UDPHeader_t ) )

All tested again with and without: DNS, LLMNR, NBNS, and UDP messages in both directions.
I also check the values of variables by stepping through the code.

Hein

[dcgaws]

Does the +16 represent sizeof(UDPPacket_t)? In any case, can you please use an appropriate symbolic value instead of the "magic number"?

[htibosch]

The +16 was there already, in two cases.
In the first case it means sizeof( LLMNRAnswer_t ),
in the second case it means sizeof( NBNSAnswer_t ).
Now adapted.

[dcgaws]

This line does not appear to be consistent with the big block comment above. Can you please confirm (i.e. especially since is probably an important block of code for mitigating buffer overflow risk, and it's not obvious to read)?

[dcgaws]

Hasn't the header already been removed before this function was called? Or did that happen without updating the data length? If the latter, that would appear to be a flaw in the interface (i.e. risky behavior).

[htibosch]

Hasn't the header already been removed before this function was called?

Indeed, the headers were not removed, the packet still contains them when FreeRTOS_recvfrom() is called.
Currently this function would look at a full packet ( >= 42 bytes ), where the xDataLength field would reflect the length of the payload.

A little further the function is accessing the buffer as:

    &( pxNetworkBuffer->pucEthernetBuffer[ ipUDP_PAYLOAD_OFFSET_IPv4 ] )

Note that ipUDP_PAYLOAD_OFFSET_IPv4 == sizeof( UDPPacket_t ) == 42.

The aim of this PR is to let xDataLength reflect the length of the buffer, independent from the context.

[htibosch]

Maybe some clarification about receiving UDP packets:

NetworkInterface receives a UDP packet :

42 bytes headers
10 bytes payload
 8 bytes padding
- - - - - - - - - - - -
60 bytes = xDataLength

The packet is sent to a queue and waits for the IP-task.

IP-task receives the network buffer and processes it in prvProcessIPPacket():

It corrects `xDataLength`: the padding is discarted:

42 bytes headers
10 bytes payload
- - - - - - - - - - - -
52 bytes = xDataLength

API call FreeRTOS_recvfrom():

Copy method: it copies the 10-byte payload to a user buffer.
Zero-copy: it returns a pointer to the Network Buffer at the offset of the payload.

Zero-copy: the user receives an object of type void * which can be read from ( character bytes ), and which shall be released by calling :

    void FreeRTOS_ReleaseUDPPayloadBuffer( void *pvBuffer )

We don't know in advance if ``xDataLength` is correct. Therefor we take the smaller of the 2 calculated payloads, with a minimum of 0 bytes.
( not sure if a zero-length UDP-packet has any use in real life ).
Hein

[lundinc2]

/bot run checks

[yuhui-zheng]

Since we got two approvals and tests are passing, I'm merging this PR as part of the oncall duty.